fix: make TraceState immutable #1597
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
Flarna
requested review from
dyladan,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
Spec requires that TraceState is immutable. Adapt TraceState API to return a TraceState for set/unset. Adapt implementation of set/unset to clone the TraceState before mutating and return the cloned TraceState.
Flarna
force-pushed
the
immuteable-tracestate
branch
from
0a0c5fe to
5e412ac
Compare
Codecov Report
@@ Coverage Diff @@
## master #1597 +/- ##
==========================================
+ Coverage 91.18% 91.19% +0.01%
==========================================
Files 164 164
Lines 5024 5033 +9
Branches 1026 1026
==========================================
+ Hits 4581 4590 +9
Misses 443 443
|
dyladan
approved these changes
Oct 15, 2020
I was also surprised. Seems noone is using it. This explains that it was not noticed that it is not spec conform. |
vmarchaud
approved these changes
Oct 21, 2020
mayurkale22
reviewed
Oct 21, 2020
| @@ -25,30 +25,30 @@ describe('TraceState', () => { | |||
| }); | |||
|
|
|||
| it('must replace keys and move them to the front', () => { | |||
There was a problem hiding this comment.
I believe we should update the description and mention that, "creates a new tracestate and replace..." WDYT?
mayurkale22
approved these changes
Oct 21, 2020
dyladan
pushed a commit
to dyladan/opentelemetry-js
that referenced
this pull request
Sep 9, 2022
dyladan
pushed a commit
to dyladan/opentelemetry-js
that referenced
this pull request
Sep 9, 2022
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Which problem is this PR solving?
Spec requires that TraceState is immutable.
Fixes: #1596
Short description of the changes
Adapt TraceState API to return a TraceState for set/unset.
Adapt implementation of set/unset to clone the TraceState before mutating and return the cloned TraceState.