fix(sampling): Remove stray sampling data tags #3197
Conversation
| transactionSampling: { | ||
| method: TransactionSamplingMethod.Sampler, | ||
| // cast to number in case it's a boolean | ||
| rate: Number(sampleRate), |
There was a problem hiding this comment.
@jan-auer Once consequence of this change is that rates will come through as numbers rather than strings. (When I did the original PR, tags had to be strings, which is why I stringified the value in the first place.) LMK if that's not good from a relay perspective and I can turn them back into strings.
There was a problem hiding this comment.
Is this what you put into the sample_rates item header? The schema in there is:
type SampleRate = {
id: String,
rate: Number,
}If this goes into the header, let's rather keep it consistent to avoid mistakes please. As for the rate, that should be a number all along.
size-limit report
|
| @@ -3,6 +3,7 @@ | |||
| **/ | |||
| export interface DebugMeta { | |||
| images?: Array<DebugImage>; | |||
| transactionSampling?: { rate?: number; method?: string }; | |||
There was a problem hiding this comment.
If we call it rate and id in the protocol, couldn't we unify it here as well? It'd make some object deconstruction easier.
There was a problem hiding this comment.
It would make object deconstruction easier. I didn't do it because id is opaque. What's it the id of? Unless you feel strongly I'd rather keep it as is.
There was a problem hiding this comment.
I totally agree, but why is Relay using it in the first place? :P
There was a problem hiding this comment.
You'll have to take that up with @jan-auer 😛
There was a problem hiding this comment.
See also #3197 (comment). Relay doesn't use this field, but it also sample in its own way. The id field identifies not only the method but in case of Relay also the rule which applied.
Ultimately, this all ends up in Sentry and in our data pipeline.
lobsterkatie
force-pushed
the
kmclb-fix-sampling-metadata-tags
branch
from
75ee7af
to
ab8e93b
Compare
lobsterkatie
force-pushed
the
kmclb-fix-sampling-metadata-tags
branch
from
ab8e93b
to
c69a371
Compare
Follow up to #3068.
In that PR, tags were used as a temporary way to get data through the event processors to the point where envelope headers are created. Once used, they were deleted from
event.tags. Unfortunately, by that point they'd already been copied intocontexts.trace, from which they weren't being deleted, causing them to show up in the UI.This PR fixes that by using
debug_metainstead oftags(which means that even if something goes wrong, it's only visible in the event JSON, not the UI) and adds a test to ensure that the data isn't where it shouldn't be. (It also does some cleanup work in that test suite to simplify things.)