Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions sentry-core/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,16 +524,25 @@ impl Client {

/// Captures a metric and sends it to Sentry.
#[cfg(feature = "metrics")]
pub fn capture_metric(&self, metric: Metric, _: &Scope) {
if let Some(batcher) = self
.metrics_batcher
.read()
.expect("metrics batcher lock could not be acquired")
.as_ref()
{
batcher.enqueue(metric);
pub fn capture_metric(&self, metric: Metric, scope: &Scope) {
if let Some(metric) = self.prepare_metric(metric, scope) {
if let Some(batcher) = self
.metrics_batcher
.read()
.expect("metrics batcher lock could not be acquired")
.as_ref()
{
batcher.enqueue(metric);
}
}
}

/// 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> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

scope.apply_to_metric(&mut metric);
Some(metric)
}
}

// Make this unwind safe. It's not out of the box because of the
Expand Down
9 changes: 9 additions & 0 deletions sentry-core/src/performance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,15 @@ impl TransactionOrSpan {
TransactionOrSpan::Span(span) => span.finish(),
}
}

/// Get the span ID of this [`TransactionOrSpan`].
#[cfg(all(feature = "metrics", feature = "client"))]
pub(crate) fn span_id(&self) -> SpanId {
match self {
TransactionOrSpan::Transaction(transaction) => transaction.get_trace_context().span_id,
TransactionOrSpan::Span(span) => span.get_span_id(),
}
}
}

#[derive(Debug)]
Expand Down
13 changes: 13 additions & 0 deletions sentry-core/src/scope/real.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use std::sync::Mutex;
use std::sync::{Arc, PoisonError, RwLock};

use crate::performance::TransactionOrSpan;
#[cfg(feature = "metrics")]
use crate::protocol::Metric;
use crate::protocol::{
Attachment, Breadcrumb, Context, Event, Level, TraceContext, Transaction, User, Value,
};
Expand Down Expand Up @@ -399,6 +401,17 @@ impl Scope {
}
}

/// 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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9f8e156. Configure here.

metric.trace_id = self
.get_span()
.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());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9f8e156. Configure here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3d71e21. Configure here.


/// Set the given [`TransactionOrSpan`] as the active span for this scope.
pub fn set_span(&mut self, span: Option<TransactionOrSpan>) {
self.span = Arc::new(span);
Expand Down
81 changes: 80 additions & 1 deletion sentry-core/tests/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use anyhow::{Context, Result};
use sentry::protocol::MetricType;
use sentry_core::protocol::{EnvelopeItem, ItemContainer};
use sentry_core::test;
use sentry_core::{ClientOptions, Hub};
use sentry_core::{ClientOptions, Hub, TransactionContext};
use sentry_types::protocol::v7::Metric;

/// Test that metrics are sent when metrics are enabled.
Expand Down Expand Up @@ -204,6 +204,85 @@ fn test_metrics_batching_over_limit() {
)
}

/// Test that metrics in the same scope share the same trace_id when no span is active.
///
/// This tests that trace ID is set from the propagation context when there is no active span.
#[test]
fn metrics_share_trace_id_without_active_span() {
let options = ClientOptions {
enable_metrics: true,
..Default::default()
};

let envelopes = test::with_captured_envelopes_options(
|| {
capture_test_metric("test-1");
capture_test_metric("test-2");
},
options,
);
let envelope = envelopes
.try_into_only_item()
.expect("expected one envelope");
let item = envelope
.into_items()
.try_into_only_item()
.expect("expected one item");
let metrics = item.into_metrics().expect("expected metrics item");

let [metric1, metric2] = metrics.as_slice() else {
panic!("expected exactly two metrics");
};

assert_eq!(
metric1.trace_id, metric2.trace_id,
"metrics in the same scope should share the same trace_id"
);

assert!(metric1.span_id.is_none());
assert!(metric2.span_id.is_none());
}

/// Test that span_id is set from the active span when one is present.
#[test]
fn metrics_span_id_from_active_span() {
let options = ClientOptions {
enable_metrics: true,
..Default::default()
};

let mut expected_span_id = None;
let envelopes = test::with_captured_envelopes_options(
|| {
let transaction_ctx = TransactionContext::new("test transaction", "test");
expected_span_id = Some(transaction_ctx.span_id());
let transaction = sentry_core::start_transaction(transaction_ctx);
sentry_core::configure_scope(|scope| scope.set_span(Some(transaction.clone().into())));
capture_test_metric("test");
transaction.finish();
},
options,
);

let expected_span_id = expected_span_id.expect("expected_span_id did not get set");

let envelope = envelopes
.try_into_only_item()
.expect("expected one envelope");
let item = envelope
.into_items()
.try_into_only_item()
.expect("expected one item");
let mut metrics = item.into_metrics().expect("expected metrics item");
let metric = metrics.pop().expect("expected one metric");

assert_eq!(
metric.span_id,
Some(expected_span_id),
"span_id should be set from the active span"
);
}

/// Returns a [`Metric`] with [type `Counter`](MetricType),
/// the provided name, and a value of `1.0`.
fn test_metric<S>(name: S) -> Metric
Expand Down
Loading