Skip to content

Commit fbf8dd7

Browse files
committed
fix(traces): an empty attribute key spends no slot
The encoder drops an empty key, so charging it against the cap lost a real attribute. Scalars skip the truncation walk, event attributes go through the shared copier so a non-mapping logs, and the traceback test is named for what it asserts: the outermost exception survives the cut.
1 parent 0725004 commit fbf8dd7

5 files changed

Lines changed: 62 additions & 31 deletions

File tree

‎posthog/test/tracing/test_limits.py‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
1+
from unittest import mock
2+
13
import pytest
24

5+
from posthog.tracing import _limits as limits_module
36
from posthog.tracing._limits import (
47
bound_attributes,
58
truncate_attribute_value,
@@ -19,6 +22,13 @@ class TestTruncateAttributeValue:
1922
def test_truncates_a_long_string(self):
2023
assert truncate_attribute_value("x" * 40000, 8192) == "x" * 8192
2124

25+
def test_never_walks_a_scalar(self):
26+
with mock.patch.object(limits_module, "_truncate") as walk:
27+
assert truncate_attribute_value("short", 8) == "short"
28+
assert truncate_attribute_value(7, 8) == 7
29+
assert truncate_attribute_value(None, 8) is None
30+
assert not walk.called
31+
2232
def test_returns_a_short_string_unchanged(self):
2333
assert truncate_attribute_value("short", 8192) == "short"
2434

@@ -138,8 +148,10 @@ def test_bounds_each_value(self):
138148
attributes, _ = bound_attributes({"a": "x" * 20}, 2, 5)
139149
assert attributes == {"a": "xxxxx"}
140150

141-
def test_a_non_mapping_yields_nothing(self):
142-
assert bound_attributes(["a"], 2, 5) == ({}, 0)
151+
def test_an_empty_key_spends_no_slot_and_counts_no_drop(self):
152+
attributes, dropped = bound_attributes({"": 1, "b": 2, "c": 3}, 2, 8)
153+
assert attributes == {"b": 2, "c": 3}
154+
assert dropped == 0
143155

144156

145157
class TestTruncateAttributes:

‎posthog/test/tracing/test_span.py‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212

1313
from posthog.tracing import _span as span_module
1414
from posthog.tracing._config import MAX_ATTRIBUTES_PER_EVENT
15-
from posthog.tracing._otlp import SpanRecord, build_otlp_span
15+
from posthog.tracing._otlp import CIRCULAR_VALUE, SpanRecord, build_otlp_span
1616
from posthog.tracing._sanitize import (
1717
FALLBACK_SPAN_NAME,
1818
FUNCTION_VALUE,
@@ -793,14 +793,34 @@ def test_truncates_event_attributes_names_and_status_messages(self):
793793
assert record.events[0].attributes == {"k": "long"}
794794
assert record.status.message == "long"
795795

796-
def test_a_self_referencing_attribute_ends_the_span(self):
796+
def test_a_self_referencing_attribute_is_stored_as_the_encoders_marker(self):
797797
records: list = []
798798
value: dict = {}
799799
value["self"] = value
800800
span = make_span(records)
801801
span.set_attribute("loop", value)
802802
span.end()
803-
assert len(records) == 1
803+
assert records[0].attributes["loop"] == {"self": CIRCULAR_VALUE}
804+
805+
def test_an_empty_key_spends_no_slot(self):
806+
records: list = []
807+
span = make_span(records, max_attributes=1)
808+
span.set_attribute("", 1).set_attribute("real", 2)
809+
span.set_attributes({"": 3})
810+
span.add_event("e", {"": 1, "k": 2})
811+
span.end()
812+
assert records[0].attributes == {"real": 2}
813+
assert records[0].dropped_attributes_count == 0
814+
assert records[0].events[0].attributes == {"k": 2}
815+
816+
def test_add_event_with_a_non_mapping_logs_and_keeps_the_event(self, caplog):
817+
records: list = []
818+
span = make_span(records)
819+
with caplog.at_level("DEBUG", logger="posthog"):
820+
span.add_event("e", ["not", "a", "mapping"])
821+
span.end()
822+
assert records[0].events[0].attributes == {}
823+
assert "expected a mapping" in caplog.text
804824

805825

806826
class TestExceptionStacktrace:
@@ -823,9 +843,9 @@ def test_omits_the_stack_of_an_exception_that_was_never_raised(self):
823843
span.end()
824844
assert "exception.stacktrace" not in records[0].events[0].attributes
825845

826-
def test_bounds_the_stack_keeping_the_crash_site(self):
827-
# Python lists the most recent call last, so the tail is what matters:
828-
# the raising frame and the exception line.
846+
def test_bounds_the_stack_keeping_the_outermost_exception(self):
847+
# Python prints a cause first and the outermost exception last, so
848+
# the tail keeps the outermost raise and may cut the cause off.
829849
def recurse(depth):
830850
if depth == 0:
831851
raise ValueError("the actual crash site")

‎posthog/tracing/_limits.py‎

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ def truncate_attribute_value(value: Any, max_length: int) -> Any:
3636
3737
A value that cannot be walked is returned as it is.
3838
"""
39+
# The common case never allocates a walk.
40+
if isinstance(value, str):
41+
return truncate_string(value, max_length)
42+
if value is None or isinstance(value, (bool, int, float)):
43+
return value
3944
try:
4045
return _truncate(value, max_length, _WalkState(), 0)
4146
except Exception:
@@ -113,32 +118,20 @@ def _truncate_items(value: Any, max_length: int, state: _WalkState, depth: int)
113118

114119

115120
def bound_attributes(
116-
source: Any, max_count: int, max_length: int
121+
source: Mapping[str, Any], max_count: int, max_length: int
117122
) -> Tuple[Dict[str, Any], int]:
118-
"""A copy of ``source`` with at most ``max_count`` entries, each bounded, and
119-
how many entries the cap refused. A ``None`` value spends no slot."""
120-
if not isinstance(source, Mapping):
121-
return {}, 0
122-
try:
123-
keys = list(source.keys())
124-
except Exception:
125-
return {}, 0
123+
"""A copy of a ``copy_user_attributes`` result with at most ``max_count``
124+
entries, each bounded, and how many entries the cap refused. An empty key
125+
or a ``None`` value spends no slot."""
126126
attributes: Dict[str, Any] = {}
127127
dropped = 0
128-
for key in keys:
129-
key_str = attribute_key(key)
130-
if key_str is None:
131-
continue
132-
try:
133-
value = source[key]
134-
except Exception:
135-
value = UNSERIALIZABLE_VALUE
136-
if value is None:
128+
for key, value in source.items():
129+
if not key or value is None:
137130
continue
138-
if len(attributes) >= max_count and key_str not in attributes:
131+
if len(attributes) >= max_count and key not in attributes:
139132
dropped += 1
140133
continue
141-
attributes[key_str] = truncate_attribute_value(value, max_length)
134+
attributes[key] = truncate_attribute_value(value, max_length)
142135
return attributes, dropped
143136

144137

‎posthog/tracing/_span.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,8 @@ def _exception_event_attributes(
193193
"exception.type": exc_type,
194194
"exception.message": message,
195195
}
196-
# The tail is kept: Python lists the raising frame last.
196+
# The tail is kept: Python lists the raising frame last. For a chained
197+
# exception that is the outermost one; a cause printed above it goes first.
197198
stacktrace = describe_stacktrace(error)
198199
if stacktrace:
199200
attributes["exception.stacktrace"] = stacktrace[-max_length:]
@@ -277,6 +278,10 @@ def _mutable(self, operation: str) -> bool:
277278

278279
def _write_attribute(self, key: str, value: Any) -> None:
279280
"""Write an attribute unless the span is at its cap of distinct user keys."""
281+
if not key:
282+
# The encoder drops it, so it must not spend a slot.
283+
log.debug("Dropping an attribute with an empty key")
284+
return
280285
if value is None:
281286
# None removes the key, freeing its slot.
282287
if key in self._attributes and key not in self._auto_keys:
@@ -322,7 +327,7 @@ def add_event(
322327
dropped = 0
323328
if attributes is not None:
324329
bounded, dropped = bound_attributes(
325-
attributes,
330+
copy_user_attributes({}, attributes),
326331
MAX_ATTRIBUTES_PER_EVENT,
327332
self._max_attribute_value_length,
328333
)

‎posthog/tracing/span.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ def record_exception(self, exception: BaseException) -> "Span":
8484
Ignored after ``end()``. The event carries ``exception.type``,
8585
``exception.message`` and, for a raised exception,
8686
``exception.stacktrace`` (its last ``max_attribute_value_length``
87-
characters). Returns the span, so calls chain. Inside
87+
characters, so a chained traceback keeps the outermost exception and
88+
may lose its cause). Returns the span, so calls chain. Inside
8889
``with span:`` a raised ``Exception`` is recorded automatically, so
8990
this is for exceptions that are caught and handled.
9091

0 commit comments

Comments
 (0)