Skip to content

[API] TraceState::Set overwrites an entry with the same key - #4586

Merged
dbarker merged 13 commits into
open-telemetry:mainfrom
dbarker:fix_trace_state_set
Sep 21, 2026
Merged

dbarker merged 13 commits into
open-telemetry:mainfrom
dbarker:fix_trace_state_set

Conversation

@dbarker

@dbarker dbarker commented Sep 16, 2026

Copy link
Copy Markdown
Member

Fixes #4583

Changes

  • Add a KeyValueProperties::HasKey method to prevent unneeded value copies
  • Update TraceState::Set to overwrite an existing entry if the key matches
  • Update TraceState::Delete to remove unnecessary kv value copies.
  • Update TraceState::FromHeader to discard duplicate keys.
  • Add unit tests coverage of the above

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.52%. Comparing base (f5aef4f) to head (e897e52).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4586      +/-   ##
==========================================
+ Coverage   86.51%   86.52%   +0.01%     
==========================================
  Files         525      525              
  Lines       20464    20475      +11     
==========================================
+ Hits        17702    17713      +11     
  Misses       2762     2762              
Files with missing lines Coverage Δ
api/include/opentelemetry/common/kv_properties.h 98.93% <100.00%> (+0.07%) ⬆️
api/include/opentelemetry/trace/trace_state.h 98.95% <100.00%> (+0.08%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbarker
dbarker marked this pull request as ready for review September 17, 2026 01:21
@dbarker
dbarker requested a review from a team as a code owner September 17, 2026 01:21
}
auto allocate_size = curr_size;
if (curr_size < kMaxKeyValuePairs)
const size_t curr_size = kv_properties_->Size();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change to Set is an observable behavior change but also a correctness change. Before if a key passed to Set was an existing key for an entry then a new entry would be added with the same key. This violates the spec that keys must be unique.

{
allocate_size -= 1;
}
const size_t curr_size = kv_properties_->Size();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change to delete is not an observable behavior change. The intent is to improve performance by replacing the GetValue call with HasKey to prevent unnecessary value copies.

@dbarker dbarker changed the title [API] TraceState::Set owerwrites an entry with the same key [API] TraceState::Set overwrites an entry with the same key Sep 17, 2026
Comment thread api/include/opentelemetry/trace/trace_state.h
Comment thread CHANGELOG.md Outdated
Comment thread api/include/opentelemetry/trace/trace_state.h

@marcalff marcalff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see a minor optimization comment.

Comment thread api/include/opentelemetry/trace/trace_state.h
@dbarker
dbarker enabled auto-merge (squash) September 20, 2026 23:33
@dbarker
dbarker merged commit 1525d6a into open-telemetry:main Sep 21, 2026
76 checks passed
@dbarker
dbarker deleted the fix_trace_state_set branch September 21, 2026 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API] TraceState::Set does not overwrite existing entries

4 participants