Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
38 changes: 27 additions & 11 deletions relay-event-schema/src/protocol/contexts/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,29 @@ 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::parse_str(s).map_err(|_| InvalidTraceId::Uuid)?;
if uuid.is_nil() {
return Err(InvalidTraceId::Nil);
}
Ok(TraceId::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.
Expand Down Expand Up @@ -138,9 +149,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 +427,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
}
}
22 changes: 19 additions & 3 deletions relay-server/src/processing/trace_metrics/validate.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
use relay_event_schema::protocol::{MetricType, TraceMetric};
use relay_protocol::Annotated;

use crate::envelope::ClientName;
use crate::managed::Managed;
use crate::processing::Context;
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 +60,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
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