feat(sampling): Add profile context early and expose getter for it #2715
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,8 +31,8 @@ use relay_event_normalization::{GeoIpLookup, RawUserAgentInfo}; | |
| use relay_event_schema::processor::{self, ProcessingAction, ProcessingState}; | ||
| use relay_event_schema::protocol::{ | ||
| Breadcrumb, ClientReport, Contexts, Csp, Event, EventType, ExpectCt, ExpectStaple, Hpkp, | ||
| IpAddr, LenientString, Metrics, NetworkReportError, OtelContext, ProfileContext, RelayInfo, | ||
| Replay, SecurityReportType, SessionAggregates, SessionAttributes, SessionStatus, SessionUpdate, | ||
| Timestamp, TraceContext, UserReport, Values, | ||
| }; | ||
| use relay_filter::FilterStatKey; | ||
|
|
@@ -58,7 +58,7 @@ use { | |
| crate::actors::project_cache::UpdateRateLimits, | ||
| crate::utils::{EnvelopeLimiter, MetricsLimiter}, | ||
| relay_event_normalization::{span, StoreConfig, StoreProcessor}, | ||
| relay_event_schema::protocol::Span, | ||
| relay_metrics::Aggregator, | ||
| relay_quotas::{RateLimitingError, RedisRateLimiter}, | ||
| relay_redis::RedisPool, | ||
|
|
@@ -1096,15 +1096,15 @@ impl EnvelopeProcessorService { | |
| .items() | ||
| .filter(|item| item.ty() == &ItemType::Transaction) | ||
| .count(); | ||
| let mut profile_id = None; | ||
| state.managed_envelope.retain_items(|item| match item.ty() { | ||
| // Drop profile without a transaction in the same envelope. | ||
| ItemType::Profile if transaction_count == 0 => ItemAction::DropSilently, | ||
| // First profile found in the envelope, we'll keep it if metadata are valid. | ||
| ItemType::Profile if profile_id.is_none() => { | ||
| match relay_profiling::parse_metadata(&item.payload(), state.project_id) { | ||
| Ok(id) => { | ||
| profile_id = Some(id); | ||
| ItemAction::Keep | ||
| } | ||
| Err(err) => ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling( | ||
|
|
@@ -1118,7 +1118,22 @@ impl EnvelopeProcessorService { | |
| ))), | ||
| _ => ItemAction::Keep, | ||
| }); | ||
| state.has_profile = profile_id.is_some(); | ||
|
|
||
| if let Some(event) = state.event.value_mut() { | ||
| if event.ty.value() == Some(&EventType::Transaction) { | ||
| let contexts = event.contexts.get_or_insert_with(Contexts::new); | ||
| // If we found a profile, add its ID to the profile context on the transaction. | ||
| if let Some(profile_id) = profile_id { | ||
| contexts.add(ProfileContext { | ||
| profile_id: Annotated::new(profile_id), | ||
| }); | ||
| // If not, we delete the profile context. | ||
| } else { | ||
| contexts.remove::<ProfileContext>(); | ||
| } | ||
iker-barriocanal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
There was a problem hiding this comment. This block was copied and adapted from |
||
| } | ||
|
|
||
| /// Normalize monitor check-ins and remove invalid ones. | ||
|
|
@@ -1182,17 +1197,13 @@ impl EnvelopeProcessorService { | |
| } | ||
| _ => ItemAction::Keep, | ||
| }); | ||
| 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>(); | ||
| } | ||
|
Comment on lines
+1200
to
+1206
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 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.
That is correct and a bit unfortunate. To fix this we would have to move the entirety of There was a problem hiding this comment. Sounds good, thanks for the explanation! |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -2776,7 +2787,6 @@ impl EnvelopeProcessorService { | |
|
|
||
| if_processing!({ | ||
| self.enforce_quotas(state)?; | ||
| self.process_profiles(state); | ||
| self.process_check_ins(state); | ||
| }); | ||
|
|
||
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.
I added this type alias to make the return type more obvious in function signatures.
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.
Shouldn't we just new type it here instead?
Then it's actually a separate type and cant get mixed