Fix TraceState.update to only update already existing keys - #5543
Fix TraceState.update to only update already existing keys#5543amdadulbari wants to merge 3 commits into
Conversation
264b58c to
7c471e5
Compare
7c471e5 to
a4d1fe8
Compare
xrmx
left a comment
xrmx
left a comment
There was a problem hiding this comment.
Nit in the changelog (please also update the PR title), thanks a lot!
a4d1fe8 to
5466132
Compare
amdadulbari
changed the title
Done, updated the changelog to your suggested wording and renamed the PR title to match. Thanks for the review! |
Pull request dashboard statusWaiting on maintainers · refreshed 2026-08-21 01:46 UTC Merge when ready. Status above doesn't look right?
|
TraceState.update() builds a new entry list and passes it to the constructor, which discards everything and returns an empty TraceState when the list exceeds the 32-key W3C limit. As a result, upserting a new key into a tracestate that already holds 32 entries silently wiped all 32 existing key/value pairs, contradicting update()'s documented contract that "the same tracestate will be returned". Guard the limit the way add() already does, but only for genuinely new keys, so updates to existing keys at capacity keep working. Add tests for both cases. Signed-off-by: Md. Amdadul Bari Imad <amdadulbari@gmail.com>
49865cc to
f597006
Compare
|
@amdadulbari We'll take care of rebasing, no need to do that yourself, thanks. |
| if not _is_valid_pair(key, value): | ||
| _logger.warning("Invalid key/value pair (%s, %s) found.", key, value) | ||
| return self | ||
| if key not in self._dict: |
There was a problem hiding this comment.
Isn't this a breaking change? It will affect samplers like this (although experimental). Would a safer fix be, preserve upsert behavior but return unchanged when a missing key would exceed 32 entries?
There was a problem hiding this comment.
Thanks for catching that, I kinda hoped that any internal users relying on upsert would have failing tests after this change, e.g. if I raise when updating a key that is not there I see no failures in api and sdk.
There was a problem hiding this comment.
Thanks for the reviews. I reworked this to the narrower, non-breaking fix you both pointed at:
update() keeps its existing upsert behavior - a new key is still added when there's room. The only change is that when the tracestate is already at the 32-key maximum, adding a new key now returns the tracestate unchanged instead of silently dropping all 32 entries. Updating an existing key still works even at capacity, so update()'s contract is unchanged and samplers relying on insert aren't affected.
On @xrmx 's point about raising vs. returning: I went with returning unchanged, to stay consistent with how add()/delete() and the invalid-pair path already behave (they return self rather than throw). Happy to switch to raising if you'd prefer that signal instead.
Added a test confirming new keys are still added below capacity, plus the existing test for the at-capacity case. Reverted the docstring back to describing the upsert behavior.
Address review feedback: instead of restricting update() to existing keys (a breaking change for samplers relying on upsert), keep upsert behavior and only return the tracestate unchanged when adding a *new* key would exceed the 32-key maximum. Fixes the silent drop of all entries at capacity.
6dbc06f to
c913c4e
Compare
3 participants
Description
TraceState.update()can silently discard every existing entry.update()builds a new list of entries and passes it to theTraceStateconstructor. When a tracestate already holds the W3C maximum of 32 members andupdate()is called with a new key, the new list has 33 entries; the constructor detectslen(entries) > 32, logs a warning, and returns an emptyTraceState. So upserting one new key into a full tracestate wipes all 32 existing key/value pairs.This also contradicts
update()'s own docstring, which states that if the pair "results in tracestate that violates tracecontext specification ... the same tracestate will be returned."The sibling
add()already guards this limit;update()did not. This change applies the same guard, but only when the key is genuinely new, so updating the value of an existing key while at capacity keeps working.Type of change
How Has This Been Tested?
Added two unit tests in
opentelemetry-api/tests/trace/test_tracestate.py:test_tracestate_update_at_capacity_new_key_preserved— updating with a new key at 32 entries preserves the existing 32 (fails before this change:len == 0).test_tracestate_update_at_capacity_existing_key— updating an existing key at capacity still applies and stays within the limit.python -m pytest opentelemetry-api/tests/tracepasses (55 tests);ruff check/ruff formatare clean on the changed files.Checklist
.changelog/).