-
-
Notifications
You must be signed in to change notification settings - Fork 171
feat: Add data option in tracing integration to force sampling of transaction #945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Report❌ Patch coverage is 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 |
|
Hi @Ten0, thank you for the PR. sentry-rust/sentry-tracing/src/layer.rs Line 147 in 196501d
Generally, all of our other SDKs offer |
|
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. |
|
@Ten0 thanks for the context.
|
Description
Similarly to
sentry.trace, this allows overriding whether the transaction is sampled while still going through thetracingintegration.