Uh oh!
There was an error while loading. Please reload this page.
Add sample rate to baggage as well as trace in envelope header and flatten user - #2135
Conversation
adinauer
commented
Jun 24, 2022
@AbhiPrasad@lforst@Lms24 can any of you please leave a review, once you feel the docs are ready to make sure this aligns? |
Codecov Report
@@ Coverage Diff @@## main #2135 +/- ##
============================================
- Coverage 80.95% 80.94% -0.02% - Complexity 3257 3290 +33
============================================
Files 231 233 +2 Lines 11964 12044 +80 Branches 1589 1594 +5 ============================================
+ Hits 9686 9749 +63 - Misses 1698 1712 +14 - Partials 580 583 +3
Continue to review full report at Codecov.
|
| } | ||
| public void setSampleRate(final @Nullable String sampleRate) { | ||
| set("sentry-samplerate", sampleRate); |
There was a problem hiding this comment.
We decided to make baggage and envelope header key names identical (except for the sentry- prefix in baggage keys) so that we're able to just dump the DSC data into both transport mechanisms without any mapping. (Linking to the preview here for more details)
Therefore, we should change the snake case keys to underscores (like in the example below)
| set("sentry-samplerate", sampleRate); | |
| set("sentry-sample_rate", sampleRate); |
There was a problem hiding this comment.
Does that also mean
sentry-publickeybecomessentry-public_keysentry-traceidbecomessentry_trace_idsentry-useridbecomessentry-user_idsentry-usersegmentbecomessentry-user_segment
There was a problem hiding this comment.
sentry-traceidbecomessentry_trace_id
It becomes sentry-trace_id, but that probably just was a typo ^^
There was a problem hiding this comment.
OK thanks, will update.
Yeah just a typo on the _ there.
| ## Unreleased | ||
| ### Features | ||
| - Add sample rate to baggage as well as trace in envelope header and flatten user ([#2135](https://github.com/getsentry/sentry-java/pull/2135)) |
There was a problem hiding this comment.
Merge main into the branch to fix the changelog.
| private static @Nullable String getSegment(final @NotNull User user) { | ||
| final Map<String, String> others = user.getOthers(); | ||
| if (others != null) { | ||
| return others.get("segment"); |
There was a problem hiding this comment.
Should segment be a new field in the Userprotocol maybe?
There was a problem hiding this comment.
JS seems to have it https://github.com/getsentry/sentry-javascript/blob/master/packages/types/src/user.ts while cocoa does not. Didn't check any other SDKs. Not sure what exactly this is used for / if it should be added. Can create a separate issue to track if you want.
There was a problem hiding this comment.
Yes, please, but let's be sure that the approach with others works if not now.
Also, in this issue, we should upgrade the protocol https://develop.sentry.dev/sdk/event-payloads/user/ (adding the new field).
There was a problem hiding this comment.
Maybe there's a confusion here. Retrieving segment from others is not new behaviour. It's been there before. See
| if (sampleRateAsDouble < 0.0 || sampleRateAsDouble > 1.0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Maybe we extract this logic from SentryOptions#setSampleRate and SentryOptions#setTracesSampleRate and reuse it as an util function.
There was a problem hiding this comment.
setSampleRate has sampleRate > 1.0 || sampleRate <= 0.0 while setTracesSampleRate has tracesSampleRate > 1.0 || tracesSampleRate < 0.0. Can add a util but won't reuse in both.
There was a problem hiding this comment.
Indeed, tracesSampleRate can be 0.0
| DecimalFormat df = | ||
| new DecimalFormat("#.################", DecimalFormatSymbols.getInstance(Locale.US)); | ||
| return df.format(sampleRateAsDouble); |
There was a problem hiding this comment.
We should avoid US locale and use Locale.ROOT instead, some custom OS versions might not have the US locale.
Why don't we just transform the Double to String and send it as it is?
There was a problem hiding this comment.
Because we don't want exponent formatting. 0.00000021.toString() would become 2.1E-7 which we don't want.
Can replace the locale.
There was a problem hiding this comment.
Is the spec defining the number of max digits? because ################ can lead to a different value, not sure if this would round or not.
There was a problem hiding this comment.
It would round on the last digit. Yes we're aligning with JS on the max digits here.
| return baggage; | ||
| } | ||
| public static final class TraceContextUser implements JsonUnknown, JsonSerializable { |
There was a problem hiding this comment.
Would that be a breaking change if people is upgrading SDKs from X to Y? how does the ser/deser behave if you're reading a JSON with the older structure.
There was a problem hiding this comment.
Yes, updated the code to parse from legacy JSON in #2141
* Commit tests * Add sample rate from traces sampler to DSC * Do not replace null with true/false * Restore sample rate in OutboxSender * Remove fallback for sampling decision from TraceContext * Remove sample rate fallback from TracesSamplingDecision * Test more envelope header trace fields for OutboxSender * CR changes
…y/sentry-java into feat/add-sample-rate-to-baggage
#2147) * Skip sending userId in DSC if send default pii is off * Add changelog * Add test case
📜 Description
Add sample rate to
baggageheader as well astracein envelope header.Flatten
userintouser_idanduser_segmentfortracein envelope header.💡 Motivation and Context
To allow for dynamic sampling and align with docs / other SDKs: getsentry/develop#613
💚 How did you test it?
📝 Checklist
🔮 Next steps