-
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] Prefer span metrics over span events #141519
Conversation
Pinging @elastic/apm-ui (Team:APM) |
@@ -52,6 +52,7 @@ export default function ApiTest({ getService }: FtrProviderContext) { | |||
environment, | |||
kuery, | |||
dependencyName, | |||
searchServiceDestinationMetrics: false, |
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 have two API tests that ensure we get the same response back regardless if searchServiceDestinationMetrics
is true or false?
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.
lgtm. Please manually double check that this does return the same data wtih and without service destination metrics.
@sqren I was adding the API test you mentioned, for span metrics, and I noticed that the values are not the same. Take a look. This is the ES response for the query. On the left querying Span events (searchServiceDestinationMetrics: false) and on the left Span metrics (searchServiceDestinationMetrics: true):
Thoughts? |
@cauemarcondes are you considering weight? These are metrics, so a single span metrics might represent multiple documents. Simply averaging the duration isn't accurate because no weight is applied. This automatically happens for histogram fields, but this is not a histogram. |
181dcc1
to
5c3761d
Compare
@dgieselaar / @sqren I made some changes to calculate the latency manually when it's using span metrics. I sum the field What do you think? FYI: I used apm-integration-test with 100% sample rate to test it. |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* [APM] Prefer span metrics over span events * [APM] Prefer span metrics over span events * using weighted_avg when span metrics * fixing span metrics * throughput fix * adding api test (cherry picked from commit dc53a59)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
* [APM] Prefer span metrics over span events * [APM] Prefer span metrics over span events * using weighted_avg when span metrics * fixing span metrics * throughput fix * adding api test (cherry picked from commit dc53a59) Co-authored-by: Cauê Marcondes <[email protected]>
* [APM] Prefer span metrics over span events * [APM] Prefer span metrics over span events * using weighted_avg when span metrics * fixing span metrics * throughput fix * adding api test
* [APM] Prefer span metrics over span events * [APM] Prefer span metrics over span events * using weighted_avg when span metrics * fixing span metrics * throughput fix * adding api test
closes #139728