-
Notifications
You must be signed in to change notification settings - Fork 42
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
[RUM-1830] 128 bit trace IDs #680
Conversation
6747dd4
to
2fa4a82
Compare
...ages/core/src/rum/instrumentation/resourceTracking/distributedTracing/distributedTracing.tsx
Outdated
Show resolved
Hide resolved
...rum/instrumentation/resourceTracking/distributedTracing/__tests__/distributedTracing.test.ts
Outdated
Show resolved
Hide resolved
...rum/instrumentation/resourceTracking/distributedTracing/__tests__/distributedTracing.test.ts
Outdated
Show resolved
Hide resolved
...rum/instrumentation/resourceTracking/distributedTracing/__tests__/distributedTracing.test.ts
Outdated
Show resolved
Hide resolved
...rum/instrumentation/resourceTracking/distributedTracing/__tests__/distributedTracing.test.ts
Outdated
Show resolved
Hide resolved
...ages/core/src/rum/instrumentation/resourceTracking/distributedTracing/distributedTracing.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
...rum/instrumentation/resourceTracking/distributedTracing/__tests__/distributedTracing.test.ts
Outdated
Show resolved
Hide resolved
...ages/core/src/rum/instrumentation/resourceTracking/distributedTracing/distributedTracing.tsx
Outdated
Show resolved
Hide resolved
...ages/core/src/rum/instrumentation/resourceTracking/distributedTracing/distributedTracing.tsx
Outdated
Show resolved
Hide resolved
...ages/core/src/rum/instrumentation/resourceTracking/distributedTracing/distributedTracing.tsx
Outdated
Show resolved
Hide resolved
...ages/core/src/rum/instrumentation/resourceTracking/distributedTracing/distributedTracing.tsx
Outdated
Show resolved
Hide resolved
7319839
to
4be127d
Compare
I have changed the implementation, here is how it works now: Separating Span ID and Trace IDThe It can only be created through 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 IdentifierTracingIdFormat (previously 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:
I will add a few more tests to match the RFC specs more accurately, and verify @ganeshnj's comment #680 (review). |
a069027
to
1995e82
Compare
Example of generated headers:
|
There was a problem hiding this 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.
packages/core/src/rum/instrumentation/resourceTracking/distributedTracing/TracingIdentifier.tsx
Outdated
Show resolved
Hide resolved
...rum/instrumentation/resourceTracking/distributedTracing/__tests__/distributedTracing.test.ts
Outdated
Show resolved
Hide resolved
26daf7c
to
aade66d
Compare
aade66d
to
85e8440
Compare
What does this PR do?
Introduces 128 bit trace IDs.
Review checklist (to be filled by reviewers)