diff --git a/CHANGELOG.md b/CHANGELOG.md index 09bd4820..f85fd2a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Added [`DynamicSamplingContext::with_maybe_sample_rate`](https://docs.rs/sentry-types/0.49.2/sentry_types/protocol/v7/struct.DynamicSamplingContext.html#method.with_maybe_sample_rate) and [`DynamicSamplingContext::with_maybe_sampled`](https://docs.rs/sentry-types/0.49.2/sentry_types/protocol/v7/struct.DynamicSamplingContext.html#method.with_maybe_sampled), which set or clear the corresponding fields from `Option` values. +### Improvements + +- The SDK no longer reports transactions and spans discarded while tracing is disabled as sampling losses. Tracing is disabled when no tracing sample rate or sampling function is configured ([#1290](https://github.com/getsentry/sentry-rust/pull/1290)). + ## 0.49.1 ### Fixes diff --git a/sentry-core/src/performance/headers.rs b/sentry-core/src/performance/headers.rs index a3742135..703b3051 100644 --- a/sentry-core/src/performance/headers.rs +++ b/sentry-core/src/performance/headers.rs @@ -132,6 +132,11 @@ impl TracePropagationContext { }) } + /// Set the `sampled` field, accepting `Option` values. + pub(crate) fn with_maybe_sampled(self, sampled: Option) -> Self { + Self { sampled, ..self } + } + /// Attempts to construct a [`TracePropagationContext`] from the given Sentry trace header. /// /// Returns [`None`] if the header cannot be parsed. diff --git a/sentry-core/src/performance/mod.rs b/sentry-core/src/performance/mod.rs index 390c6081..5304148d 100644 --- a/sentry-core/src/performance/mod.rs +++ b/sentry-core/src/performance/mod.rs @@ -275,16 +275,12 @@ impl TransactionContext { let (trace_id, parent_span_id, sampled) = match span { TransactionOrSpan::Transaction(transaction) => { let inner = transaction.inner.lock().unwrap(); - ( - inner.context.trace_id, - inner.context.span_id, - Some(inner.sampled), - ) + (inner.context.trace_id, inner.context.span_id, inner.sampled) } TransactionOrSpan::Span(span) => { let sampled = span.sampled; let span = span.span.lock().unwrap(); - (span.trace_id, span.span_id, Some(sampled)) + (span.trace_id, span.span_id, sampled) } }; @@ -676,7 +672,10 @@ impl TransactionOrSpan { pub(crate) struct TransactionInner { #[cfg(feature = "client")] client: Option>, - sampled: bool, + /// Whether the transaction is sampled. + /// + /// A value of `None` indicates that tracing is disabled. + sampled: Option, pub(crate) context: protocol::TraceContext, pub(crate) transaction: Option>, } @@ -685,16 +684,16 @@ type TransactionArc = Arc>; /// Functional implementation of how a new transaction's sample rate is chosen. /// -/// Split out from `Client.is_transaction_sampled` for testing. +/// Returns `None` when tracing is disabled. #[cfg(feature = "client")] fn transaction_sample_rate( traces_sampling_strategy: &TracesSamplingStrategy, ctx: &TransactionContext, -) -> f32 { +) -> Option { match traces_sampling_strategy { - &TracesSamplingStrategy::FixedRate(rate) => ctx.sampled.map_or(rate, f32::from), - TracesSamplingStrategy::Function(traces_sampler) => traces_sampler(ctx), - TracesSamplingStrategy::Disabled => 0.0, + &TracesSamplingStrategy::FixedRate(rate) => Some(ctx.sampled.map_or(rate, f32::from)), + TracesSamplingStrategy::Function(traces_sampler) => Some(traces_sampler(ctx)), + TracesSamplingStrategy::Disabled => None, } } @@ -714,11 +713,14 @@ fn should_continue_trace( /// Determine whether the new transaction should be sampled. #[cfg(feature = "client")] impl Client { - fn determine_sampling_decision(&self, ctx: &TransactionContext) -> (bool, f32) { + /// Determines the sampling decision and sample rate at which the decision was made. + /// + /// Returns `None` when tracing is disabled. + fn determine_sampling_decision(&self, ctx: &TransactionContext) -> Option<(bool, f32)> { let client_options = self.options(); - let sample_rate = transaction_sample_rate(&client_options.traces_sampling_strategy, ctx); + let sample_rate = transaction_sample_rate(&client_options.traces_sampling_strategy, ctx)?; let sampled = self.sample_should_send(sample_rate); - (sampled, sample_rate) + Some((sampled, sample_rate)) } } @@ -727,7 +729,9 @@ impl Client { #[derive(Clone, Debug)] struct TransactionMetadata { /// The sample rate used when making the sampling decision for the associated transaction. - sample_rate: f32, + /// + /// `None` when tracing is disabled. + sample_rate: Option, } /// A running Performance Monitoring Transaction. @@ -778,7 +782,7 @@ impl<'a> TransactionData<'a> { impl Transaction { #[cfg(feature = "client")] fn new(client: Option>, mut ctx: TransactionContext) -> Self { - let ((sampled, sample_rate), transaction) = match client.as_ref() { + let (sampling_info, transaction) = match client.as_ref() { Some(client) => { let options = client.options(); let sdk_org_id = options.org_id.or_else(|| options.dsn.as_ref()?.org_id()); @@ -805,15 +809,13 @@ impl Transaction { }), ) } - None => ( - ( - ctx.sampled.unwrap_or(false), - ctx.sampled.map_or(0.0, f32::from), - ), - None, - ), + None => (ctx.sampled.map(|sampled| (sampled, sampled.into())), None), }; + let (sampled, sample_rate) = sampling_info + .map(|(sampled, sample_rate)| (Some(sampled), Some(sample_rate))) + .unwrap_or_default(); + let context = protocol::TraceContext { trace_id: ctx.trace_id, parent_span_id: ctx.parent_span_id, @@ -841,7 +843,7 @@ impl Transaction { op: Some(ctx.op), ..Default::default() }; - let sampled = ctx.sampled.unwrap_or(false); + let sampled = ctx.sampled; Self { inner: Arc::new(Mutex::new(TransactionInner { @@ -943,7 +945,7 @@ impl Transaction { pub fn iter_headers(&self) -> TraceHeadersIter { let inner = self.inner.lock().unwrap(); let trace = TracePropagationContext::new(inner.context.trace_id, inner.context.span_id) - .with_sampled(inner.sampled); + .with_maybe_sampled(inner.sampled); TraceHeadersIter { sentry_trace: Some(trace.sentry_trace_header()), } @@ -963,7 +965,7 @@ impl Transaction { /// correct results. #[deprecated = "the returned value may not accurately represent the sampling decision"] pub fn is_sampled(&self) -> bool { - self.inner.lock().unwrap().sampled + self.inner.lock().unwrap().sampled.unwrap_or_default() } /// Finishes the Transaction with the provided end timestamp. @@ -974,24 +976,18 @@ impl Transaction { with_client_impl! {{ let mut inner = self.inner.lock().unwrap(); - // Discard `Transaction` unless sampled. - if !inner.sampled { - if let Some(transaction) = inner.transaction.take() { - if let Some(client) = inner.client.as_ref() { - client.record_lost_data(&transaction, ClientReportReason::SampleRate); - } - } - return; - } - - if let Some(mut transaction) = inner.transaction.take() { - if let Some(client) = inner.client.take() { + if let (Some(sampled), Some(mut transaction), Some(client)) = + (inner.sampled, inner.transaction.take(), inner.client.take()) + { + if sampled { transaction.finish_with_timestamp(_timestamp); transaction .contexts .insert("trace".into(), inner.context.clone().into()); - Hub::current().with_current_scope(|scope| scope.apply_to_transaction(&mut transaction)); + Hub::current() + .with_current_scope(|scope| scope.apply_to_transaction(&mut transaction)); + let opts = client.options(); transaction.release.clone_from(&opts.release); transaction.environment.clone_from(&opts.environment); @@ -1000,20 +996,23 @@ impl Transaction { let mut dsc = protocol::DynamicSamplingContext::new() .with_trace_id(inner.context.trace_id) - .with_sample_rate(self.metadata.sample_rate) - .with_sampled(inner.sampled); + .with_maybe_sample_rate(self.metadata.sample_rate) + .with_maybe_sampled(inner.sampled); if let Some(public_key) = client.dsn().map(|dsn| dsn.public_key()) { dsc = dsc.with_public_key(public_key.to_owned()); } drop(inner); - let mut envelope = protocol::Envelope::new().with_headers( - protocol::EnvelopeHeaders::new().with_trace(dsc) - ); + let mut envelope = protocol::Envelope::new() + .with_headers(protocol::EnvelopeHeaders::new().with_trace(dsc)); envelope.add_item(transaction); client.send_envelope(envelope) + } else { + // Client reports should only be recorded when tracing is enabled. They should + // not be recorded when inner.sampled is None. + client.record_lost_data(&transaction, ClientReportReason::SampleRate); } } }} @@ -1126,7 +1125,7 @@ impl DerefMut for Data<'_> { #[derive(Clone, Debug)] pub struct Span { pub(crate) transaction: TransactionArc, - sampled: bool, + sampled: Option, span: SpanArc, } @@ -1238,8 +1237,8 @@ impl Span { /// trace's distributed tracing headers. pub fn iter_headers(&self) -> TraceHeadersIter { let span = self.span.lock().unwrap(); - let trace = - TracePropagationContext::new(span.trace_id, span.span_id).with_sampled(self.sampled); + let trace = TracePropagationContext::new(span.trace_id, span.span_id) + .with_maybe_sampled(self.sampled); TraceHeadersIter { sentry_trace: Some(trace.sentry_trace_header()), } @@ -1258,7 +1257,7 @@ impl Span { /// correct results. #[deprecated = "the returned value may not accurately represent the sampling decision"] pub fn is_sampled(&self) -> bool { - self.sampled + self.sampled.unwrap_or_default() } /// Finishes the Span with the provided end timestamp. @@ -1460,40 +1459,40 @@ mod tests { let ctx = TransactionContext::new("noop", "noop"); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::FixedRate(0.3), &ctx), - 0.3 + Some(0.3) ); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::FixedRate(0.7), &ctx), - 0.7 + Some(0.7) ); let mut ctx = TransactionContext::new("noop", "noop"); ctx.set_sampled(true); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::FixedRate(0.3), &ctx), - 1.0 + Some(1.0) ); ctx.set_sampled(false); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::FixedRate(0.3), &ctx), - 0.0 + Some(0.0) ); let ctx = TransactionContext::new("noop", "noop"); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::Disabled, &ctx), - 0.0 + None ); let mut ctx = TransactionContext::new("noop", "noop"); ctx.set_sampled(true); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::Disabled, &ctx), - 0.0 + None ); ctx.set_sampled(false); assert_eq!( transaction_sample_rate(&TracesSamplingStrategy::Disabled, &ctx), - 0.0 + None ); // Function and FixedRate are mutually exclusive strategy variants. A function @@ -1501,9 +1500,9 @@ mod tests { let mut ctx = TransactionContext::new("noop", "noop"); let sampler = |_: &TransactionContext| 0.7_f32; let strategy = TracesSamplingStrategy::Function(Arc::new(sampler) as Arc); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 0.7); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(0.7)); ctx.set_sampled(false); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 0.7); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(0.7)); let sampler = |ctx: &TransactionContext| match ctx.sampled() { Some(true) => 0.8_f32, @@ -1512,9 +1511,9 @@ mod tests { }; let strategy = TracesSamplingStrategy::Function(Arc::new(sampler) as Arc); ctx.set_sampled(true); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 0.8); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(0.8)); ctx.set_sampled(None); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 0.6); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(0.6)); let sampler = |ctx: &TransactionContext| { if ctx.name() == "must-name" || ctx.operation() == "must-operation" { @@ -1533,11 +1532,11 @@ mod tests { }; let strategy = TracesSamplingStrategy::Function(Arc::new(sampler) as Arc); let ctx = TransactionContext::new("noop", "must-operation"); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 1.0); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(1.0)); let ctx = TransactionContext::new("must-name", "noop"); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 1.0); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(1.0)); let mut ctx = TransactionContext::new("noop", "noop"); ctx.custom_insert("rate".to_owned(), serde_json::json!(0.7)); - assert_eq!(transaction_sample_rate(&strategy, &ctx), 0.7); + assert_eq!(transaction_sample_rate(&strategy, &ctx), Some(0.7)); } } diff --git a/sentry-core/tests/client_reports.rs b/sentry-core/tests/client_reports.rs index 4c2aa8dd..dcb5eecb 100644 --- a/sentry-core/tests/client_reports.rs +++ b/sentry-core/tests/client_reports.rs @@ -141,6 +141,29 @@ fn client_report_records_unsampled_transaction_and_spans() { ); } +#[test] +fn client_report_does_not_record_disabled_tracing_as_sample_rate_drop() { + let transport = TestTransport::new(); + let client = Arc::new(client_with_options(transport.clone(), ClientOptions::new())); + + Hub::run( + Arc::new(Hub::new(Some(client.clone()), Arc::new(Default::default()))), + || { + let transaction = sentry_core::start_transaction(TransactionContext::new("tx", "op")); + transaction.start_child("child", "one").finish(); + transaction.start_child("child", "two").finish(); + transaction.finish(); + }, + ); + client.send_envelope(Envelope::new()); + + let envelopes = transport.fetch_and_clear_envelopes(); + assert_eq!(envelopes.len(), 1); + assert!(!envelopes[0] + .items() + .any(|item| matches!(item, EnvelopeItem::ClientReport(_)))); +} + #[test] fn client_report_records_transaction_span_cap_drop() { // Keep in sync with `MAX_SPANS` in `sentry-core/src/performance.rs`.