-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APM] Improve logic to determine when to use the transaction.duration.summary field #171315
Merged
crespocarlos
merged 34 commits into
elastic:main
from
crespocarlos:167578-empty-latency-chart-fix
Jan 16, 2024
Merged
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
0de5219
Improve logic to determine when to use the transaction.duration.summa…
crespocarlos 6a53f6d
Merge branch 'main' of github.com:elastic/kibana into 167578-empty-la…
crespocarlos c7f7683
Simplifying the solution
crespocarlos ec4177d
Small optimization
crespocarlos 9a1d36f
Add test case
crespocarlos 83e1c55
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 0e329c9
Fix after merge
crespocarlos 0351b2f
Clean up
crespocarlos 618cddc
CR fixes
crespocarlos 0416fe9
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 72782f3
Verify when hasDurationSummaryField is true on a cluster level
crespocarlos bb2d2cd
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine a109347
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 72f76b0
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine ea01c51
Type fix
crespocarlos cf8a8f7
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 28a82ee
CR fixes
crespocarlos 3c89c0f
Revert changes to base_client
crespocarlos 1710bf6
Revert changes to base_client
crespocarlos aa2e343
Clean up
crespocarlos 7a80eb7
More clean up
crespocarlos 196724c
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine c5addd4
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine f89d228
CR fixes
crespocarlos 0709537
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 0ee3106
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine 22d3e61
Refactor get_document_sources
crespocarlos ed45132
Small improvement
crespocarlos 6b3f0bf
Clean up
crespocarlos 33efa72
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 6d5eed5
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine f207b54
Merge branch 'main' into 167578-empty-latency-chart-fix
kibanamachine 5897ee7
Rename variable
crespocarlos ecfa42c
Fix types
crespocarlos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
71 changes: 71 additions & 0 deletions
71
packages/kbn-apm-synthtrace/src/scenarios/service_summary_field_version_dependent.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { ApmFields, apm } from '@kbn/apm-synthtrace-client'; | ||
import { random } from 'lodash'; | ||
import { pipeline, Readable } from 'stream'; | ||
import semver from 'semver'; | ||
import { Scenario } from '../cli/scenario'; | ||
import { | ||
addObserverVersionTransform, | ||
deleteSummaryFieldTransform, | ||
} from '../lib/utils/transform_helpers'; | ||
import { withClient } from '../lib/utils/with_client'; | ||
|
||
const scenario: Scenario<ApmFields> = async ({ logger, versionOverride }) => { | ||
const version = versionOverride as string; | ||
const isLegacy = versionOverride && semver.lt(version, '8.7.0'); | ||
return { | ||
bootstrap: async ({ apmEsClient }) => { | ||
if (isLegacy) { | ||
apmEsClient.pipeline((base: Readable) => { | ||
const defaultPipeline = apmEsClient.getDefaultPipeline()( | ||
base | ||
) as unknown as NodeJS.ReadableStream; | ||
|
||
return pipeline( | ||
defaultPipeline, | ||
addObserverVersionTransform(version), | ||
deleteSummaryFieldTransform(), | ||
(err) => { | ||
if (err) { | ||
logger.error(err); | ||
} | ||
} | ||
); | ||
}); | ||
} | ||
}, | ||
generate: ({ range, clients: { apmEsClient } }) => { | ||
const successfulTimestamps = range.ratePerMinute(6); | ||
const instance = apm | ||
.service({ | ||
name: `java${isLegacy ? '-legacy' : ''}`, | ||
environment: 'production', | ||
agentName: 'java', | ||
}) | ||
.instance(`instance`); | ||
|
||
return withClient( | ||
apmEsClient, | ||
successfulTimestamps.generator((timestamp) => { | ||
const randomHigh = random(1000, 4000); | ||
const randomLow = random(100, randomHigh / 5); | ||
const duration = random(randomLow, randomHigh); | ||
return instance | ||
.transaction({ transactionName: 'GET /order/{id}' }) | ||
.timestamp(timestamp) | ||
.duration(duration) | ||
.success(); | ||
}) | ||
); | ||
}, | ||
}; | ||
}; | ||
|
||
export default scenario; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not check both before/after here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to - but I might be wrong. The
hasDoc
case seems to be different, at least based on the comment provided. For thedurationSummary
I think we're only interested if in the current timestamp all documents have the new field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right! Because the check is inversed (which is not possible with document sources), checking the current time range is enough.