Skip to content

feat(contexts): Add TraceId by default#5759

Open
thetruecpaul wants to merge 4 commits intomasterfrom
cpaul/031926/default-trace-id
Open

feat(contexts): Add TraceId by default#5759
thetruecpaul wants to merge 4 commits intomasterfrom
cpaul/031926/default-trace-id

Conversation

@thetruecpaul
Copy link
Copy Markdown

We are beginning to store events in EAP, which — as a Trace-centric datastore — requires all TraceItems to be associated with a TraceId. This is problematic, since we currently don't require that events actually have TraceIds. That means that we're currently just silently dropping a bunch of events before we can successfully ingest them into EAP.

This PR adds a random TraceId & SpanId to events that do not have a TraceContext.

@thetruecpaul thetruecpaul requested a review from a team March 23, 2026 17:54
@thetruecpaul thetruecpaul requested a review from a team as a code owner March 23, 2026 17:54
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Random TraceContext not added when contexts is None
    • normalize_contexts now initializes Annotated<Contexts> with Contexts::new before processor::apply, ensuring a random TraceContext is added even when contexts were missing.

Create PR

Or push these changes by commenting:

@cursor push 42352a05a6
Preview (42352a05a6)
diff --git a/relay-event-normalization/src/event.rs b/relay-event-normalization/src/event.rs
--- a/relay-event-normalization/src/event.rs
+++ b/relay-event-normalization/src/event.rs
@@ -1308,6 +1308,7 @@
 
 /// Normalizes incoming contexts for the downstream metric extraction.
 fn normalize_contexts(contexts: &mut Annotated<Contexts>) {
+    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.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch 2 times, most recently from 95690de to 3579819 Compare March 23, 2026 18:15
@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch 4 times, most recently from 0fefa72 to 64d1970 Compare March 23, 2026 21:30
Comment on lines +336 to +351
impl TraceContext {
/// Generates a random [`TraceId`] and random [`SpanId`].
/// Leaves all other fields blank.
pub fn random() -> Self {
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"));
TraceContext {
trace_id: Annotated(Some(TraceId::random()), trace_meta),
span_id: Annotated(Some(SpanId::random()), span_meta),
..Default::default()
}
}
}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of two non-test changes.

Comment on lines +1320 to +1325
// We need a TraceId to ingest the event into EAP.
// If the event lacks a TraceContext, add a random one.
if !contexts.contains::<TraceContext>() {
contexts.add(TraceContext::random())
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of two non-test changes.

@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch 2 times, most recently from a159791 to eae2186 Compare March 23, 2026 23:15
@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch from eae2186 to e30157b Compare March 23, 2026 23:25
@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch 2 times, most recently from 5406ae5 to 75a2d8a Compare March 23, 2026 23:40
@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch from 75a2d8a to ba308fb Compare March 24, 2026 17:16
@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch 2 times, most recently from 785348e to 9c7ebde Compare March 24, 2026 17:38
@thetruecpaul thetruecpaul requested a review from mjq March 24, 2026 19:50
Comment on lines +346 to +349
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"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 origin field of the trace context to something like "relay".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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

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 _meta.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: SpanId byte order inconsistency between random and from_str
    • SpanId::random() now uses to_be_bytes() so its internal byte order matches SpanId::from_str() and remains consistent across platforms.

Create PR

Or push these changes by commenting:

@cursor push 73d35bc862
Preview (73d35bc862)
diff --git a/relay-event-schema/src/protocol/contexts/trace.rs b/relay-event-schema/src/protocol/contexts/trace.rs
--- a/relay-event-schema/src/protocol/contexts/trace.rs
+++ b/relay-event-schema/src/protocol/contexts/trace.rs
@@ -172,7 +172,7 @@
 impl SpanId {
     pub fn random() -> Self {
         let value: u64 = rand::random_range(1..=u64::MAX);
-        Self(value.to_ne_bytes())
+        Self(value.to_be_bytes())
     }
 }

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

pub fn random() -> Self {
let value: u64 = rand::random_range(1..=u64::MAX);
Self(value.to_ne_bytes())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpanId byte order inconsistency between random and from_str

Low Severity

SpanId::random() stores the u64 using to_ne_bytes() (native endian), but SpanId::from_str() stores it using to_be_bytes() (big endian). On little-endian platforms (virtually all modern servers), these produce different byte orderings for the same u64 value. While the Display/from_str round-trip happens to preserve bytes correctly, this inconsistency means randomly-generated SpanIds follow a different internal byte convention than parsed ones. Any future code that interprets the inner [u8; 8] as a u64 via from_be_bytes (the convention implied by from_str) would get wrong results for random SpanIds. Using to_be_bytes() here would maintain consistency.

Fix in Cursor Fix in Web

@thetruecpaul thetruecpaul requested a review from jjbayer March 30, 2026 18:13
Copy link
Copy Markdown
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks OK to me but I would be more comfortable if we could feature flag the auto-generation (see code linked below). Then we can dogfood it, see what it does to average payload sizes (e.g. in S4S) and check what the existing UI looks like.

/// Features exposed by project config.
#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub enum Feature {
)

We are beginning to store events in EAP, which — as a Trace-centric datastore — requires all TraceItems to be associated with a TraceId. This is problematic, since we currently don't require that events actually have TraceIds. That means that we're currently just silently dropping a bunch of events before we can successfully ingest them into EAP.

This PR adds a random TraceId & SpanId to events that do not have a TraceContext.
@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch from ff8697f to 7d666ae Compare April 1, 2026 21:15
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@thetruecpaul
Copy link
Copy Markdown
Author

Added a feature flag; PR adding it to flagpole up in https://github.com/getsentry/sentry-options-automator/pull/7064

@thetruecpaul thetruecpaul force-pushed the cpaul/031926/default-trace-id branch from 7d666ae to eeb9e61 Compare April 1, 2026 21:29
impl SpanId {
pub fn random() -> Self {
let value: u64 = rand::random_range(1..=u64::MAX);
Self(value.to_ne_bytes())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: SpanId::random() uses native-endian bytes (to_ne_bytes), but serialization/deserialization (Display/FromStr) expect big-endian. This causes incorrect roundtripping on little-endian systems.
Severity: MEDIUM

Suggested Fix

In SpanId::random(), change the call from rng.gen::<u64>().to_ne_bytes() to rng.gen::<u64>().to_be_bytes(). This will align the byte order during generation with the big-endian order expected by the Display and FromStr implementations, ensuring correct serialization and deserialization roundtrips.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: relay-event-schema/src/protocol/contexts/trace.rs#L174

Potential issue: A byte-ordering inconsistency exists in `SpanId` handling. The
`SpanId::random()` function generates an ID using native-endian byte order
(`to_ne_bytes()`), while the `Display` and `FromStr` implementations, used for
serialization and deserialization, assume big-endian byte order. On common little-endian
architectures like x86_64, this causes a randomly generated `SpanId` to fail a
serialization-deserialization roundtrip. For example, a generated ID will be displayed
with its bytes reversed, and parsing that string back will result in a different ID
value. This can lead to data integrity issues and broken trace continuity if these
randomly generated IDs are ever stored and re-ingested.

@thetruecpaul thetruecpaul requested a review from jjbayer April 2, 2026 20:31
@thetruecpaul
Copy link
Copy Markdown
Author

I'll go through and revert the test changes that are causing the failures once I get an OK on the new direction / use of Feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants