feat(sampling): Add profile context early and expose getter for it #2715
Conversation
jjbayer
changed the title
| /// Unique identifier for a profile. | ||
| /// | ||
| /// Same format as event IDs. | ||
| pub type ProfileId = EventId; |
There was a problem hiding this comment.
I added this type alias to make the return type more obvious in function signatures.
There was a problem hiding this comment.
Shouldn't we just new type it here instead?
Then it's actually a separate type and cant get mixed
| contexts.remove::<ProfileContext>(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block was copied and adapted from process_profiles.
| if found_profile_id.is_none() { | ||
| // Remove profile context from event. | ||
| if let Some(event) = state.event.value_mut() { | ||
| if event.ty.value() == Some(&EventType::Transaction) { | ||
| if let Some(contexts) = event.contexts.value_mut() { | ||
| contexts.remove::<ProfileContext>(); | ||
| } |
There was a problem hiding this comment.
When profile processing fails, the profile ID is removed here. This means that some (invalid) profiles that were opted in by a dynamic sampling rule based on contexts.profile.profile_id might end up without a profile ID in the final payload.
There was a problem hiding this comment.
So this implies that we are performing sampling on profile ids that we don't know whether they are valid of not, am I right?
From the code flow, I see that we do filter the profile with context injection, we then move onto sampling (assuming the profile id refers to a valid profile), and then we process the profile and remove the context in case it's invalid?
Is this an expected behavior? Just curious
There was a problem hiding this comment.
So this implies that we are performing sampling on profile ids that we don't know whether they are valid of not, am I right?
That is correct and a bit unfortunate. To fix this we would have to move the entirety of process_profiles to non-processing relays (and before dynamic sampling), and I'm not sure if that's currently possible. cc @jan-auer
There was a problem hiding this comment.
Sounds good, thanks for the explanation!
There was a problem hiding this comment.
The caveat of over-sampling is fine, at least it will still catch events with a profile.
In the longer run, a sampling rule like this might not have the ideal semantics. We would ideally want to sample events where there's a profile in the same trace. However, we're currently not able to guarantee such a rule.
In order to create dynamic sampling rules depending on the profile ID, add the profile ID to the transaction event's
profilecontext and expose it via the event'sGetterimplementation.Note that most transaction processing still occurs only in processing relays, and it might happen that events that have a profile ID at the time of sampling will not have a profile ID anymore after processing.
Fixes #2710