-
-
Notifications
You must be signed in to change notification settings - Fork 279
feat: trace connected metrics #3450
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
base: main
Are you sure you want to change the base?
Conversation
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Remove addSpan method from TelemetryProcessor interface - Remove span buffer from DefaultTelemetryProcessor - Remove captureSpan method from SentryClient - Remove traceLifecycle property from SentryOptions - Remove span imports and exports - Update mocks to remove span-related code Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
Android Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| e5ae2a6 | 441.98 ms | 465.47 ms | 23.49 ms |
| cc4e375 | 426.15 ms | 482.34 ms | 56.19 ms |
| 793f4dc | 462.68 ms | 544.21 ms | 81.53 ms |
| c002f00 | 352.65 ms | 332.69 ms | -19.95 ms |
| 6287dc4 | 390.30 ms | 399.12 ms | 8.82 ms |
| 2cb90b9 | 479.38 ms | 552.69 ms | 73.31 ms |
| e45c0e1 | 447.29 ms | 558.33 ms | 111.04 ms |
| cdf371b | 367.64 ms | 377.02 ms | 9.38 ms |
| 8775665 | 415.88 ms | 439.26 ms | 23.38 ms |
| 192b44c | 472.26 ms | 477.34 ms | 5.08 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| e5ae2a6 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| cc4e375 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
| 793f4dc | 6.54 MiB | 7.69 MiB | 1.15 MiB |
| c002f00 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| 6287dc4 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 2cb90b9 | 6.54 MiB | 7.70 MiB | 1.17 MiB |
| e45c0e1 | 6.54 MiB | 7.70 MiB | 1.16 MiB |
| cdf371b | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 8775665 | 13.93 MiB | 15.18 MiB | 1.25 MiB |
| 192b44c | 13.93 MiB | 14.93 MiB | 1.00 MiB |
Previous results on branch: feat/metrics
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| e3e55a5 | 377.14 ms | 364.56 ms | -12.58 ms |
| ffd9fc7 | 398.98 ms | 399.96 ms | 0.98 ms |
| 2a553a2 | 375.06 ms | 373.61 ms | -1.45 ms |
| 586ae3d | 364.23 ms | 355.31 ms | -8.92 ms |
| b6bff0d | 445.71 ms | 450.43 ms | 4.72 ms |
| eb2cad9 | 407.67 ms | 415.29 ms | 7.62 ms |
| 435917d | 426.85 ms | 423.47 ms | -3.38 ms |
| 3282f7a | 406.78 ms | 408.26 ms | 1.48 ms |
| 875ca84 | 402.21 ms | 395.55 ms | -6.66 ms |
| cd836b5 | 427.61 ms | 428.29 ms | 0.68 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| e3e55a5 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| ffd9fc7 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| 2a553a2 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| 586ae3d | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| b6bff0d | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| eb2cad9 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| 435917d | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| 3282f7a | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| 875ca84 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
| cd836b5 | 14.09 MiB | 15.28 MiB | 1.19 MiB |
iOS Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d789735 | 1240.58 ms | 1246.41 ms | 5.82 ms |
| 8775665 | 1234.17 ms | 1230.04 ms | -4.13 ms |
| e45c0e1 | 1269.08 ms | 1278.83 ms | 9.75 ms |
| 944b773 | 1252.82 ms | 1254.08 ms | 1.27 ms |
| f579250 | 1248.14 ms | 1250.15 ms | 2.01 ms |
| 2f63d89 | 1251.67 ms | 1263.94 ms | 12.27 ms |
| a5b28db | 1260.16 ms | 1260.08 ms | -0.08 ms |
| 7cfbbd6 | 1270.63 ms | 1285.36 ms | 14.72 ms |
| 0265ce5 | 1261.66 ms | 1250.42 ms | -11.24 ms |
| 6e7d494 | 1261.37 ms | 1265.35 ms | 3.99 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d789735 | 5.53 MiB | 5.96 MiB | 443.28 KiB |
| 8775665 | 5.53 MiB | 6.02 MiB | 502.13 KiB |
| e45c0e1 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
| 944b773 | 5.53 MiB | 6.00 MiB | 479.98 KiB |
| f579250 | 5.66 MiB | 6.09 MiB | 448.36 KiB |
| 2f63d89 | 5.65 MiB | 6.09 MiB | 446.25 KiB |
| a5b28db | 5.53 MiB | 6.02 MiB | 501.31 KiB |
| 7cfbbd6 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
| 0265ce5 | 5.66 MiB | 6.09 MiB | 448.36 KiB |
| 6e7d494 | 5.53 MiB | 6.01 MiB | 488.14 KiB |
Previous results on branch: feat/metrics
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2a553a2 | 1268.10 ms | 1258.41 ms | -9.69 ms |
| 875ca84 | 1246.04 ms | 1252.00 ms | 5.96 ms |
| cd836b5 | 1255.17 ms | 1248.47 ms | -6.70 ms |
| b6bff0d | 1258.00 ms | 1257.47 ms | -0.53 ms |
| d530afc | 1249.87 ms | 1245.18 ms | -4.69 ms |
| 1081ca3 | 1238.83 ms | 1246.23 ms | 7.40 ms |
| e3e55a5 | 1273.33 ms | 1276.81 ms | 3.48 ms |
| 586ae3d | 1255.46 ms | 1259.27 ms | 3.81 ms |
| 435917d | 1235.96 ms | 1238.35 ms | 2.40 ms |
| 3282f7a | 1264.02 ms | 1257.23 ms | -6.79 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2a553a2 | 5.66 MiB | 6.10 MiB | 451.21 KiB |
| 875ca84 | 5.66 MiB | 6.09 MiB | 449.93 KiB |
| cd836b5 | 5.66 MiB | 6.10 MiB | 451.07 KiB |
| b6bff0d | 5.66 MiB | 6.10 MiB | 450.59 KiB |
| d530afc | 5.66 MiB | 6.10 MiB | 451.07 KiB |
| 1081ca3 | 5.65 MiB | 6.10 MiB | 451.30 KiB |
| e3e55a5 | 5.66 MiB | 6.10 MiB | 451.25 KiB |
| 586ae3d | 5.65 MiB | 6.09 MiB | 450.66 KiB |
| 435917d | 5.66 MiB | 6.09 MiB | 449.93 KiB |
| 3282f7a | 5.66 MiB | 6.10 MiB | 451.08 KiB |
…onality - Added imports for telemetry metrics in `sentry_options.dart` and `sentry.dart`. - Updated `SentryMetric` class to use string types for metric types instead of an enum. - Adjusted metric constructors to reflect the new string-based type system. - Modified tests to accommodate changes in metric handling.
- Introduced DefaultSentryMetrics and NoOpSentryMetrics classes for metric handling. - Updated MetricsSetupIntegration to utilize DefaultSentryMetrics. - Refactored SentryMetrics interface to abstract metric methods. - Added metrics implementation to the Sentry export in sentry.dart.
…t attributes - Introduced MetricCapturePipeline for capturing and processing metrics. - Added default attributes for telemetry metrics in the new default_attributes.dart file. - Implemented DefaultSentryMetrics for metric handling and integrated it into SentryClient. - Updated SentryClient to utilize the MetricCapturePipeline for capturing metrics. - Added tests to ensure proper functionality of the new metric capturing and processing features.
…nd MetricsSetupIntegration - Added internal logging for metric capture events in MetricCapturePipeline to provide better visibility on dropped and captured metrics. - Updated MetricsSetupIntegration to log when metrics are disabled or when custom metrics are already configured. - Introduced tests for capturing metrics in the Hub, ensuring proper functionality and scope handling. - Added tests for creating SentryEnvelope and SentryEnvelopeItem from metrics data, verifying correct headers and payloads.
- Added new constants for replay ID and buffering state in SemanticAttributesConstants. - Refactored SentryLogger to utilize a new extension for formatting Sentry attributes. - Introduced utility extensions for formatting Sentry attributes and maps of attributes. - Updated MetricCapturePipeline to log metrics with formatted attributes. - Replaced ReplayLogIntegration with ReplayTelemetryIntegration in Flutter integration tests and codebase. - Added tests to ensure proper functionality of the new replay integration and attribute formatting.
- Added a unit field to the DefaultSentryMetrics for better metric representation. - Updated the metrics test to verify that the unit is included when provided.
- Changed the MetricCapturePipeline to log the processed metric instead of the original metric. - Removed unused import from metrics_test.dart to clean up the codebase.
| @internal | ||
| extension AddAllAbsentX<K, V> on Map<K, V> { | ||
| void addAllIfAbsent(Map<K, V> other) { | ||
| for (final e in other.entries) { | ||
| putIfAbsent(e.key, () => e.value); | ||
| } | ||
| } | ||
| } |
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.
from span-first telemetry enrichment
| internalLogger.debug(() => | ||
| 'Sentry.metrics.count("$name", $value) called with attributes ${_formatAttributes(attributes)}'); |
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'm using lazy evaluation (() => for these since these formattings can be potentially heavy when the user has many attributes
|
|
||
| final _operatingSystem = getSentryOperatingSystem(); | ||
|
|
||
| Map<String, SentryAttribute> defaultAttributes(SentryOptions options, |
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.
from span-first branch, this is used by all telemetry pipelines
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 was already wondering why i'm seeing this added so often. :D
| if (scope != null) { | ||
| metric.attributes.addAllIfAbsent(scope.attributes); | ||
| } |
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.
merge scope attributes before proceeding with the rest
| num value; | ||
| SentryId traceId; | ||
| SpanId? spanId; | ||
| String? unit; |
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.
according to spec we must not constraint this with an enum and instead the user needs to know which unit types we support
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.
Then ignore/resolve my previous comment. 🙇♂️
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.
Looks great overall, some comments regarding naming/api/callbacks timing. 🥳
| return 'feedback'; | ||
| case DataCategory.metricBucket: | ||
| return 'metric_bucket'; | ||
| case DataCategory.metric: |
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.
All other types are called like the raw value, so in this case it should be 'traceMetric'
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.
We should keep it as metrics as per spec:
The envelope item type is named trace_metric for internal usage to avoid naming collisions with other metric systems within Sentry's infrastructure. From an SDK perspective, these are simply referred to as "metrics".
| num value; | ||
| SentryId traceId; | ||
| SpanId? spanId; | ||
| String? unit; |
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.
Then ignore/resolve my previous comment. 🙇♂️
| /// Increments a counter metric by the given [value]. | ||
| /// | ||
| /// Use counters to track the number of times an event occurs. | ||
| void count(String name, int value, |
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.
Does it make sense to also expose the Future, or should this API be strictly fire & forget?
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.
imo it should just be a fire and forget (many other SDKs do it like that as well)
Future adds a lot of noise since it could get flagged for some people as a warning with the analyzer
|
|
||
| if (options.enableMetrics) { | ||
| options.lifecycleRegistry | ||
| .registerCallback<OnProcessMetric>((event) async { |
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.
For logs we are registering for OnBefore capture, why do we do it for onProcess for metrics? Shouldn't both add attributes at the same time in their journey through the SDK?
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.
the OnBeforeCapture usage is actually not correct for logs since it runs after beforeSend
in the next PR I would also replace that with an OnProcessLog
OnProcess would run before the default telemetry attributes enrichment so more specific enrichments can be added first and we dont need to do this removal of integrations anymore
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 didn't wanna update that for logs right now so I don't explode this PR
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.
Opened another PR for log specific changes
|
|
||
| SentryFlutterOptions? _options; | ||
| SdkLifecycleCallback<OnBeforeCaptureLog>? _onBeforeCaptureLog; | ||
| SdkLifecycleCallback<OnProcessMetric>? _onProcessMetric; |
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.
Same here, why does log use OnBeforeCapture (waiting after default auttributes were added), on metrics use OnProcess?
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.
answered above
…proved initialization
…t instance of NoOpSentryMetrics
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| traceId: _traceIdFor(scope), | ||
| attributes: attributes ?? {}); | ||
|
|
||
| _captureMetricCallback(metric); |
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.
Undocumented unawaited futures in metrics capture
Low Severity · Bugbot Rules
Flagged per user review rules about unawaited futures. CaptureMetricCallback is typed as void Function(SentryMetric), but hub.captureMetric returns Future<void>. When _captureMetricCallback(metric) is invoked in count(), gauge(), and distribution(), the resulting Future is created but never awaited. This pattern is not documented as intentional. The user's Dart-specific rules state: "Avoid unawaited futures unless intentional and documented."
📜 Description
Adds trace connected metrics feature to Dart
💡 Motivation and Context
Closes #3341
💚 How did you test it?
Manual and unit tests
📝 Checklist
Protocol ✅
trace_metricSentryItemType.metric = 'trace_metric'application/vnd.sentry.items.trace-metric+json{"items": [...]}item_countMetric Payload ✅
timestamp(seconds)millisecondsSinceEpoch / 1000.0trace_id(32-char hex)SentryId.toString()span_id(16-char hex, optional)SpanId.toString()name,value,typeunit(optional)attributes(typed, optional)Public API ✅
count(name, value, {attributes, scope})gauge(name, value, {unit, attributes, scope})distribution(name, value, {unit, attributes, scope})Configuration ✅
enableMetricsdefaulttruebeforeSendMetricDefault Attributes ✅
sentry.environment,sentry.release,sentry.sdk.name,sentry.sdk.version✅sentry.replay_id,sentry._internal.replay_is_buffering✅ (via lifecycle hooks)user.id,user.name,user.email) guarded bysendDefaultPii✅SDK Behavior ✅
Data Category & Rate Limiting ✅
trace_metric→DataCategory.metric✅Buffering & Debug Logging ✅
InMemoryTelemetryBuffer✅internalLogger.debug()✅🔮 Next steps