tracer: support 128-bit trace ids #1833
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarksComparing candidate commit d48d3ef in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 17 metrics, 0 unstable metrics. scenario:BenchmarkConcurrentTracing-24
|
ajgajg1134
previously approved these changes
Mar 28, 2023
There was a problem hiding this comment.
I focused my review on the non-test files and it all looks good to me, only 2 things we should probably address before merging:
- Investigate benchmark regression (this may be related to tweaks done in the benchmarking platform, I have a meeting tomorrow with R&P team to look deeper)
- Add
TraceID128Bytes() [16]bytetoSpanContextW3Cinterface (to be used by otel api)
Adds support for generating 128 bit trace ids and exposing them from the Datadog API via a new SpanContextW3C. Future PRs will propagate this id for distributed tracing.
Remove the WithTraceID128 API that was added to this feature branch (not currently in a dd-trace-go release or in main).
Support DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED to only log 128 bit trace id values when enabled.
Rather than generate a random 64 bits for the upper half of a 128-bit trace id, use the first 32 bits as a timestamp, and the remaining 32 bits padded with zeroes.
Supports 128-bit trace id propagation in B3 multi headers. Other carriers will be supported in future PRs, and TODOs have been added in the code where it is needed. Also fixes the issue where a spanContext needed to have an associated span to access and store 128-bit trace ids. It is now a separate field of the spanContext. Now, the _dd.p.tid tag in the Meta map (upper 64 bits of 128-bit trace ID) is populated from the spanContext upon span finish.
Small refactor to tests to make it easier to test with 128-bit trace ids for other carriers. These tests could probably be refactored even further to have less repetition, but for now, it's just extending them to support 128-bit trace ids.
Supports 128-bit trace propagation of the remaining carriers per the specification.
When DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED is false, only the lower-order 64-bits of the TraceId should be logged. When DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED is true, the format of the injected TraceId is conditional based on the higher-order 64 bits of the TraceId - specifically, when: zero - the injected TraceId will be its decimal string encoding (preserving the current behavior of 64-bit TraceIds) non-zero - the injected TraceId will be encoded as 32 lower-case hexadecimal characters with zero-padding as necessary
- guarantee that the propagating tags have been set before reading them during extraction - only set _dd.p.tid if the top 64 bits is not empty or zero - always set _dd.p.tid to 16 characters, with padding if needed
* WIP: more refactoring, still broken tests though * Even more bug fixes * Update ddtrace/tracer/spancontext.go * PR Comments: use strconv.ParseUint instead of hex.Decode since it can handle odd length strings Co-authored-by: Katie Hockman <katie@hockman.dev>
katiehockman
force-pushed
the
katiehockman/merge-branches
branch
from
e60ac66
to
f858a2f
Compare
ajgajg1134
previously approved these changes
Mar 28, 2023
dianashevchenko
approved these changes
Mar 30, 2023
eliottness
pushed a commit
that referenced
this pull request
Mar 31, 2023
eliottness
pushed a commit
that referenced
this pull request
Apr 5, 2023
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR merges all of the changes made to the shared/128-bit branch to support 128-bit trace id generation, propagation, and logging.
The main changes are the following:
SpanContextW3Cinterface type to theddtracepackage which supports a newTraceID128method._dd.p.tid, to propagate the upper 64 bits to the AgentDD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED, which is off by defaultDD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED, which is off by defaultMotivation
128 bit trace ids are generated and propagated by default for OpenTelemetry use cases
Describe how to test/QA your changes
Reviewer's Checklist
Triagemilestone is set.