Skip to content
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

Dynamic Sampling baggage header changes #23

Closed
6 tasks done
marandaneto opened this issue Jun 24, 2022 · 9 comments
Closed
6 tasks done

Dynamic Sampling baggage header changes #23

marandaneto opened this issue Jun 24, 2022 · 9 comments

Comments

@marandaneto
Copy link

marandaneto commented Jun 24, 2022

Sentry Spec https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/
JS implementation: getsentry/sentry-javascript#5292 (there are more PRs related to this)
W3 Spec https://www.w3.org/TR/baggage/

@adinauer
Copy link
Member

  • Add sample_rate to trace envelope header (should be tracesSampleRate or tracesSampler callback result; see Add tracesSampler as source for sample_rate develop#619)
  • Add sentry-sample_rate to baggage (same value as above)
  • Rename keys in baggage to have _, e.g. sentry-trace_id instead of sentry-traceid
  • Flatten user in trace envelope header into user_id and user_segment

@marandaneto
Copy link
Author

@antonpirker please let us know when the backend sample is ready for testing so we can validate on our end as well, thank you for doing this.

@marandaneto
Copy link
Author

@brustolin and @adinauer Please consider adding the getsentry/develop#625 check.

@marandaneto
Copy link
Author

Consider:

For all implementors of the sampling context: please double check that your implementation always writes the sample rate as a string with only basic floating point numbers (e.g. 0.123). Ensure that whichever stringify function you use never goes into exponent notation for extra small sample rates and you do not serialize invalid values like NaN or Inf (which shouldn't exist in your impl anyway).

@marandaneto
Copy link
Author

marandaneto commented Jul 5, 2022

For now, because of PII reasons, we're ditching the sentry-transaction from the Baggage header.
Also sentry-user_id which was guarded by isSendDefaultPii as well.
Spec is in the making by the Web team.

Edit: See getsentry/develop#635

@marandaneto
Copy link
Author

#30 has to be done, but not now for Mobile.

@marandaneto
Copy link
Author

marandaneto commented Jul 21, 2022

#30 is also a mandatory field for DS.
Doc changes https://github.com/getsentry/develop/pull/651/files

@marandaneto
Copy link
Author

@brustolin and @adinauer can you tick the boxes in the PR description that are done? Most likely Cocoa, and Test Android/Cocoa?

@adinauer
Copy link
Member

To figure out priority of #39 we should check with other teams / SDKs if this was already implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants