feat: Add data option in tracing integration to force sampling of transaction

[Ten0]

Description

Similarly to sentry.trace, this allows overriding whether the transaction is sampled while still going through the tracing integration.

[cursor]

Bug: `sentry.sample` field not removed in `on_record` method

The on_record method removes SENTRY_TRACE_FIELD to prevent it from being recorded as span data since it cannot be applied retroactively. However, the new SENTRY_SAMPLE_FIELD is not similarly removed. This causes sentry.sample to be incorrectly stored as span data when recorded retroactively via span.record, even though the value has no effect on sampling (which is only determined during on_new_span). This is inconsistent with how sentry.trace is handled and pollutes span data with useless fields.

sentry-tracing/src/layer.rs#L436-L437

sentry-rust/sentry-tracing/src/layer.rs

Lines 436 to 437 in 4072dc5

	// `sentry.trace` cannot be applied retroactively
	data.json_values.remove(SENTRY_TRACE_FIELD);

 

---

[codecov]

Codecov Report

❌ Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.80%. Comparing base (196501d) to head (4072dc5).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #945      +/-   ##
==========================================
- Coverage   73.81%   73.80%   -0.01%     
==========================================
  Files          64       64              
  Lines        7504     7517      +13     
==========================================
+ Hits         5539     5548       +9     
- Misses       1965     1969       +4     

[lcian]

Hi @Ten0, thank you for the PR.
Do you need tail-based sampling based on some attributes, or would using a span_filter work for you?

sentry-rust/sentry-tracing/src/layer.rs

Line 147 in 196501d

pub fn span_filter<F>(mut self, filter: F) -> Self

Generally, all of our other SDKs offer before_send_span/before_send_transaction hooks to centrally manage span filtering and mutation. So we should consider whether we want to introduce this, the hooks, or both.
There's also traces_sampler which is already available in this SDK, albeit less powerful (very similar to span_filter but it acts on whole traces as opposed to spans).

[Ten0]

Hello, yes I'm looking for specifying sampling not just based on the span itself, specifically I'm using another header than sentry trace to specify just "please sample this", without specifying a trace ID. That's useful e.g. when request is initialized via Insomnia.
I guess traces_sampler might work as well if I could efficiently read an attribute that I would dedicate to this...

[lcian]

@Ten0 thanks for the context.
I think TracesSampler wouldn't work, because you only have access to some limited information about the trace there like name and op but not attributes.

before_send_span/transaction however would work because that would happen, clearly, before sending, so all attributes would be available in that callback.
If we implemented before_send_span, you could even recreate the feature you're PRing here on your own, so that could be another point in favor of implementing one of these hooks.

[Ten0]

AFAICT this would not allow to avoid propagating sampling decisions to sub-transactions in other microservices: because the decision on whether to sample would be delegated to after everything has happened, it's unclear what to send in the sentry-trace header with regards to sampling decision.

That being said for now I can keep managing this somewhat-manually directly via the Sentry integration, and I would indeed like to be able to override sampling decision (from false to true) before sending, so being able to edit transaction at the end would be useful.

So overall it seems that what we could do is always allow reading/updating the sampling decision on the transaction itself.
That way I could init Sentry via tracing, then post-update sampling decision directly via the sentry SDK, and that would also allow me to update the sampling decision just before finishing the transaction (if e.g. it was very long so it deserves to be sampled).
While before_send_transaction might be useful here, we don't absolutely need it since we already have our own wrappers around the Sentry SDK, to avoid paying for the heavy Sentry machinery whenever I use a crate that uses tracing.

[indiv0]

While before_send_transaction might be useful here, we don't absolutely need it since we already have our own wrappers around the Sentry SDK, to avoid paying for the heavy Sentry machinery whenever I use a crate that uses tracing.

@Ten0 Tangential to the issue at hand, would you mind expanding on these wrappers you have? I believe I'm also encountering the heavy costs but I'm not sure how to work around them...

[Ten0]

We do this to avoid heavy Sentry stuff outside of places where we have started Sentry transactions:

use tracing_subscriber::{filter::LevelFilter, prelude::*};

pub(crate) fn init() {
	tracing_subscriber::Registry::default()
		.with(
			sentry::integrations::tracing::layer()
				.span_filter(|_| {
					// Constructing a transaction or span is not cheap, and the `tracing`/`sentry` integration
					// will crate a transaction if there isn't already one, so we should try to quickly filter
					// out such events if we know they are irrelevant before they hit the Sentry/Tracing
					// integration.
					//
					// This should also filter on `scope.is_sampled()` once that is public.
					// https://github.com/getsentry/sentry-rust/pull/669

					// This part of the filter cannot be put in `with_filter` below because function passed to
					// `with_filter` is expected to be a pure function, since it seems to be cached for
					// every span.

					// Note that this returns `false` when there's no Sentry Hub because bool::default() is false
					sentry::configure_scope(|scope| scope.get_span().is_some())
				})
				.with_filter(
					tracing_subscriber::filter::FilterFn::new(|metadata| {
						({
							// Don't catch random errors that may come from a bunch of underlying crates:
							// if we catch them in the code they shouldn't be reported as exceptions.
							// => We are only interested in span integration
							metadata.is_span()
						}) && {
							// Yes this needs to be written <= because of the ordering that is used by tracing
							*metadata.level() <= LevelFilter::INFO
						}
						// It would be fine to add more expensive filtering here because this function's result is
						// cached for every span.
					})
					.with_max_level_hint(LevelFilter::INFO),
				),
		)
		.init();
}