Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- Add metadata support for the `/upload` endpoint. ([#6028](https://github.com/getsentry/relay/pull/6028))
- Infer user agents and client addresses in the V2 standalone span pipeline. ([#6047](https://github.com/getsentry/relay/pull/6047))
- Conditionally allow additional exceptions on minidumps. ([#6063](https://github.com/getsentry/relay/pull/6063))
- Replace nil Trace IDs with random ones. ([#6079](https://github.com/getsentry/relay/pull/6079))

**Bug Fixes**:

Expand Down
2 changes: 1 addition & 1 deletion relay-conventions/sentry-conventions
7 changes: 4 additions & 3 deletions relay-event-normalization/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1372,9 +1372,10 @@ fn normalize_force_trace_context(event: &mut Event) {
let contexts = event.contexts.get_or_insert_with(Contexts::new);
let trace = contexts.get_or_default::<TraceContext>();

let trace_id = trace
.trace_id
.get_or_insert_with(|| TraceId::from(*event.id.get_or_insert_with(Default::default)));
let trace_id = trace.trace_id.get_or_insert_with(|| {
TraceId::try_from(*event.id.get_or_insert_with(Default::default))
.unwrap_or_else(|_| TraceId::random())
Comment on lines +1376 to +1377

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was another place where nil-trace IDs could originate.

});
let _ = trace
.span_id
.get_or_insert_with(|| SpanId::derive_from_trace_id(trace_id));
Expand Down
78 changes: 48 additions & 30 deletions relay-event-schema/src/protocol/contexts/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,61 +66,74 @@ impl TraceId {

relay_common::impl_str_serde!(TraceId, "a trace identifier");

/// Error for an invalid trace ID.
#[derive(Debug)]
pub enum InvalidTraceId {
/// The trace ID is all zeros.
Nil,
/// The trace ID is syntactically invalid.
Invalid,
}

impl FromStr for TraceId {
type Err = Error;
type Err = InvalidTraceId;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Uuid::parse_str(s)
.map(Into::into)
.map_err(|_| Error::invalid("the trace id is not valid"))
let uuid = Uuid::from_str(s).map_err(|_| InvalidTraceId::Uuid)?;
Self::try_from(uuid)
}
}

impl TryFrom<&str> for TraceId {
type Error = Error;
type Error = InvalidTraceId;

fn try_from(value: &str) -> Result<Self, Self::Error> {
value.parse()
Comment thread
sentry[bot] marked this conversation as resolved.
}
}

impl TryFrom<&[u8]> for TraceId {
type Error = Error;
type Error = InvalidTraceId;

fn try_from(value: &[u8]) -> Result<Self, Self::Error> {
let uuid =
Uuid::from_slice(value).map_err(|_| Error::invalid("the trace id is not valid"))?;
Ok(Self(uuid))
Uuid::from_slice(value)
.map_err(|_| InvalidTraceId::Uuid)
.and_then(Self::try_from)
}
}

impl fmt::Display for TraceId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0.as_simple())
impl TryFrom<Uuid> for TraceId {
type Error = InvalidTraceId;
fn try_from(uuid: Uuid) -> Result<Self, Self::Error> {
if uuid.is_nil() {
return Err(InvalidTraceId::Nil);
}
Ok(TraceId(uuid))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Serde nil IDs not substituted

Medium Severity

TryFrom/FromStr now reject all-zero trace IDs, and impl_str_serde deserializes through FromStr. Nil IDs in types like DynamicSamplingContext fail parsing instead of being replaced with a random ID, unlike the FromValue path that substitutes nil and records nil_trace_id.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4b98c24. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In a follow-up PR I will make parsing more restrictive and always reject on nil IDs. For now, an error in the DSC will hit the error boundary and default to None.

}
}

impl fmt::Debug for TraceId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "TraceId(\"{}\")", self.0.as_simple())
impl TryFrom<EventId> for TraceId {
type Error = InvalidTraceId;
fn try_from(event_id: EventId) -> Result<Self, Self::Error> {
Self::try_from(event_id.0)
}
}

impl From<Uuid> for TraceId {
fn from(uuid: Uuid) -> Self {
TraceId(uuid)
impl From<TraceId> for Uuid {
fn from(trace_id: TraceId) -> Self {
trace_id.0
}
}

impl From<EventId> for TraceId {
fn from(event_id: EventId) -> Self {
Self::from(event_id.0)
impl fmt::Display for TraceId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0.as_simple())
}
}

impl From<TraceId> for Uuid {
fn from(trace_id: TraceId) -> Self {
trace_id.0
impl fmt::Debug for TraceId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "TraceId(\"{}\")", self.0.as_simple())
Comment on lines +115 to +136

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I moved the constructors up so they are all in one place. Messes with the diff but makes for better readability over all IMO.

}
}

Expand All @@ -138,9 +151,14 @@ impl FromValue for TraceId {
Self: Sized,
{
match value {
Annotated(Some(Value::String(value)), mut meta) => match value.parse() {
Annotated(Some(Value::String(value)), mut meta) => match value.parse::<TraceId>() {
Ok(trace_id) => Annotated(Some(trace_id), meta),
Err(_) => {
Err(InvalidTraceId::Nil) => {
meta.add_remark(Remark::new(RemarkType::Substituted, "nil_trace_id"));
meta.set_original_value(Some(value));
Annotated(Some(TraceId::random()), meta)
}
Err(InvalidTraceId::Uuid) => {
meta.add_error(Error::invalid("not a valid trace id"));
meta.set_original_value(Some(value));
Annotated(None, meta)
Expand Down Expand Up @@ -411,18 +429,18 @@ mod tests {
assert_eq!(trace_id.as_u128(), 0x4c79f60c11214eb38604f4ae0781bfb2);

// Test empty string (should return 0)
let empty_trace_id: Result<TraceId, Error> = "".parse();
let empty_trace_id: Result<TraceId, _> = "".parse();
assert!(empty_trace_id.is_err());

// Test string with invalid length (should return 0)
let short_trace_id: Result<TraceId, Error> = "4c79f60c11214eb38604f4ae0781bfb".parse(); // 31 chars
let short_trace_id: Result<TraceId, _> = "4c79f60c11214eb38604f4ae0781bfb".parse(); // 31 chars
assert!(short_trace_id.is_err());

let long_trace_id: Result<TraceId, Error> = "4c79f60c11214eb38604f4ae0781bfb2a".parse(); // 33 chars
let long_trace_id: Result<TraceId, _> = "4c79f60c11214eb38604f4ae0781bfb2a".parse(); // 33 chars
assert!(long_trace_id.is_err());

// Test string with invalid hex characters (should return 0)
let invalid_trace_id: Result<TraceId, Error> = "4c79f60c11214eb38604f4ae0781bfbg".parse(); // 'g' is not a hex char
let invalid_trace_id: Result<TraceId, _> = "4c79f60c11214eb38604f4ae0781bfbg".parse(); // 'g' is not a hex char
assert!(invalid_trace_id.is_err());
}

Expand Down
35 changes: 35 additions & 0 deletions relay-event-schema/src/protocol/trace_metric/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,4 +225,39 @@ mod tests {
Some(&MetricUnit::Custom("customunit".parse().unwrap()))
);
}

#[test]
fn test_trace_metric_with_nil_uuid() {
let json = r#"{
"timestamp": 1544719860.0,
"trace_id": "00000000000000000000000000000000",
"name": "custom.metric",
"type": "counter",
"value": 42
}"#;

let data = Annotated::<TraceMetric>::from_json(json).unwrap();
let trace_metric = data.value().unwrap();

assert!(!trace_metric.trace_id.value().unwrap().is_nil());

insta::assert_debug_snapshot!(trace_metric.trace_id.meta(), @r#"
Meta {
remarks: [
Remark {
ty: Substituted,
rule_id: "nil_trace_id",
range: None,
},
],
errors: [],
original_length: None,
original_value: Some(
String(
"00000000000000000000000000000000",
),
),
}
"#);
}
}
4 changes: 2 additions & 2 deletions relay-sampling/tests/fixtures/dynamic_sampling_context.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"trace_id": "00000000-0000-0000-0000-000000000000",
"trace_id": "00000000-0000-0000-0000-000000000001",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Tests with nil IDs would now fail.

"public_key": "abd0f232775f45feab79864e580d160b",
"release": "1.0",
"environment": "dev",
Expand All @@ -8,4 +8,4 @@
"user_id": "some-id",
"user_segment": "all",
"user": null
}
}
21 changes: 18 additions & 3 deletions relay-server/src/processing/trace_metrics/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use crate::processing::trace_metrics::{
Error, ExpandedTraceMetrics, Result, SerializedTraceMetrics, get_calculated_byte_size,
};
use crate::services::outcome::DiscardReason;
use crate::statsd::RelayDistributions;
use crate::statsd::{RelayCounters, RelayDistributions};
use crate::utils::client_name_tag;

/// Validates that there are no duplicated trace metric containers.
///
Expand Down Expand Up @@ -58,23 +59,37 @@ pub fn size(metrics: &mut Managed<ExpandedTraceMetrics>, ctx: Context<'_>) {

/// Validates the semantic validity of a trace metric.
pub fn validate(metrics: &mut Managed<ExpandedTraceMetrics>) {
let client_name = client_name_tag(metrics.headers.meta().client_name());
metrics.retain(
|metrics| &mut metrics.metrics,
|metric, _| {
validate_trace_metric(metric).inspect_err(|err| {
validate_trace_metric(metric, client_name).inspect_err(|err| {
relay_log::debug!("failed to validate trace metric: {err}");
})
},
);
}

fn validate_trace_metric(metric: &Annotated<TraceMetric>) -> Result<()> {
fn validate_trace_metric(metric: &Annotated<TraceMetric>, client_name: &str) -> Result<()> {
match metric.value().and_then(|m| m.ty.value()) {
Some(MetricType::Gauge | MetricType::Distribution | MetricType::Counter) => {}
Some(MetricType::Unknown(_)) | None => {
return Err(Error::Invalid(DiscardReason::InvalidTraceMetric));
}
}

if let Some(trace_id_meta) = metric.value().map(|m| m.trace_id.meta())
&& trace_id_meta
.iter_remarks()
.find(|rem| rem.rule_id == "nil_trace_id")
.is_some()
{
relay_statsd::metric!(
counter(RelayCounters::TraceMetricNilTraceId) += 1,
sdk = client_name
);
}

Ok(())
}

Expand Down
4 changes: 2 additions & 2 deletions relay-server/src/services/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2108,7 +2108,7 @@ mod tests {
let mut envelope = Envelope::from_request(None, request_meta);

let dsc = r#"{
"trace_id": "00000000-0000-0000-0000-000000000000",
"trace_id": "00000000-0000-0000-0000-000000000001",
"public_key": "e12d836b15bb49d7bbf99e64295d995b",
"sample_rate": "0.2"
}"#;
Expand Down Expand Up @@ -2163,7 +2163,7 @@ mod tests {
"0.2",
),
"trace_id": String(
"00000000000000000000000000000000",
"00000000000000000000000000000001",
),
"transaction": ~,
},
Expand Down
6 changes: 6 additions & 0 deletions relay-server/src/statsd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,11 @@ pub enum RelayCounters {
UnrealEndpointExpansion,
/// The number of times that relay receives a compressed minidump.
CompressedMinidump,
/// The number of times a trace metric has a nil trace ID.
///
/// This metric is tagged with:
/// - `sdk`: low-cardinality client name
TraceMetricNilTraceId,
}

impl CounterMetric for RelayCounters {
Expand Down Expand Up @@ -1077,6 +1082,7 @@ impl CounterMetric for RelayCounters {
RelayCounters::ErrorProcessed => "event.error.processed",
RelayCounters::UnrealEndpointExpansion => "unreal.endpoint_expansion",
RelayCounters::CompressedMinidump => "minidump.compressed.count",
RelayCounters::TraceMetricNilTraceId => "trace_metric.nil_trace_id",
}
}
}
3 changes: 2 additions & 1 deletion relay-spans/src/otel_to_sentry_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,9 @@ fn otel_to_sentry_link(otel_link: OtelLink) -> Result<SpanV2Link, Error> {
Some((kv.key, Annotated::new(attr_value)))
}));

let trace_id = TraceId::try_from_or_random(otel_link.trace_id.as_slice());

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

try_from_or_random is used in other Otel converters, so I thought I might as well use it here.

Comment thread
sentry[bot] marked this conversation as resolved.
let span_link = SpanV2Link {
trace_id: Annotated::new(hex::encode(otel_link.trace_id).parse()?),
trace_id,
Comment thread
cursor[bot] marked this conversation as resolved.
span_id: SpanId::try_from(otel_link.span_id.as_slice())?.into(),
sampled: (otel_link.flags & W3C_TRACE_CONTEXT_SAMPLED != 0).into(),
attributes: Annotated::new(attributes),
Comment thread
jjbayer marked this conversation as resolved.
Expand Down
Loading