Skip to content
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.

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

[RUM-1830] 128 bit trace IDs #680

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

marco-saia-datadog
Copy link
Member

@marco-saia-datadog marco-saia-datadog commented Jun 25, 2024

What does this PR do?

Introduces 128 bit trace IDs.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-1830/128-bit-trace-ids branch from 6747dd4 to 2fa4a82 Compare June 25, 2024 13:38
@marco-saia-datadog marco-saia-datadog marked this pull request as ready for review June 25, 2024 13:40
@marco-saia-datadog marco-saia-datadog requested a review from a team as a code owner June 25, 2024 13:40
Copy link

@ganeshnj ganeshnj left a comment

Choose a reason for hiding this comment

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

Some suggestions, overall on right track.

One thing that I couldn't see how we are dealing with RUM, Trace or Log events with this update. I assume some encode/decode to be changed.

@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-1830/128-bit-trace-ids branch 3 times, most recently from 7319839 to 4be127d Compare June 28, 2024 15:15
@marco-saia-datadog
Copy link
Member Author

I have changed the implementation, here is how it works now:

Separating Span ID and Trace ID

The TracingIdentifier constructor is now private.

It can only be created through TracingIdentifier.createSpanId() or TracingIdentifier.createTraceId(), and it returns different types respectively.

Span IDs will always be generated as 64 bits BigInt(s) and Trace IDs as 128 bits BigInt(s)

String representation takes into account the type of Tracing Identifier

TracingIdFormat (previously TracingIdRepresentation) is now more generic, and the .toString implementation has been modified accordingly.

When requesting a padded HEX value, the bit length of the current type will be taken into account.

Requesting the low bits of a 128 bit Trace ID will get you a 64 bit ID, while requesting the low bits of a 64 bit Span ID will get you a 32 bit ID.

Here is a runnable example.

Here is an example output for reference:

FORMAT VALUE
Trace ID FULL HEX 667ed836000000000d8dbaedc8adcf59
Trace ID PADDED FULL HEX: 667ed836000000000d8dbaedc8adcf59
Trace ID HIGH HEX 667ed83600000000
Trace ID PADDED HIGH HEX: 667ed83600000000
Trace ID LOW HEX d8dbaedc8adcf59
Trace ID PADDED LOW HEX: 0d8dbaedc8adcf59
Span ID FULL HEX 378ad79b0519da05
Span ID PADDED FULL HEX 378ad79b0519da05
Span ID HIGH HEX 378ad79b
Span ID PADDED HIGH HEX 378ad79b
Span ID LOW HEX 519da05
Span ID PADDED LOW HEX 0519da05

I will add a few more tests to match the RFC specs more accurately, and verify @ganeshnj's comment #680 (review).

@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-1830/128-bit-trace-ids branch 3 times, most recently from a069027 to 1995e82 Compare July 1, 2024 13:19
@marco-saia-datadog
Copy link
Member Author

Example of generated headers:

Header Value
x-datadog-origin rum
x-datadog-sampling-priority 1
x-datadog-trace-id 17731188736533748218
x-datadog-parent-id 17034905968583185706
x-datadog-tags _dd.p.tid=6682b2ee00000000
traceparent 00-6682b2ee00000000f611d636468059fa-ec6824b93f95152a-01
tracestate dd=s:1;o:rum;p:ec6824b93f95152a
b3 6682b2ee00000000f611d636468059fa-ec6824b93f95152a-1
X-B3-TraceId 6682b2ee00000000f611d636468059fa
X-B3-SpanId ec6824b93f95152a
X-B3-Sampled 1

Copy link

@ganeshnj ganeshnj left a comment

Choose a reason for hiding this comment

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

Nice updates, just a few improvement comments but looks good to me.

@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-1830/128-bit-trace-ids branch 2 times, most recently from 26daf7c to aade66d Compare July 4, 2024 10:55
@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-1830/128-bit-trace-ids branch from aade66d to 85e8440 Compare July 4, 2024 11:27
@marco-saia-datadog marco-saia-datadog merged commit 3cff869 into develop Jul 4, 2024
6 checks passed
@marco-saia-datadog marco-saia-datadog deleted the marcosaia/RUM-1830/128-bit-trace-ids branch July 4, 2024 11:46
@marco-saia-datadog marco-saia-datadog mentioned this pull request Jul 4, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants