-
-
Notifications
You must be signed in to change notification settings - Fork 444
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
Aggregate metrics for spans #3238
Conversation
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b7fa26f | 440.10 ms | 533.98 ms | 93.88 ms |
6b4dd70 | 399.62 ms | 479.08 ms | 79.47 ms |
3d8bd2b | 379.45 ms | 450.36 ms | 70.90 ms |
4e29063 | 376.38 ms | 390.98 ms | 14.60 ms |
86f0096 | 368.63 ms | 446.92 ms | 78.29 ms |
0bd723b | 412.52 ms | 496.65 ms | 84.13 ms |
d6d2b2e | 463.14 ms | 545.56 ms | 82.42 ms |
4e29063 | 384.14 ms | 447.74 ms | 63.60 ms |
4e260b3 | 378.73 ms | 454.18 ms | 75.45 ms |
c7e2fbc | 398.35 ms | 468.52 ms | 70.17 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b7fa26f | 1.70 MiB | 2.27 MiB | 583.82 KiB |
6b4dd70 | 1.70 MiB | 2.27 MiB | 583.82 KiB |
3d8bd2b | 1.72 MiB | 2.29 MiB | 577.53 KiB |
4e29063 | 1.72 MiB | 2.29 MiB | 578.38 KiB |
86f0096 | 1.72 MiB | 2.29 MiB | 576.50 KiB |
0bd723b | 1.72 MiB | 2.29 MiB | 578.09 KiB |
d6d2b2e | 1.72 MiB | 2.27 MiB | 555.05 KiB |
4e29063 | 1.72 MiB | 2.29 MiB | 578.38 KiB |
4e260b3 | 1.72 MiB | 2.27 MiB | 554.95 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
Previous results on branch: feat/metrics-span-aggregations
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
350fd13 | 411.08 ms | 511.49 ms | 100.41 ms |
450fa50 | 399.91 ms | 474.27 ms | 74.35 ms |
a5701de | 385.06 ms | 457.86 ms | 72.79 ms |
f88769c | 381.13 ms | 464.45 ms | 83.32 ms |
bac9455 | 398.20 ms | 538.94 ms | 140.74 ms |
7ebd815 | 376.46 ms | 456.26 ms | 79.80 ms |
b26392f | 356.48 ms | 429.53 ms | 73.05 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
350fd13 | 1.70 MiB | 2.28 MiB | 591.92 KiB |
450fa50 | 1.70 MiB | 2.28 MiB | 591.92 KiB |
a5701de | 1.70 MiB | 2.28 MiB | 591.93 KiB |
f88769c | 1.70 MiB | 2.28 MiB | 591.91 KiB |
bac9455 | 1.70 MiB | 2.28 MiB | 591.91 KiB |
7ebd815 | 1.70 MiB | 2.28 MiB | 591.91 KiB |
b26392f | 1.70 MiB | 2.28 MiB | 591.97 KiB |
…/sentry-java into feat/metrics-span-aggregations
sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt
Outdated
Show resolved
Hide resolved
…estMetadataReaderTest.kt Co-authored-by: Stefano <stefano.siano@sentry.io>
...ry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MetricsActivity.kt
Outdated
Show resolved
Hide resolved
@@ -300,6 +321,25 @@ public static final class Deserializer implements JsonDeserializer<SentrySpan> { | |||
case JsonKeys.MEASUREMENTS: | |||
measurements = reader.nextMapOrNull(logger, new MeasurementValue.Deserializer()); | |||
break; | |||
case JsonKeys.METRICS_SUMMARY: | |||
if (reader.peek() == JsonToken.NULL) { |
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.
Can we extract this logic into a new method in JsonObjectReader
like nextMapOfListOrNull
?
So we can reuse it in other places, like SentryTransaction
val span = fixture.getSut() | ||
assertNotNull(span.localMetricsAggregator) | ||
|
||
assertSame(span.localMetricsAggregator, span.localMetricsAggregator) |
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.
is it a mistake?
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.
Nope this is intended, it just ensures the getter returns a reference to the same LocalMetricsAggregator
.
Let me add a comment for this 👍
val aggregator = mock<IMetricsAggregator>() | ||
val localMetricsAggregator = mock<LocalMetricsAggregator>() | ||
|
||
val api = MetricsApi(object : IMetricsInterface { |
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 would extract this into a Fixture
class, and the verify
s into some private methods
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.
Just left few minor comments, but good!
📜 Description
Aggregate metrics based on spans: https://develop.sentry.dev/sdk/metrics/#span-aggregation.
#skip-changelog
-- as the feature hasn't been released yet.
💡 Motivation and Context
Example event: https://sentry-sdks.sentry.io/performance/sentry-android%3A70c95de097d64ca68ebdbbe6f923d625/?openPanel=open&referrer=metrics#span-a39379e81c714978
In order to improve the link between metrics and spans (see RFC) we want to aggregate metric emissions when a span is active.
The span summary is grouped by
export_key
(an unique identifier, ignoring the metric tags) and then bymetric_key
(an unique identifier, including metric tags), the RFC provides a good example to illustrate why this is practical.💚 How did you test it?
Added unit tests, as well as an integration test
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps