diff --git a/.changelog/5543.fixed b/.changelog/5543.fixed new file mode 100644 index 00000000000..ecbb79bd01c --- /dev/null +++ b/.changelog/5543.fixed @@ -0,0 +1 @@ +`opentelemetry-api`: fix `TraceState.update` dropping all entries when adding a new key at the 32-key limit diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index 745f4f5ff08..7d3462bebe8 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -321,13 +321,23 @@ def update(self, key: str, value: str) -> TraceState: Returns: A new TraceState with the modifications applied. - If the provided key-value pair is invalid or results in tracestate - that violates tracecontext specification, they are discarded and - same tracestate will be returned. + If the provided pair is invalid, or adding a new key would exceed + the maximum of 32 key/value pairs, they are discarded and the same + tracestate is returned unchanged. Updating an existing key is always + allowed, even at the maximum. """ if not _is_valid_pair(key, value): _logger.warning("Invalid key/value pair (%s, %s) found.", key, value) return self + # Adding a new key at the maximum would push the tracestate over the + # limit and cause the constructor to drop every entry. Return unchanged + # instead of silently discarding existing state. + if ( + key not in self._dict + and len(self._dict) >= _TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS + ): + _logger.warning("There can't be more 32 key/value pairs.") + return self prev_state = self._dict.copy() prev_state.pop(key, None) new_state = [(key, value), *prev_state.items()] diff --git a/opentelemetry-api/tests/trace/test_tracestate.py b/opentelemetry-api/tests/trace/test_tracestate.py index 555cef16bde..62e6f5fe29b 100644 --- a/opentelemetry-api/tests/trace/test_tracestate.py +++ b/opentelemetry-api/tests/trace/test_tracestate.py @@ -51,6 +51,33 @@ def test_tracestate_update_invalid(self): self.assertNotEqual(new_state.get("a"), ",,2,,f") self.assertEqual(new_state.get("a"), "1") + def test_tracestate_update_new_key_added_below_capacity(self): + # update() keeps its upsert behavior: a key that is not present is added + # as long as there is room below the 32-entry limit. + small_state = TraceState([("a", "1")]) + new_state = small_state.update("b", "2") + self.assertEqual(new_state.get("b"), "2") + self.assertEqual(new_state.get("a"), "1") + + def test_tracestate_update_at_capacity_new_key_preserved(self): + # Guards the previous bug: adding a new key at the 32-entry limit used to + # push the list to 33 entries and cause the constructor to wipe them all. + # Now the tracestate is returned unchanged, preserving existing entries. + pairs = [(f"key{i}", f"value{i}") for i in range(32)] + state = TraceState(pairs) + new_state = state.update("newkey", "newvalue") + self.assertEqual(len(new_state), 32) + self.assertIsNone(new_state.get("newkey")) + self.assertEqual(new_state.get("key0"), "value0") + + def test_tracestate_update_existing_key_at_capacity(self): + # Updating an existing key while at the limit stays within the limit. + pairs = [(f"key{i}", f"value{i}") for i in range(32)] + state = TraceState(pairs) + new_state = state.update("key0", "changed") + self.assertEqual(len(new_state), 32) + self.assertEqual(new_state.get("key0"), "changed") + def test_tracestate_delete_preserved(self): state = TraceState([("a", "1"), ("b", "2"), ("c", "3")]) new_state = state.delete("b")