fix(relay): Correctly serialize UUID without hypens by iambriccardo · Pull Request #4673 · getsentry/relay

iambriccardo · 2025-04-14T10:35:04Z

This PR fixes a problem where the trace_id was serialized differently in the _dsc and other parts of the event payload. The issue stemmed from the fact that Uuid is, by default, serialized with hyphens.

The solution is to use the TraceId type across the DSC and other parts of the code (relay-monitors) to ensure consistent implementation. The new TraceId is also using a Uuid inside even though the format is a subset of a uuid v4.

iambriccardo · 2025-04-14T13:09:02Z

relay-sampling/src/evaluation.rs

-fn pseudo_random_from_uuid(id: Uuid) -> f64 {
-    let big_seed = id.as_u128();
-    let mut generator = Pcg32::new((big_seed >> 64) as u64, big_seed as u64);
+fn pseudo_random_from_seed(seed: u128) -> f64 {


Decided to just remove our dependence from the Uuid and simply use u128.

This can stay as uuid now, since the trace id just wraps one now? That should make sure we don't accidentally mess up the seed

Yeah, I was thinking of keeping it since it makes the whole thing depend on a much simpler piece of code, but if you feel like reducing the surface for possible errors, I can revert.

iambriccardo · 2025-04-14T13:11:16Z

relay-event-schema/src/protocol/contexts/trace.rs

+    /// - The trace ID string is not exactly 32 characters
+    ///
+    /// Otherwise, it returns the trace ID as an u128 integer.
+    pub fn as_u128(&self) -> u128 {


This conversion assumes that for each byte we can take out a hex value from 0-15. This is not super efficient because we could technically represent the same data with 16 bytes instead of 32, but for the sake of simplicity (given the Empty, IntoValue and ProcessValue), we will keep the type as a String. We might want to make this more efficient in the future.

As discussed, probably worth switching all of this now to [u8; 16] like EventId

Dav1dde · 2025-04-14T14:23:49Z

relay-sampling/src/dsc.rs

        let jsons = [
            r#"{
-                "trace_id": "00000000-0000-0000-0000-000000000000",
+                "trace_id": "67e5504410b1426f9247bb680e5fe0c8",


Haven't taken a full look, so maybe it's there already, it would be good if we keep at least one test with the old format.

A trace id is not valid if it's made up of only zeros: https://www.w3.org/TR/trace-context/#trace-id.

I will keep some tests with the -.

Dav1dde · 2025-04-14T14:25:05Z

relay-monitors/src/lib.rs

-    #[serde(default, serialize_with = "uuid_simple")]
-    pub check_in_id: Uuid,
+    #[serde(default)]
+    pub check_in_id: TraceId,


I don't know if we can change that, check ins may actually use a uuid here.

Isn't out format compatible with a UUID? But yeah, I should use a different type name, it was my bad for keeping TraceId.

I am also fine to keep Uuid.

Oh it was not even serialized with hyphens before, right? Then it's safe to switch, nice. Maybe just make it a type alias to CheckInId, that should be fine.

Yeah, this is why it had the simple serializer. Because the simple variant has no -.

Dav1dde · 2025-04-14T14:25:47Z

relay-event-schema/src/protocol/contexts/trace.rs

+    /// - The trace ID string is not exactly 32 characters
+    ///
+    /// Otherwise, it returns the trace ID as an u128 integer.
+    pub fn as_u128(&self) -> u128 {


As discussed, probably worth switching all of this now to [u8; 16] like EventId

iambriccardo · 2025-04-16T06:03:44Z

relay-event-normalization/src/trimming.rs

    fn test_untrimmable_fields() {
        let original_description = "a".repeat(819163);
-        let original_trace_id = TraceId("b".repeat(48));
+        let original_trace_id = TraceId::parse_str(&"b".repeat(32)).unwrap();


We have to make this valid, otherwise it will not compile.

iambriccardo · 2025-04-16T06:15:02Z

relay-spans/src/span.rs

        )),
        timestamp: Timestamp(end_timestamp).into(),
-        trace_id: TraceId(trace_id).into(),
+        trace_id: TraceId::parse_str_annotated(&trace_id),


I decided to implement annotated in case of errors instead of using Result, so that we can have in the payload the errors.

My only worry is what will happen upstream in case no trace_id is set.

Dav1dde · 2025-04-16T07:31:37Z

relay-event-normalization/src/clock_drift.rs

                contexts.add(TraceContext {
-                    trace_id: Annotated::new(TraceId("4c79f60c11214eb38604f4ae0781bfb2".into())),
+                    trace_id: Annotated::new(
+                        TraceId::parse_str("3c79f60c11214eb38604f4ae0781bfb2").unwrap(),


Suggested change

TraceId::parse_str("3c79f60c11214eb38604f4ae0781bfb2").unwrap(),

"3c79f60c11214eb38604f4ae0781bfb2".parse().unwrap(),

Since it implements FromStr

Is there a particular reason to change the ID itself here?

Dav1dde · 2025-04-16T07:34:07Z

relay-event-schema/src/protocol/contexts/trace.rs

        match value {
-            Annotated(Some(Value::String(mut value)), mut meta) => {
-                if !is_hex_string(&value, 32) || value.bytes().all(|x| x == b'0') {
+            Annotated(Some(Value::String(value)), mut meta) => match FromStr::from_str(&value) {


Usually you'd just value.parse() here, instead of going through the trait

Dav1dde · 2025-04-16T07:38:35Z

relay-sampling/src/evaluation.rs

-fn pseudo_random_from_uuid(id: Uuid) -> f64 {
-    let big_seed = id.as_u128();
-    let mut generator = Pcg32::new((big_seed >> 64) as u64, big_seed as u64);
+fn pseudo_random_from_seed(seed: u128) -> f64 {


This can stay as uuid now, since the trace id just wraps one now? That should make sure we don't accidentally mess up the seed

loewenheim · 2025-04-16T07:39:57Z

relay-event-normalization/src/clock_drift.rs

                contexts.add(TraceContext {
-                    trace_id: Annotated::new(TraceId("4c79f60c11214eb38604f4ae0781bfb2".into())),
+                    trace_id: Annotated::new(
+                        TraceId::parse_str("3c79f60c11214eb38604f4ae0781bfb2").unwrap(),


Is there a particular reason to change the ID itself here?

loewenheim · 2025-04-16T07:49:34Z

relay-server/src/metrics_extraction/transactions/mod.rs

                ),
                trace_id: TraceId(
-                    "ff62a8b040f340bda5d830223def1d81",
+                    ff62a8b0-40f3-40bd-a5d8-30223def1d81,


Why are -s being introduced in these snapshots?

This is because we are now using a Uuid whose debug/display impl renders with -. This is the drawback of using the Uuid type instead of a custom type that does the validation.

We could overwrite the debug impl, I think I like that, then the snapshots would be matching.

Yep, I will do this! Good idea, so that it keeps being consistent, should we also use the ""? Might be unnecessary but idk

Oh, duh, I missed that this is Debug, not serialization. Still, I would also agree with overriding the Debug impl.

TraceId(abcd...) should be fine, don't really see a need for the " but nbd either way

iambriccardo · 2025-04-16T07:59:27Z

@loewenheim that was my bad, it was messed up during a codebase wide rename.

iambriccardo added 12 commits April 14, 2025 12:34

fix(relay): Correctly serialize UUID without hypens

ad83e30

Fix

6a34838

Fix

26350d3

Fix

a07248b

Improve

103d81c

Fix

cc75893

Fix

a0162be

Fix

225e6b6

Fix

e78ff79

Fix

68e133e

Merge

982fb34

Fix

faa681a

iambriccardo commented Apr 14, 2025

View reviewed changes

Update comment

e344873

Dav1dde reviewed Apr 14, 2025

View reviewed changes

iambriccardo added 7 commits April 15, 2025 14:57

Fix

1601802

Fix

4fc58a7

Fix

a6dd3f3

Fix

a1387b9

Fix

1d58c9b

Fix

4039f9b

Fix

4c15882

iambriccardo commented Apr 16, 2025

View reviewed changes

iambriccardo added 2 commits April 16, 2025 09:05

Fix

75bb0ed

Fix

9a56dc4

iambriccardo marked this pull request as ready for review April 16, 2025 07:29

iambriccardo requested a review from a team as a code owner April 16, 2025 07:29

Dav1dde reviewed Apr 16, 2025

View reviewed changes

loewenheim reviewed Apr 16, 2025

View reviewed changes

Fix

d805532

iambriccardo added 7 commits April 16, 2025 10:02

Fix

d0d7c97

Fix

7f676d2

Fix

a6dc002

Fix

88d3631

Fix

78ef8de

Fix

74925c2

Fix

bd5b3ea

iambriccardo requested review from Dav1dde and loewenheim April 16, 2025 09:36

Dav1dde approved these changes Apr 16, 2025

View reviewed changes

Update changelog

faf8246

loewenheim approved these changes Apr 16, 2025

View reviewed changes

iambriccardo added 2 commits April 16, 2025 12:55

Merge

a171b2b

Fix

901193c

iambriccardo enabled auto-merge (squash) April 16, 2025 11:06

iambriccardo merged commit bb5c0c9 into master Apr 16, 2025
31 checks passed

iambriccardo deleted the riccardo/fix/wrong-uuid branch April 16, 2025 11:09

This was referenced May 20, 2025

ref(protocol): Represent SpanId and TraceId as bytes #3535

Closed

Represent SpanId as bytes. #4751

Closed

	TraceId::parse_str("3c79f60c11214eb38604f4ae0781bfb2").unwrap(),
	"3c79f60c11214eb38604f4ae0781bfb2".parse().unwrap(),

Conversation

iambriccardo commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iambriccardo commented Apr 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iambriccardo commented Apr 14, 2025 •

edited

Loading