Fix DSM pathway timestamp corruption: ZigZag-encode varints to match every other Datadog tracer - #6170
Conversation
…every other Datadog tracer Datadog::DataStreams::PathwayContext encoded pathway_start/edge_start as plain unsigned LEB128, but every other Datadog tracer that implements DSM (dd-trace-go, dd-trace-py, dd-trace-js, dd-trace-java via sketches-java, dd-trace-dotnet) ZigZag-maps these same fields before applying LEB128. Ruby was the sole outlier. Any non-Ruby DSM consumer decoding a pathway produced by this gem silently un-ZigZags a value that was never ZigZag-mapped, producing a garbage timestamp years to decades off from the true value -- no exception raised on either side. Verified end-to-end against the real, unmodified source of dd-trace-go, dd-trace-py, and sketches-java (the library dd-trace-java calls into) -- not reimplementations: all three produce byte-identical wire output to this fix for the same input, and all three independently decode a real Kafka message (produced through a real application's production Karafka/WaterDrop code path with this fix applied) to the correct timestamp. Also confirmed dd-trace-js's real encoding.js matches. Reverting the fix and re-decoding the same real Go-encoded payload reproduces the exact failure mode (a ~57-year-off date), matching the production symptom this was filed for. Added a regression test locking in wire compatibility against a real dd-trace-go-produced fixture.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a83ce7e3a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| # If high bit is clear, we're done | ||
| return value unless (byte & 0x80).nonzero? | ||
| return (value >> 1) ^ -(value & 1) unless (byte & 0x80).nonzero? |
There was a problem hiding this comment.
During a rolling upgrade, or when consuming queued messages whose dd-pathway-ctx-base64 header was produced by a previous dd-trace-rb release, this unconditional ZigZag decode misreads the old unsigned millisecond varints as signed ZigZag values; for example the legacy bytes for 1786000000123 ms decode to a 1941 timestamp, causing set_checkpoint to record decades of pathway/edge latency instead of preserving DSM continuity. Please keep a legacy decode path, version marker, or sanity fallback for inbound contexts before switching emitters to the new wire format.
AGENTS.md reference: AGENTS.md:L115-L120
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Fixes
Datadog::DataStreams::PathwayContextto ZigZag-encode/decode the two pathway timestamps (pathway_start,edge_start), matching every other Datadog tracer's wire format for DSM.Motivation
internal/datastreams.Encode/Decodeindd-trace-go(viasketches-go'sEncodeVarint64/DecodeVarint64) ZigZag-map these two fields before applying plain unsigned LEB128. This gem previously skipped the ZigZag step and used plain unsigned LEB128 directly. The bytes still parse successfully on the Go side —DecodeVarint64never errors — but the un-ZigZag step silently produces a garbage timestamp offset by years to decades from the true value, depending on the parity of the raw millisecond value.This isn't a "which side is canonical" ambiguity — checking every other Datadog tracer that implements DSM shows Ruby is the sole outlier:
dd-trace-go)sketches-go'sEncodeVarint64/DecodeVarint64)dd-trace-py)ddtrace/internal/datastreams/encoding.py:v >> 63 ^ (v << 1))dd-trace-js)packages/dd-trace/src/datastreams/encoding.js, explicitly documented as such)dd-trace-java)VarEncodingHelper.encodeSignedVarLong/decodeSignedVarLongviasketches-java)dd-trace-dotnet)VarEncodingHelper.WriteVarLongZigZag/ReadVarLongZigZag)dd-trace-rb)dd-trace-cpp)Net effect (pre-fix): any pathway where a Ruby producer using this gem feeds a Python, JavaScript, Java, .NET, or Go DSM consumer (or vice versa) shows wildly incorrect latency in Data Streams Monitoring -- no exception on either side, so it typically only surfaces as anomalous dashboard data. Ruby-to-Ruby pathways are unaffected since both ends share the same (previously missing) ZigZag step consistently.
Originally investigated and filed as DataDog/dd-trace-go#5163 against the Go side; re-filed here once the cross-SDK survey above made clear Ruby is the one that needs to change, not Go.
Verification
Tested end-to-end against the real, unmodified source of every other Datadog tracer that implements DSM (not reimplementations) -- Go, Python, Java (via
sketches-java, the librarydd-trace-javacalls into), and JavaScript:hash=424242, pathwayStart=1786000000123ms, edgeStart=1786000005456ms) with each SDK's own real, unmodified code produces byte-identical output on every one of them:MnkGAAAAAAD2kare+meg5are+mc=-- Go, Python, Java, JavaScript, and this fix all agree.Datadog::DataStreams::Processor/WaterDrop::Producercode, produced one real message to a real local Kafka broker, consumed the raw header back, then decoded that exact real header with Go's, Python's, and Java's own realDecode/decode_var_int_64/decodeSignedVarLongimplementations. All three independently decoded it to the same correct hash and timestamp, matching real time (not decades-off garbage).2083-03-11instead of the correct 2026 date (roughly double the true epoch-ms value, which is exactly what un-ZigZagging a plain-unsigned value produces for an even input). This matches the multi-decade-offset symptom reported in production DSM dashboards.(.NET's
dd-trace-dotnetwas confirmed via source reading only --VarEncodingHelper.WriteVarLongZigZag/ReadVarLongZigZagnaming is unambiguous -- no .NET runtime was available to execute it directly, but happy to if useful.)Added a regression test (
spec/datadog/data_streams/pathway_context_spec.rb) with a realdd-trace-go-produced base64 fixture, asserting both correct decode and byte-identical encode -- this locks in cross-SDK wire compatibility going forward rather than only testing this gem's internal round-trip (which the existing tests already covered and would not have caught this bug, since they only ever encode and decode with the same, symmetrically-broken implementation).All existing tests in
spec/datadog/data_streams/pathway_context_spec.rbcontinue to pass unmodified (8 examples), plus the 2 new regression tests (10/10 total).How to test the change?