Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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 @@ -6,6 +6,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))
- Replace nil Trace IDs with random ones. ([#6079](https://github.com/getsentry/relay/pull/6079))

**Bug Fixes**:

Expand Down
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 UUID.
#[derive(Debug)]
pub enum InvalidTraceId {
/// The UUID is all zeros.
Nil,
/// Not a valid UUID.
Uuid,

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.

nit: Maybe no need to leak that there is an underlying uuid id as there are defined rules for trace ids which don't necessarily match a uuid. Could just call it 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)
.map(TraceId)
Comment thread
cursor[bot] marked this conversation as resolved.
Outdated

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.

Shouldn't this delegate to TryFrom<Uuid> other wise the nil check is missing?

}
}

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