Skip to content

chore(contexts): Remove default-trace-ID option #5979

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.

Sign up for GitHub

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

Open
thetruecpaul wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore(contexts): Remove default-trace-ID option #5979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84c16a3
chore(contexts): Remove default-trace-ID option
thetruecpaul May 12, 2026
c024935
tests
thetruecpaul May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
- Enable compression for forwarded uploads. ([#5965](https://github.com/getsentry/relay/pull/5965))
- Change the default partitioning for the envelope buffer from semantic to round-robin. ([#5967](https://github.com/getsentry/relay/pull/5967))
- Enable retries for upload requests to upstream. ([#5975](https://github.com/getsentry/relay/pull/5975))
- Unconditionally create a trace context with a trace id for errors. ([#5979](https://github.com/getsentry/relay/pull/5979))

## 26.4.2

Expand Down
2 changes: 1 addition & 1 deletion py/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
## 0.9.25

- feat(tracemetric): Add TraceMetricByte datacategory ([#5719](https://github.com/getsentry/relay/pull/5719))
- **Breaking**: To prevent false positives, non-public email addresses (e.g. `user@localhost`) are no longer scrubbed by default. ([#5737](https://github.com/getsentry/relay/pull/5737))
- **Breaking**: To prevent false positives, non-public email addresses (e.g. `user@localhost`) are no longer scrubbed by default. ([#5737](https://github.com/getsentry/relay/pull/5737))

## 0.9.24

Expand Down
21 changes: 12 additions & 9 deletions py/tests/test_processing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,17 +119,20 @@ def test_normalize_user_agent(must_normalize):
)

if must_normalize:
assert event["contexts"] == {
"browser": {
"browser": "Firefox 15.0.1",
"name": "Firefox",
"version": "15.0.1",
"type": "browser",
},
"client_os": {"name": "Ubuntu", "os": "Ubuntu", "type": "os"},
assert event["contexts"]["browser"] == {
"browser": "Firefox 15.0.1",
"name": "Firefox",
"version": "15.0.1",
"type": "browser",
}
assert event["contexts"]["client_os"] == {
"name": "Ubuntu",
"os": "Ubuntu",
"type": "os",
}
assert set(event["contexts"].keys()) == {"browser", "client_os", "trace"}
else:
assert "contexts" not in event
assert set(event["contexts"].keys()) == {"trace"}
Copy link
Copy Markdown
Member

@shashjar shashjar May 18, 2026

Choose a reason for hiding this comment

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

Should we assert the fields contained in the trace context, e.g. trace_id or other necessary fields are present?

Copy link
Copy Markdown
Member

@Dav1dde Dav1dde May 20, 2026

Choose a reason for hiding this comment

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

Generally a good idea, but these are the Python library tests and this is a test for normalizing the user agent, so I think this is fine.



def test_validate_pii_selector():
Expand Down
1 change: 0 additions & 1 deletion relay-cabi/src/processing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,6 @@ pub unsafe extern "C" fn relay_store_normalizer_normalize_event(
span_allowed_hosts: &[], // only supported in relay
span_op_defaults: Default::default(), // only supported in relay
performance_issues_spans: Default::default(),
derive_trace_id: Default::default(),
};
normalize_event(&mut event, &normalization_config);

Expand Down
3 changes: 0 additions & 3 deletions relay-dynamic-config/src/feature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,6 @@ pub enum Feature {
/// Upload non-prosperodmp playstation attachments via the upload endpoint.
#[serde(rename = "projects:relay-playstation-uploads")]
PlaystationUploads,
/// Add a random trace ID to events that lack one.
#[serde(rename = "organizations:relay-default-trace-id")]
AddDefaultTraceID,
/// Enable experimental expansion of the unreal report in the endpoint rather than in the
/// processor. Only enable for organizations with sufficient attachment quota.
#[serde(rename = "organizations:relay-unreal-endpoint-expansion")]
Expand Down
66 changes: 47 additions & 19 deletions relay-event-normalization/src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use relay_event_schema::protocol::{
AsPair, Attributes, AutoInferSetting, ClientSdkInfo, Context, ContextInner, Contexts,
DebugImage, DeviceClass, Event, EventId, EventType, Exception, Headers, IpAddr, Level,
LogEntry, Measurement, Measurements, PerformanceScoreContext, ReplayContext, Request, Span,
SpanStatus, SpanV2, Tags, Timestamp, TraceContext, User, VALID_PLATFORMS,
SpanStatus, SpanV2, Tags, Timestamp, TraceContext, TraceId, User, VALID_PLATFORMS,
};
use relay_protocol::{
Annotated, Empty, Error, ErrorKind, FiniteF64, FromValue, Getter, Meta, Object, Remark,
Expand Down Expand Up @@ -168,9 +168,6 @@ 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,
Comment on lines -172 to -173
Copy link
Copy Markdown
Member

@Dav1dde Dav1dde May 20, 2026

Choose a reason for hiding this comment

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

I think we should keep this here, add some documentation why this is needed and then just permanently set it to true where the config gets created.

Usually I'd agree with just by default enabling it, but there is a large impact on tests which are testing unrelated things but now need to assert a random trace id and context.

We can still test the context creation with the boolean and have integration tests to ensure this actually is enabled.

}

impl Default for NormalizationConfig<'_> {
Expand Down Expand Up @@ -205,7 +202,6 @@ impl Default for NormalizationConfig<'_> {
span_allowed_hosts: Default::default(),
span_op_defaults: Default::default(),
performance_issues_spans: Default::default(),
derive_trace_id: Default::default(),
}
}
}
Expand Down Expand Up @@ -334,7 +330,7 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) {
let event_id = event.id.value().unwrap().0;

// Some contexts need to be normalized before metrics extraction takes place.
normalize_contexts(&mut event.contexts, event_id, config);
normalize_contexts(&mut event.contexts, event_id);

if config.normalize_spans && event.ty.value() == Some(&EventType::Transaction) {
crate::normalize::normalize_app_start_spans(event);
Expand Down Expand Up @@ -1355,15 +1351,9 @@ fn remove_logger_word(tokens: &mut Vec<&str>) {
}

/// Normalizes incoming contexts for the downstream metric extraction.
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);
}
fn normalize_contexts(contexts: &mut Annotated<Contexts>, event_id: Uuid) {
// We will always need a TraceContext.
Comment thread
cursor[bot] marked this conversation as resolved.
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
Expand All @@ -1374,8 +1364,13 @@ fn normalize_contexts(
// 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>() {
if !contexts.contains::<TraceContext>() {
Comment thread
Dav1dde marked this conversation as resolved.
Copy link
Copy Markdown
Member

@shashjar shashjar May 18, 2026

Choose a reason for hiding this comment

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

I think you could collapse this if/else with something like the below (does span_id default have to be added as well?)

let trace_ctx = contexts.get_or_default::<TraceContext>();
if trace_ctx.trace_id.0.is_none() {
    trace_ctx.trace_id = Annotated::new(TraceId::from(event_id))
}

Copy link
Copy Markdown
Contributor Author

@thetruecpaul thetruecpaul May 19, 2026

Choose a reason for hiding this comment

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

So, there are four possible cases:

  1. The TraceContext is not set. In this case, we generate a random SpanID and a TraceID from the EventID. (See TraceContext::random.)
  2. The TraceContext is set with a TraceID and SpanID. In this case, we want to do nothing.
  3. The TraceContext is set with no TraceID but an existing SpanID. In this case, we want to create new TraceID but not change the existing SpanID.
  4. The TraceContext is set with no TraceID and no SpanID. In this case, we are currently only generating the TraceID. My reasoning is that "empty context" is different from "missing context" and we should only make the minimal update needed for our purposes (ingesting to EAP), but I could be talked around to updating both for the purpose of consistency with case (1).

All this to say — what does default mean for TraceContext? Is it something with all the attributes set to None? Ideally we'd want to short-circuit to TraceContext::random if the context doesn't exist, not TraceContext::default or whatnot (this method doesn't exist).

Copy link
Copy Markdown
Member

@shashjar shashjar May 19, 2026

Choose a reason for hiding this comment

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

I would say we should defer to @Dav1dde on the question of default span ID (whether or not that should be added, and where that should be added, or if it is already).

But I think the code I attached above works for all of the cases in which trace ID needs to be populated (and additional if branches can be added if needed if there are other defaults we need to set)

Copy link
Copy Markdown
Member

@Dav1dde Dav1dde May 20, 2026

Choose a reason for hiding this comment

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

The more I look into this I begin to question whether this doesn't have a lot of unintentional side-effects.
We're generating a trace context here for the purpose of storing in EAP, but that context is also visible to users with side effects as running it through dynamic sampling to mark the sampling decision on the error.
That's more a fundamental concern which also was brought up with the introduction of the feature flag.

I would say we should defer to @Dav1dde on the question of default span ID (whether or not that should be added, and where that should be added, or if it is already).

The span_id is required on the context, that means if you want to generate a context it also needs to be filled with a span_id.

All this to say — what does default mean for TraceContext? Is it something with all the attributes set to None?

All attributes set to Annotated::empty() (practically None). There is an argument to be made, that it should not implement Default at all.

contexts.add(TraceContext::random(event_id))
Copy link
Copy Markdown
Member

@shashjar shashjar May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If there's a TraceContext with valid information but it's missing trace_id (not sure if this actually occurs in practice) - I think we'll end up overriding the entire context with a new one that has only trace_id? I think we want to get_or_default the trace context, then check whether trace_context.trace_id is null, and then set only trace_context.trace_id if so

Copy link
Copy Markdown
Member

@Dav1dde Dav1dde May 20, 2026

Choose a reason for hiding this comment

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

TraceContext::random interior should not add metadata to the individual fields that they were missing (trace_id/span_id).

If you want to make the trace context effectively required, instead we should add this marker to the context itself.

} else {
let trace_ctx = contexts.get_or_default::<TraceContext>();
if trace_ctx.trace_id.0.is_none() {
trace_ctx.trace_id = Annotated::new(TraceId::from(event_id))
Copy link
Copy Markdown
Member

@Dav1dde Dav1dde May 20, 2026

Choose a reason for hiding this comment

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

Isn't this inconsistent with TraceContext::random which does add metadata for a "missing" trace id?

}
Comment thread
cursor[bot] marked this conversation as resolved.
}
Comment on lines +1370 to 1374
Copy link
Copy Markdown

@sentry sentry Bot May 18, 2026

Choose a reason for hiding this comment

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

Bug: If an event has an existing but empty TraceContext, the normalization logic only sets the trace_id, leaving the required span_id unset, creating an inconsistent state.
Severity: MEDIUM

Suggested Fix

In the else branch where contexts.contains::<TraceContext>() is true, ensure that span_id is also set along with trace_id. This could be done by generating a new span_id if trace_ctx.span_id is None, similar to how trace_id is handled. Alternatively, consider replacing the partial context with a fully new one using TraceContext::random() if the existing one is incomplete.

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-normalization/src/event.rs#L1370-L1374

Potential issue: When an event is processed that already contains a `TraceContext` but
lacks a `trace_id` (e.g., `{"contexts": {"trace": {}}}`), the normalization logic enters
a branch that sets the `trace_id`. However, this logic fails to also generate and set
the `span_id`. This is inconsistent with the case where no `TraceContext` exists, in
which a new context is created with both a `trace_id` and a `span_id` via
`TraceContext::random()`. The result is a partially normalized trace context where the
`span_id`, a required field, remains `None`.


for annotated in &mut contexts.0.values_mut() {
Expand Down Expand Up @@ -4882,14 +4877,48 @@ mod tests {
},
);

insta::assert_ron_snapshot!(SerializableAnnotated(&event.contexts), {}, @r###"
insta::assert_ron_snapshot!(SerializableAnnotated(&event.contexts), {
".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": {
Expand Down Expand Up @@ -4958,7 +4987,6 @@ mod tests {
&mut Meta::default(),
&NormalizationConfig {
performance_score: Some(&performance_score),
derive_trace_id: true,
..Default::default()
},
);
Expand Down
Loading
Loading