-
Notifications
You must be signed in to change notification settings - Fork 112
feat(contexts): Add TraceId by default #5759
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,6 +166,9 @@ pub struct NormalizationConfig<'a> { | |
|
|
||
| /// Set a flag to enable performance issue detection on spans. | ||
| pub performance_issues_spans: bool, | ||
|
|
||
| /// Should add a random trace ID to events that lack one. | ||
| pub derive_trace_id: bool, | ||
| } | ||
|
|
||
| impl Default for NormalizationConfig<'_> { | ||
|
|
@@ -201,6 +204,7 @@ impl Default for NormalizationConfig<'_> { | |
| span_allowed_hosts: Default::default(), | ||
| span_op_defaults: Default::default(), | ||
| performance_issues_spans: Default::default(), | ||
| derive_trace_id: Default::default(), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -333,8 +337,11 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) { | |
| Ok(()) | ||
| }); | ||
|
|
||
| // We know this exists thanks to normalize_default_attributes. | ||
| let event_id = event.id.value().unwrap().0; | ||
|
|
||
| // Some contexts need to be normalized before metrics extraction takes place. | ||
| normalize_contexts(&mut event.contexts); | ||
| normalize_contexts(&mut event.contexts, event_id, config); | ||
|
|
||
| if config.normalize_spans && event.ty.value() == Some(&EventType::Transaction) { | ||
| span::reparent_broken_spans::reparent_broken_spans(event); | ||
|
|
@@ -1307,13 +1314,29 @@ fn remove_logger_word(tokens: &mut Vec<&str>) { | |
| } | ||
|
|
||
| /// Normalizes incoming contexts for the downstream metric extraction. | ||
| fn normalize_contexts(contexts: &mut Annotated<Contexts>) { | ||
| fn normalize_contexts( | ||
| contexts: &mut Annotated<Contexts>, | ||
| event_id: Uuid, | ||
| config: &NormalizationConfig, | ||
| ) { | ||
| if config.derive_trace_id { | ||
| // We will always need a TraceContext. | ||
| let _ = contexts.get_or_insert_with(Contexts::new); | ||
| } | ||
|
|
||
| let _ = processor::apply(contexts, |contexts, _meta| { | ||
| // Reprocessing context sent from SDKs must not be accepted, it is a Sentry-internal | ||
| // construct. | ||
| // [`normalize`] does not run on renormalization anyway. | ||
| contexts.0.remove("reprocessing"); | ||
|
|
||
| // We need a TraceId to ingest the event into EAP. | ||
| // If the event lacks a TraceContext, add a random one. | ||
|
|
||
| if config.derive_trace_id && !contexts.contains::<TraceContext>() { | ||
| contexts.add(TraceContext::random(event_id)) | ||
| } | ||
|
|
||
|
Comment on lines
+1333
to
+1339
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of two non-test changes. |
||
| for annotated in &mut contexts.0.values_mut() { | ||
| if let Some(ContextInner(Context::Trace(context))) = annotated.value_mut() { | ||
| context.status.get_or_insert_with(|| SpanStatus::Unknown); | ||
|
|
@@ -4003,6 +4026,122 @@ mod tests { | |
| "###); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_normalize_adds_trace_id() { | ||
| let json = r#" | ||
| { | ||
| "type": "transaction", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually want this normalization to also apply to transactions? If not, we should probably filter on the event type in |
||
| "timestamp": "2021-04-26T08:00:05+0100", | ||
| "start_timestamp": "2021-04-26T08:00:00+0100", | ||
| "measurements": { | ||
| "inp": {"value": 120.0} | ||
| } | ||
| } | ||
| "#; | ||
|
|
||
| let mut event = Annotated::<Event>::from_json(json).unwrap().0.unwrap(); | ||
|
|
||
| let performance_score: PerformanceScoreConfig = serde_json::from_value(json!({ | ||
| "profiles": [ | ||
| { | ||
| "name": "Desktop", | ||
| "scoreComponents": [ | ||
| { | ||
| "measurement": "inp", | ||
| "weight": 1.0, | ||
| "p10": 0.1, | ||
| "p50": 0.25 | ||
| }, | ||
| ], | ||
| "condition": { | ||
| "op":"and", | ||
| "inner": [] | ||
| }, | ||
| "version": "beta" | ||
| } | ||
| ] | ||
| })) | ||
| .unwrap(); | ||
|
|
||
| normalize( | ||
| &mut event, | ||
| &mut Meta::default(), | ||
| &NormalizationConfig { | ||
| performance_score: Some(&performance_score), | ||
| derive_trace_id: true, | ||
| ..Default::default() | ||
| }, | ||
| ); | ||
|
|
||
| insta::assert_ron_snapshot!(SerializableAnnotated(&event.contexts), { | ||
| ".event_id" => "[event-id]", | ||
| ".trace.trace_id" => "[trace-id]", | ||
| ".trace.span_id" => "[span-id]" | ||
| }, @r#" | ||
| { | ||
| "performance_score": { | ||
| "score_profile_version": "beta", | ||
| "type": "performancescore", | ||
| }, | ||
| "trace": { | ||
| "trace_id": "[trace-id]", | ||
| "span_id": "[span-id]", | ||
| "status": "unknown", | ||
| "exclusive_time": 5000.0, | ||
| "type": "trace", | ||
| }, | ||
| "_meta": { | ||
| "trace": { | ||
| "span_id": { | ||
| "": Meta(Some(MetaInner( | ||
| rem: [ | ||
| [ | ||
| "span_id.missing", | ||
| s, | ||
| ], | ||
| ], | ||
| ))), | ||
| }, | ||
| "trace_id": { | ||
| "": Meta(Some(MetaInner( | ||
| rem: [ | ||
| [ | ||
| "trace_id.missing", | ||
| s, | ||
| ], | ||
| ], | ||
| ))), | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| "#); | ||
| insta::assert_ron_snapshot!(SerializableAnnotated(&event.measurements), {}, @r###" | ||
| { | ||
| "inp": { | ||
| "value": 120.0, | ||
| "unit": "millisecond", | ||
| }, | ||
| "score.inp": { | ||
| "value": 0.0, | ||
| "unit": "ratio", | ||
| }, | ||
| "score.ratio.inp": { | ||
| "value": 0.0, | ||
| "unit": "ratio", | ||
| }, | ||
| "score.total": { | ||
| "value": 0.0, | ||
| "unit": "ratio", | ||
| }, | ||
| "score.weight.inp": { | ||
| "value": 1.0, | ||
| "unit": "ratio", | ||
| }, | ||
| } | ||
| "###); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_computes_standalone_cls_performance_score() { | ||
| let json = r#" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,6 +168,13 @@ pub struct SpanId([u8; 8]); | |
|
|
||
| relay_common::impl_str_serde!(SpanId, "a span identifier"); | ||
|
|
||
| impl SpanId { | ||
| pub fn random() -> Self { | ||
| let value: u64 = rand::random_range(1..=u64::MAX); | ||
| Self(value.to_ne_bytes()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Suggested FixIn Prompt for AI Agent |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SpanId byte order inconsistency between
|
||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| impl FromStr for SpanId { | ||
| type Err = Error; | ||
|
|
||
|
|
@@ -324,6 +331,23 @@ pub struct TraceContext { | |
| pub other: Object<Value>, | ||
| } | ||
|
|
||
| impl TraceContext { | ||
| /// Generates a random [`SpanId`] and takes `[TraceId]` from the event's UUID. | ||
| /// Leaves all other fields blank. | ||
| pub fn random(event_id: Uuid) -> Self { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method named random but trace_id is deterministicLow Severity
Reviewed by Cursor Bugbot for commit 9927ecc. Configure here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could rename to |
||
| let mut trace_meta = Meta::default(); | ||
| trace_meta.add_remark(Remark::new(RemarkType::Substituted, "trace_id.missing")); | ||
|
|
||
| let mut span_meta = Meta::default(); | ||
| span_meta.add_remark(Remark::new(RemarkType::Substituted, "span_id.missing")); | ||
|
Comment on lines
+339
to
+342
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will these remarks show up in the UI if we merge this right now? Might be worth trying with a local relay hooked up to production sentry.io before merging (see https://github.com/getsentry/relay/?tab=readme-ov-file#building-and-running). I'm worried that UI annotates this as some kind of processing error, and then users see this on almost every error event. An alternative would be to not set a remark at all, and rather set the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The UI doesn't show meta at all right now from the spans dataset. It's a task for a project upcoming shortly (attribute explorer).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But it does for the errors data set, right? I.e. JSON ends up in nodestore, and the views in Issues will render at least some of the |
||
| TraceContext { | ||
| trace_id: Annotated(Some(TraceId::from(event_id)), trace_meta), | ||
| span_id: Annotated(Some(SpanId::random()), span_meta), | ||
| ..Default::default() | ||
| } | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
Comment on lines
+334
to
+349
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of two non-test changes. |
||
|
|
||
| impl super::DefaultContext for TraceContext { | ||
| fn default_key() -> &'static str { | ||
| "trace" | ||
|
|
@@ -615,4 +639,31 @@ mod tests { | |
| let remark = annotated.meta().iter_remarks().next().unwrap(); | ||
| assert_eq!(remark.rule_id(), "trace_id.invalid"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_random_trace_context() { | ||
| let rand_context = TraceContext::random(Uuid::new_v4()); | ||
| assert!(rand_context.trace_id.value().is_some()); | ||
| assert_eq!( | ||
| rand_context | ||
| .trace_id | ||
| .meta() | ||
| .iter_remarks() | ||
| .next() | ||
| .unwrap() | ||
| .rule_id(), | ||
| "trace_id.missing" | ||
| ); | ||
| assert!(rand_context.span_id.value().is_some()); | ||
| assert_eq!( | ||
| rand_context | ||
| .span_id | ||
| .meta() | ||
| .iter_remarks() | ||
| .next() | ||
| .unwrap() | ||
| .rule_id(), | ||
| "span_id.missing" | ||
| ); | ||
| } | ||
| } | ||


Uh oh!
There was an error while loading. Please reload this page.