feat(core)!: defer tracestate validation until needed #5499
Conversation
| traceState._internalState.set(key, value); | ||
| return traceState; | ||
|
|
||
| // TODO: here potentially we could remove key/values from other vendors |
There was a problem hiding this comment.
Left this comment to start a discussion. In #790 there is this concern about modifying other vendor keys.
Further, we allow arbitrary keys to be overwritten by calling traceState.set despite the fact that we should not be modifying other vendor keys.
There was a problem hiding this comment.
Hmm, the OTel Spec for this seems to be in-development at the moment.
It seems to me that since the spec is in development, and the we're implementing the stable part of the spec, we cannot make any "safe" assumption yet on what to disallow here. So I'd opt to push the requirement of OTel specification compliance down to the consumer of TraceState#set() and not validate that only the "ot" key is used.
There was a problem hiding this comment.
I think the question being asked is whether or not we should allow existing KVs to be overwritten. For example, if foo=bar exists already in tracestate, we allow a new value baz for key foo, so foo=bar is removed from wherever it existed in the tracestate, and a new foo=baz is added to the start. A non-foo example would be vendor1=value1,vendor2=value2,vendor3=value3 could get changed to vendor2=value9,vendor1=value1,vendor3=value3. EDIT: The test asserting this current behavior probably describes it better than me 😅 .
FWIW it looks like other languages allow this right now. Here is go's implementation and java's builder implementation.
With the spec still in development and the other languages allowing this today, I'd be inclined to leave the logic as-is, because it is still unclear whether or not that will change (or when).
There was a problem hiding this comment.
Thanks both of you for your feedback :) I'll leave the logic as-is
| const traceState = this._clone(); | ||
| traceState._internalState.delete(key); | ||
| return traceState; | ||
| // TODO: here potentially we could remove key/values from other vendors |
There was a problem hiding this comment.
Same here about modifying other vendor keys. In this case unsetting them
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5499 +/- ##
=======================================
Coverage 94.95% 94.95%
=======================================
Files 308 308
Lines 7928 7931 +3
Branches 1604 1607 +3
=======================================
+ Hits 7528 7531 +3
Misses 400 400
|
| assert.deepStrictEqual(state.get('a'), undefined); | ||
| assert.deepStrictEqual(state.serialize(), ''); | ||
| assert.deepStrictEqual(state.serialize(), raw); |
There was a problem hiding this comment.
My interpretation of #790 is that is that get should return only valid values but the state is not going to be parsed until set or unset is called.
david-luna
changed the title
…na/opentelemetry-js into 790-skip-trace-state-validation
pichlermarc
added
the
target:next-major-release
packages/opentelemetry-core/test/common/trace/tracestate.test.ts
Outdated
Show resolved
Hide resolved
| return agg; | ||
| }, []) | ||
| .join(LIST_MEMBERS_SEPARATOR); | ||
| return this._state; |
There was a problem hiding this comment.
🤔 Do we still want this to truncate here before returning this._state in case it exceeds max length already?
There was a problem hiding this comment.
Actually I guess we want to propagate unchanged, in which case we'll just need to update tests to match the new behavior (e.g. doesn't truncate when the list is too long, unless set is being called.)
There was a problem hiding this comment.
You can see the update on the tests here but I want your feedback on that.
When calling .get the state is parsed to get the value resulting in undefined if _state is too large. But .serialize return the state unchanged if no set/unset calls are made. My question is if this behaviour is what we want
|
while reviewing your comments I realised the current implementation of set is not taking in consideration the max length of the state while setting a new key. So we can surpass the length just by adding key/values. I guess this is not the behaviour we want. I'll add a test case for this. |
…na/opentelemetry-js into 790-skip-trace-state-validation
done in 8c3aa04 |
Which problem is this PR solving?
Updated the behavior of
TraceStateclass to avoid parsing and validation of members until needed. The initialrawStatewill be kept and returned upon serialization. Methods that modify a key (set, unset) will activate the parsing and pass only the valid members to the new TraceState returned.Fixes #790
Short description of the changes
TraceStateclassType of change
Please delete options that are not relevant.
How Has This Been Tested?
Ran the tests with the refactor. Some tests modified to check new behavior
Checklist: