Conversation
Implement trace metrics per the Sentry SDK telemetry spec, enabling SDKs to capture and transmit quantitative performance data (counters, gauges, distributions) associated with traces. The implementation mirrors the existing structured logs architecture: - TraceMetric/TraceMetricType protocol types in sentry-types - TraceMetrics envelope item container with serialization/deserialization - MetricsBatcher with background worker thread (100-item / 5s flush) - Feature-gated behind `metrics` feature flag - Public API: metrics_count(), metrics_gauge(), metrics_distribution() - before_send_metric callback and enable_metrics client option - Trace context (trace_id, span_id) and user attributes from scope Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Replace the duplicate LogsBatcher and MetricsBatcher with a single generic Batcher<T> that handles both log and metric batching using a function pointer for envelope item conversion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only the configured fraction of metrics is submitted. Sampled metrics are annotated with a `sentry.client_sample_rate` attribute so the server can extrapolate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
A sample rate of 0.0 now disables metrics entirely, removing the need for a separate enable_metrics boolean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Changes the public API from free functions with options to a builder:
sentry::metrics::count("api.requests").emit(1);
sentry::metrics::distribution("response.time")
.with_attribute("route", "my_route")
.with_unit("millisecond")
.emit(0.123);
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Metrics are now opt-in: users must explicitly set a sample rate to enable trace metrics collection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @@ -0,0 +1,337 @@ | |||
| //! Generic batching for Sentry envelope items (logs, metrics, etc.). | |||
There was a problem hiding this comment.
This was moved & generalized from logs.rs.
sentry-core/src/logs.rs
Outdated
There was a problem hiding this comment.
This has moved to batcher.rs.
sentry-core/src/clientoptions.rs
Outdated
| /// Metrics are disabled by default. Sampled metrics are annotated with a | ||
| /// `sentry.client_sample_rate` attribute so the server can extrapolate. | ||
| #[cfg(feature = "metrics")] | ||
| pub metrics_sample_rate: f32, |
There was a problem hiding this comment.
The develop docs state that this should be a boolean flag, and that it should go into _experimental: https://develop.sentry.dev/sdk/telemetry/metrics/#initialization-options
I'm willing to change this but I still think it makes sense to have a sample rate.
sentry-core/src/batcher.rs
Outdated
| } | ||
| }, | ||
| crate::ClientOptions { | ||
| metrics_sample_rate: 1.0, |
There was a problem hiding this comment.
Spec doesn't mention any metrics_sample_rate, I don't think we should have it.
There was a problem hiding this comment.
Will revert to a boolean flag. I need sampling for the relay use case (I wouldn't dare to send the entire volume at once), but I can add the sentry.client_sample_rate attribute there manually.
What about what the specs say about an _experiments section, should we move the flag there like the spec suggests?
There was a problem hiding this comment.
I guess we could also keep the sample rate, if this is going to be used by internal services we're gonna need it at some point anyways.
Another approach which I prefer for Rust is to feature flag it and make sure we state that it's clear that the flag is experimental and could be removed.
There was a problem hiding this comment.
Keeping the enable_metrics flag for now to keep it simple & spec-conformant. The spec now seems to allow putting it in the stable section.
Switch from MetricBuilder to declarative macros (metric_count!, metric_gauge!, metric_distribution!) matching the pattern used by the logging macros. Support `unit = "..."` for measurement units and quoted `"key" = value` syntax for attribute keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
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.
Per the spec, use a simple boolean flag (default: false) instead of a sample rate float. Removes client-side sampling and the sentry.client_sample_rate attribute annotation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Haven't done an in depth review but I noticed that we're missing the rate limiting data category. |
- Default enable_metrics to true (spec requirement) - Gate user PII attributes (user.id, user.name, user.email) on send_default_pii - Add trace_metric rate limiting category so server-sent rate limits for metrics are properly respected Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…client_sample_rate Adapt to the updated sentry-rust trace metrics API (getsentry/sentry-rust#997): - Rename `metrics.trace_sample_rate` config to `metrics.send_to_sentry` - Enable `metrics_enabled` on the Sentry SDK when `send_to_sentry > 0.0` - Use sentry-core macros instead of removed free functions - Set `sentry.client_sample_rate` attribute on each trace metric so Sentry can extrapolate from the sampled data Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lcian good catch! I gave the agent the spec when it came up with the initial plan, but it skipped rate limiting. I asked it to check the spec again (it also seems to have changed since then) and it made some improvements, see 343a63d. |
This PR introduces logic for emiting & batching trace metrics, largely copy-pasted from logs.
Usage example:
Resolves #938.