feat(metrics): Associate trace data with metrics#1061
feat(metrics): Associate trace data with metrics#1061szokeasaurusrex wants to merge 1 commit intoszokeasaurusrex/core-metrics-capturefrom
Conversation
|
d425a32 to
8e8205e
Compare
8077154 to
e2abb12
Compare
| .map(|span| span.get_trace_context().trace_id) | ||
| .unwrap_or(self.propagation_context.trace_id); | ||
|
|
||
| metric.span_id = self.get_span().map(|span| span.span_id()); |
There was a problem hiding this comment.
We should fall back on the propagation context to get the span ID, as it seems the Python implementation does it that way: https://github.com/getsentry/sentry-python/blob/5d26c2f60e289c89ab260ee25607a3bea5e174c9/sentry_sdk/scope.py#L1828.
I saw that logs do the same and only consider self.span, so we should probably fix that too.
|
|
||
| /// Prepares a metric to be sent, setting trace association data from the scope. | ||
| #[cfg(feature = "metrics")] | ||
| fn prepare_metric(&self, mut metric: Metric, scope: &Scope) -> Option<Metric> { |
There was a problem hiding this comment.
This could also return just Metric. If we need to have before_send_metric (not sure if that exists) then I guess Option<Metric> makes sense though. It's an internal API anyway so it doesn't really matter.
lcian
left a comment
There was a problem hiding this comment.
LGTM other than the comment about span_id.
e2abb12 to
9f8e156
Compare
| .unwrap_or(self.propagation_context.trace_id); | ||
|
|
||
| metric.span_id = self.get_span().map(|span| span.span_id()); | ||
| } |
There was a problem hiding this comment.
Redundant double get_span() call in apply_to_metric
Low Severity
apply_to_metric calls self.get_span() twice, each time cloning the Option<TransactionOrSpan>. For the Transaction variant, this also results in get_trace_context() being called twice (once explicitly for trace_id, once internally via span_id()), each acquiring a mutex lock and cloning the full TraceContext. A single get_span() call, stored in a local variable, would avoid the redundant cloning and lock acquisitions. The sibling method apply_to_log already follows this pattern by checking self.span.as_ref() once.
Reviewed by Cursor Bugbot for commit 9f8e156. Configure here.
|
|
||
| /// Applies the contained scoped data to a trace metric, setting the `trace_id` and `span_id`. | ||
| #[cfg(feature = "metrics")] | ||
| pub(crate) fn apply_to_metric(&self, metric: &mut Metric) { |
There was a problem hiding this comment.
Inconsistent visibility between noop and real apply_to_metric
Low Severity
apply_to_metric is pub(crate) in real.rs but pub in noop.rs. Every other scope method (apply_to_log, apply_to_event, set_span, get_span, etc.) uses pub consistently in both implementations. This breaks the established pattern and means the method's external accessibility changes depending on whether the client feature flag is enabled.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9f8e156. Configure here.
Set trace and span id on metrics. Co-authored-by: Joris Bayer <joris.bayer@sentry.io> Closes #1058 Closes [RUST-186](https://linear.app/getsentry/issue/RUST-186/add-trace-metric-tracing-association-in-sentry-core)
9f8e156 to
3d71e21
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3d71e21. Configure here.
| .unwrap_or(self.propagation_context.trace_id); | ||
|
|
||
| metric.span_id = self.get_span().map(|span| span.span_id()); | ||
| } |
There was a problem hiding this comment.
Metric span_id doesn't fall back to propagation context
Medium Severity
In apply_to_metric, trace_id falls back to self.propagation_context.trace_id when no active span exists, but span_id does not similarly fall back to self.propagation_context.span_id — it just becomes None. This is inconsistent within the method itself (trace_id has a fallback but span_id doesn't), inconsistent with apply_to_event (which sets both trace_id and span_id from the propagation context when no span is active), and inconsistent with the Python SDK reference implementation. The test assertions for is_none() appear to encode the wrong expected behavior.
Reviewed by Cursor Bugbot for commit 3d71e21. Configure here.


Set trace and span id on metrics.
Stacked on #1026
Co-authored-by: Joris Bayer joris.bayer@sentry.io
Closes #1058
Closes RUST-186