feat(traces): W3C trace context ids and traceparent parsing - #949
Conversation
Prompt To Fix All With AI### Issue 1
posthog/tracing/_traceparent.py:117-121
**Malformed tracestate members accepted**
`sanitize_tracestate` only checks whether each nonempty member contains `=`, so it accepts empty members, invalid keys, empty values, multiple separators, and duplicate keys. When this value is propagated, downstream W3C implementations may discard the entire header and lose vendor state. Validate each member against the W3C key/value grammar, reject duplicates and empty members, and update the test at `posthog/test/tracing/test_traceparent.py:147-148` that currently requires empty-member preservation.
### Issue 2
posthog/tracing/_traceparent.py:13-15
**Invalid future extensions accepted**
The future-version pattern accepts any printable suffix, including invalid values such as `-extra`, spaces, or a dangling `-`. W3C future extensions are restricted to nonempty lowercase hexadecimal/hyphen fields, so malformed inbound headers can be continued and echoed instead of causing a new trace. Restrict the suffix to the future-version extension grammar and replace the tests at `posthog/test/tracing/test_traceparent.py:35-37` and `posthog/test/tracing/test_traceparent.py:98-102` with valid extension values.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(traces): W3C trace context ids and ..." | Re-trigger Greptile |
posthog-python Compliance ReportDate: 2026-09-17 03:42:54 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
Starts the posthog.tracing package with W3C Trace Context: random 16-byte trace ids and 8-byte span ids (never all zeros), and traceparent/tracestate parsing per the spec. A header with uppercase hex, version ff, or a version 00 header with trailing fields is invalid; a higher version is echoed whole. Only the sampled flag is kept. A tracestate with more than 32 members or non-printable characters is discarded, and one over 512 characters is trimmed by whole members. Not reachable from the client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkZAsCciW4PV8ZdcCHmAbA
`$` matches before a final newline, so the length check plus `match` accepted a 31-hex trace id or 15-hex span id followed by `\n`. Use `fullmatch` so the validators enforce their contract.
78a005d to
eafebf9
Compare
jzhu13
left a comment
There was a problem hiding this comment.
Reviewed against main. Tracing tests pass at the head, ruff and mypy are clean, and the traceparent parsing checks out against the W3C spec (version ff, trailing fields on version 00, all-zero ids, short higher-version headers, unknown flag bits). No blocking issues. Approving with a few non-blocking notes.
Non-blocking
posthog/tracing/_traceparent.py:90traceparent_header: an unusable explicit parent ends up handled three different ways downstream. A two-element header list becomes a child of the active span (PR 5), a garbage string starts a new root, andinert_spanreturnsNOOP_SPANeven when a pass-through span is active (PR 3). Worth picking one fallback here, documenting it, and aligning #951 and #953.posthog/tracing/_traceparent.py:106sanitize_tracestateis half-validation: it rejects members without=but forwards empty keys, uppercase keys,vendor=a=b, and duplicate keys. The spec allows pass-through, so either validate the full key/value grammar (OTel Python does) or treat the header as opaque printable ASCII and say so. The current=check protects nothing a strict downstream would not drop anyway.posthog/tracing/_ids.py:1docstring says every id is validated before it ships. Nothing in the stack validates at export; ids are valid by construction (generated here or accepted from a validated header). Reword, and drop theis_valid_*re-check inside the regex-matched parser except for the all-zero compare.TRACE_FLAGS_SAMPLEDis"01"here and0x01in #950's_otlp.py. Same name, two types, same package. Rename one.- Nits: the length-cap comment at
_traceparent.py:17talks about tracestate;_TRACESTATE_FORBIDDEN_REalso validates traceparent trailing fields; abytesheader value (raw ASGI scope) silently becomes "no parent".
Reviewed with Claude Code (Claude Fable 5.1). Behaviors above were checked by probe against this branch head.
Ids are valid by construction, so the parser keeps only the all-zero compare and the unused validators go. A bytes header value (a raw ASGI scope) is decoded rather than read as no parent. The length-cap comment and the printable-ASCII regex are named for what they cover.
|
Thanks for the review. Addressed in 205db65:
|
The parser accepted bytes but the pipeline gates an explicit parent on str first, so a raw ASGI header value started a new trace. The header helper now decodes it, and leaves undecodable bytes for the parser to reject.
💡 Motivation and Context
First slice of native tracing for the Python SDK. Starts the
posthog.tracingpackage with W3C Trace Context: random 16-byte trace ids and 8-byte span ids (never all zeros), plustraceparent/tracestateparsing per the spec. Invalid headers (uppercase hex, versionff, version00with trailing fields) are rejected, a higher version is echoed whole, and only the sampled flag is kept. Atracestatewith more than 32 members or non-printable characters is discarded; one over 512 characters is trimmed by whole members.Not reachable from the client yet.
Stack (PR 1 of 9, based on
main):traces/01-ids-traceparent← this PRtraces/02-otlp-encodingtraces/03-span-handlestraces/04-transporttraces/05-pipelinetraces/06-exporttraces/07-span-limitstraces/08-before-span-sendtraces/09-client-wiring💚 How did you test it?
Unit tests in
posthog/test/tracing/test_ids.pyandtest_traceparent.pycover id generation and every header validity rule above.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with Claude Code (Claude Opus 5) against the traces spec, one commit per slice so each PR reviews on its own. Rebased onto main and opened as a stacked draft in a later Claude Code session (Claude Fable 5.1).
🤖 Generated with Claude Code
https://claude.ai/code/session_012o7CtHLfcypjmXL7g9ZGRC