tracer: Refactor span context's TraceID to be a full [16]byte #1822
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
ajgajg1134
changed the title
|
I verified the failing parametric test is also failing for shared/128 branch so is unrelated to this refactor (and seems like a known bug in W3C textmap parsing?) |
ajgajg1134
commented
Mar 23, 2023
| context.trace = parent.trace | ||
| context.origin = parent.origin | ||
| context.errors = parent.errors | ||
| parent.ForeachBaggageItem(func(k, v string) bool { | ||
| context.setBaggageItem(k, v) | ||
| return true | ||
| }) | ||
| } else if sharedinternal.BoolEnv("DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED", false) { |
There was a problem hiding this comment.
This happens in tracer.go StartSpan now
katiehockman
approved these changes
Mar 23, 2023
Co-authored-by: Katie Hockman <katie@hockman.dev>
… handle odd length strings
katiehockman
approved these changes
Mar 24, 2023
katiehockman
pushed a commit
that referenced
this pull request
Mar 27, 2023
* 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
pushed a commit
that referenced
this pull request
Mar 28, 2023
* 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>
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?
Refactor the spancontext
traceIDto be a[16]byteinstead of auint64. This is a marginal memory gain over storing the upper bits as a hex string at a slight cost of possibly needing to hex-encode more frequently.Motivation
It's confusing to have the trace id split up across fields and as different types, it led to odd looking code occasionally where some bits of the ID would need to be encoded or decoded for the use-case at hand. I believe this refactor is an improvement in readability, although admittedly this code is still a bit confusing. Notably
newContextas thespantype can't be changed as it's exactly marshalled to the agent in its current form.Describe how to test/QA your changes
The unit tests, system tests, parametric tests should sufficiently cover this refactor I believe, but given this touches a critical component of the tracer we may want to be extra diligent during release testing to ensure there's no breakages here.
Reviewer's Checklist
Triagemilestone is set.