From 0e021ba55e96af591e1ae72571f2fe416bdc159e Mon Sep 17 00:00:00 2001 From: Tom Milligan Date: Thu, 3 Nov 2022 21:58:52 +0000 Subject: [PATCH] sentry-core: Add traces_sampler client option (#510) * sentry-core: add traces_sampler client option --- CHANGELOG.md | 1 + sentry-contexts/src/utils.rs | 2 +- sentry-core/src/clientoptions.rs | 14 +++++ sentry-core/src/hub.rs | 4 +- sentry-core/src/performance.rs | 88 ++++++++++++++++++++++++--- sentry-core/src/profiling.rs | 2 +- sentry-panic/src/lib.rs | 2 +- sentry-types/src/protocol/envelope.rs | 2 +- sentry-types/src/protocol/v7.rs | 4 +- sentry/src/transports/curl.rs | 2 +- sentry/src/transports/reqwest.rs | 2 +- sentry/src/transports/surf.rs | 2 +- sentry/src/transports/ureq.rs | 2 +- sentry/tests/test_tracing.rs | 2 +- 14 files changed, 106 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fceffa6d..e0408648 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Breaking Changes**: - The minimum supported Rust version was bumped to **1.60.0** due to requirements from dependencies. ([#498](https://github.com/getsentry/sentry-rust/pull/498)) +- Added the `traces_sampler` option to `ClientOptions`. This allows the user to customise sampling rates on a per-transaction basis. ([#510](https://github.com/getsentry/sentry-rust/pull/510)) **Features**: diff --git a/sentry-contexts/src/utils.rs b/sentry-contexts/src/utils.rs index 1340c90e..8acd9c83 100644 --- a/sentry-contexts/src/utils.rs +++ b/sentry-contexts/src/utils.rs @@ -26,7 +26,7 @@ mod model_support { return None; } - let mut buf = vec![0u8; size as usize]; + let mut buf = vec![0u8; size]; let res = libc::sysctlbyname( c_name.as_ptr() as _, buf.as_mut_ptr() as *mut c_void, diff --git a/sentry-core/src/clientoptions.rs b/sentry-core/src/clientoptions.rs index 1a62635e..8b83946b 100644 --- a/sentry-core/src/clientoptions.rs +++ b/sentry-core/src/clientoptions.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::time::Duration; use crate::constants::USER_AGENT; +use crate::performance::TracesSampler; use crate::protocol::{Breadcrumb, Event}; use crate::types::Dsn; use crate::{Integration, IntoDsn, TransportFactory}; @@ -74,6 +75,11 @@ pub struct ClientOptions { pub sample_rate: f32, /// The sample rate for tracing transactions. (0.0 - 1.0, defaults to 0.0) pub traces_sample_rate: f32, + /// If given, called with a SamplingContext for each transaction to determine the sampling rate. + /// + /// Return a sample rate between 0.0 and 1.0 for the transaction in question. + /// Takes priority over the `sample_rate`. + pub traces_sampler: Option>, /// Enables profiling pub enable_profiling: bool, /// The sample rate for profiling a transactions. (0.0 - 1.0, defaults to 0.0) @@ -196,6 +202,13 @@ impl fmt::Debug for ClientOptions { .field("environment", &self.environment) .field("sample_rate", &self.sample_rate) .field("traces_sample_rate", &self.traces_sample_rate) + .field( + "traces_sampler", + &self + .traces_sampler + .as_ref() + .map(|arc| std::ptr::addr_of!(**arc)), + ) .field("enable_profiling", &self.enable_profiling) .field("profiles_sample_rate", &self.profiles_sample_rate) .field("max_breadcrumbs", &self.max_breadcrumbs) @@ -231,6 +244,7 @@ impl Default for ClientOptions { environment: None, sample_rate: 1.0, traces_sample_rate: 0.0, + traces_sampler: None, enable_profiling: false, profiles_sample_rate: 0.0, max_breadcrumbs: 100, diff --git a/sentry-core/src/hub.rs b/sentry-core/src/hub.rs index 3fe0134b..20c1cb53 100644 --- a/sentry-core/src/hub.rs +++ b/sentry-core/src/hub.rs @@ -42,12 +42,12 @@ pub(crate) struct HubImpl { impl HubImpl { pub(crate) fn with R, R>(&self, f: F) -> R { let guard = self.stack.read().unwrap_or_else(PoisonError::into_inner); - f(&*guard) + f(&guard) } fn with_mut R, R>(&self, f: F) -> R { let mut guard = self.stack.write().unwrap_or_else(PoisonError::into_inner); - f(&mut *guard) + f(&mut guard) } fn is_active_and_usage_safe(&self) -> bool { diff --git a/sentry-core/src/performance.rs b/sentry-core/src/performance.rs index 292198fd..6672d6de 100644 --- a/sentry-core/src/performance.rs +++ b/sentry-core/src/performance.rs @@ -156,6 +156,13 @@ impl TransactionContext { } } +/// A function to be run for each new transaction, to determine the rate at which +/// it should be sampled. +/// +/// This function may choose to respect the sampling of the parent transaction (`ctx.sampled`) +/// or ignore it. +pub type TracesSampler = dyn Fn(&TransactionContext) -> f32 + Send + Sync; + // global API types: /// A wrapper that groups a [`Transaction`] and a [`Span`] together. @@ -279,6 +286,37 @@ pub(crate) struct TransactionInner { type TransactionArc = Arc>; +/// Functional implementation of how a new transation's sample rate is chosen. +/// +/// Split out from `Client.is_transaction_sampled` for testing. +#[cfg(feature = "client")] +fn transaction_sample_rate( + traces_sampler: Option<&TracesSampler>, + ctx: &TransactionContext, + traces_sample_rate: f32, +) -> f32 { + match (traces_sampler, traces_sample_rate) { + (Some(traces_sampler), _) => traces_sampler(ctx), + (None, traces_sample_rate) => ctx + .sampled + .map(|sampled| if sampled { 1.0 } else { 0.0 }) + .unwrap_or(traces_sample_rate), + } +} + +/// Determine whether the new transaction should be sampled. +#[cfg(feature = "client")] +impl Client { + fn is_transaction_sampled(&self, ctx: &TransactionContext) -> bool { + let client_options = self.options(); + self.sample_should_send(transaction_sample_rate( + client_options.traces_sampler.as_deref(), + ctx, + client_options.traces_sample_rate, + )) + } +} + /// A running Performance Monitoring Transaction. /// /// The transaction needs to be explicitly finished via [`Transaction::finish`], @@ -292,18 +330,9 @@ pub struct Transaction { impl Transaction { #[cfg(feature = "client")] fn new(mut client: Option>, ctx: TransactionContext) -> Self { - let context = protocol::TraceContext { - trace_id: ctx.trace_id, - parent_span_id: ctx.parent_span_id, - op: Some(ctx.op), - ..Default::default() - }; - let (sampled, mut transaction) = match client.as_ref() { Some(client) => ( - ctx.sampled.unwrap_or_else(|| { - client.sample_should_send(client.options().traces_sample_rate) - }), + client.is_transaction_sampled(&ctx), Some(protocol::Transaction { name: Some(ctx.name), #[cfg(all(feature = "profiling", target_family = "unix"))] @@ -314,6 +343,13 @@ impl Transaction { None => (ctx.sampled.unwrap_or(false), None), }; + let context = protocol::TraceContext { + trace_id: ctx.trace_id, + parent_span_id: ctx.parent_span_id, + op: Some(ctx.op), + ..Default::default() + }; + // throw away the transaction here, which means there is nothing to send // on `finish`. if !sampled { @@ -679,4 +715,36 @@ mod tests { assert_eq!(&parsed.0.to_string(), "09e04486820349518ac7b5d2adbf6ba5"); assert_eq!(parsed.2, Some(true)); } + + #[cfg(feature = "client")] + #[test] + fn compute_transaction_sample_rate() { + // Global rate used as fallback. + let ctx = TransactionContext::new("noop", "noop"); + assert_eq!(transaction_sample_rate(None, &ctx, 0.3), 0.3); + assert_eq!(transaction_sample_rate(None, &ctx, 0.7), 0.7); + + // If only global rate, setting sampled overrides it + let mut ctx = TransactionContext::new("noop", "noop"); + ctx.set_sampled(true); + assert_eq!(transaction_sample_rate(None, &ctx, 0.3), 1.0); + ctx.set_sampled(false); + assert_eq!(transaction_sample_rate(None, &ctx, 0.3), 0.0); + + // If given, sampler function overrides everything else. + let mut ctx = TransactionContext::new("noop", "noop"); + assert_eq!(transaction_sample_rate(Some(&|_| { 0.7 }), &ctx, 0.3), 0.7); + ctx.set_sampled(false); + assert_eq!(transaction_sample_rate(Some(&|_| { 0.7 }), &ctx, 0.3), 0.7); + // But the sampler may choose to inspect parent sampling + let sampler = |ctx: &TransactionContext| match ctx.sampled { + Some(true) => 0.8, + Some(false) => 0.4, + None => 0.6, + }; + ctx.set_sampled(true); + assert_eq!(transaction_sample_rate(Some(&sampler), &ctx, 0.3), 0.8); + ctx.set_sampled(None); + assert_eq!(transaction_sample_rate(Some(&sampler), &ctx, 0.3), 0.6); + } } diff --git a/sentry-core/src/profiling.rs b/sentry-core/src/profiling.rs index bc7013fd..5d421bee 100644 --- a/sentry-core/src/profiling.rs +++ b/sentry-core/src/profiling.rs @@ -241,7 +241,7 @@ fn get_profile_from_report( timestamp: rep.timing.start_time, transactions: vec![TransactionMetadata { id: transaction.event_id, - name: transaction.name.clone().unwrap_or_else(|| "".to_string()), + name: transaction.name.clone().unwrap_or_default(), trace_id, relative_start_ns: 0, relative_end_ns: transaction diff --git a/sentry-panic/src/lib.rs b/sentry-panic/src/lib.rs index 467d7697..4c00cab1 100644 --- a/sentry-panic/src/lib.rs +++ b/sentry-panic/src/lib.rs @@ -77,7 +77,7 @@ impl Integration for PanicIntegration { /// Extract the message of a panic. pub fn message_from_panic_info<'a>(info: &'a PanicInfo<'_>) -> &'a str { match info.payload().downcast_ref::<&'static str>() { - Some(s) => *s, + Some(s) => s, None => match info.payload().downcast_ref::() { Some(s) => &s[..], None => "Box", diff --git a/sentry-types/src/protocol/envelope.rs b/sentry-types/src/protocol/envelope.rs index ac38cdf7..43a5182c 100644 --- a/sentry-types/src/protocol/envelope.rs +++ b/sentry-types/src/protocol/envelope.rs @@ -388,7 +388,7 @@ impl Envelope { let payload_start = std::cmp::min(header_end + 1, slice.len()); let payload_end = match header.length { Some(len) => { - let payload_end = payload_start + len as usize; + let payload_end = payload_start + len; if slice.len() < payload_end { return Err(EnvelopeError::UnexpectedEof); } diff --git a/sentry-types/src/protocol/v7.rs b/sentry-types/src/protocol/v7.rs index a1d279cc..dcf08388 100644 --- a/sentry-types/src/protocol/v7.rs +++ b/sentry-types/src/protocol/v7.rs @@ -1360,7 +1360,7 @@ impl Default for SpanId { impl fmt::Display for SpanId { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - write!(fmt, "{}", hex::encode(&self.0)) + write!(fmt, "{}", hex::encode(self.0)) } } @@ -1406,7 +1406,7 @@ impl Default for TraceId { impl fmt::Display for TraceId { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - write!(fmt, "{}", hex::encode(&self.0)) + write!(fmt, "{}", hex::encode(self.0)) } } diff --git a/sentry/src/transports/curl.rs b/sentry/src/transports/curl.rs index d7af0e32..b4045954 100644 --- a/sentry/src/transports/curl.rs +++ b/sentry/src/transports/curl.rs @@ -31,7 +31,7 @@ impl CurlHttpTransport { let http_proxy = options.http_proxy.as_ref().map(ToString::to_string); let https_proxy = options.https_proxy.as_ref().map(ToString::to_string); let dsn = options.dsn.as_ref().unwrap(); - let user_agent = options.user_agent.to_owned(); + let user_agent = options.user_agent.clone(); let auth = dsn.to_auth(Some(&user_agent)).to_string(); let url = dsn.envelope_api_url().to_string(); let scheme = dsn.scheme(); diff --git a/sentry/src/transports/reqwest.rs b/sentry/src/transports/reqwest.rs index 1bee5abc..8e79896f 100644 --- a/sentry/src/transports/reqwest.rs +++ b/sentry/src/transports/reqwest.rs @@ -56,7 +56,7 @@ impl ReqwestHttpTransport { builder.build().unwrap() }); let dsn = options.dsn.as_ref().unwrap(); - let user_agent = options.user_agent.to_owned(); + let user_agent = options.user_agent.clone(); let auth = dsn.to_auth(Some(&user_agent)).to_string(); let url = dsn.envelope_api_url().to_string(); diff --git a/sentry/src/transports/surf.rs b/sentry/src/transports/surf.rs index 0b147f4e..546cfae1 100644 --- a/sentry/src/transports/surf.rs +++ b/sentry/src/transports/surf.rs @@ -43,7 +43,7 @@ impl SurfHttpTransport { } let dsn = options.dsn.as_ref().unwrap(); - let user_agent = options.user_agent.to_owned(); + let user_agent = options.user_agent.clone(); let auth = dsn.to_auth(Some(&user_agent)).to_string(); let url = dsn.envelope_api_url().to_string(); diff --git a/sentry/src/transports/ureq.rs b/sentry/src/transports/ureq.rs index 3fcc90ea..077f7c8d 100644 --- a/sentry/src/transports/ureq.rs +++ b/sentry/src/transports/ureq.rs @@ -112,7 +112,7 @@ impl UreqHttpTransport { builder.build() }); - let user_agent = options.user_agent.to_owned(); + let user_agent = options.user_agent.clone(); let auth = dsn.to_auth(Some(&user_agent)).to_string(); let url = dsn.envelope_api_url().to_string(); diff --git a/sentry/tests/test_tracing.rs b/sentry/tests/test_tracing.rs index f087dc61..07eacedd 100644 --- a/sentry/tests/test_tracing.rs +++ b/sentry/tests/test_tracing.rs @@ -106,7 +106,7 @@ fn test_tracing() { #[tracing::instrument(fields(span_field))] fn function() { - tracing::Span::current().record("span_field", &"some data"); + tracing::Span::current().record("span_field", "some data"); } #[test]