Commit 2d5e7cb
fix(tracing): accept unknown sampling mechanisms in _dd.p.dm (#19337)
## Description
**Real-world repro:** Datadog Synthetics emits sampling mechanism `15`, which isn't in dd-trace-py's
local `SamplingMechanism` enum. Before this fix, that alone was enough to drop `_dd.p.dm`, mark the
trace `_dd.propagation_error: decoding_error`, and log a warning — for a perfectly well-formed value.
Fixes #19335.
`validate_sampling_decision()` validated the incoming `_dd.p.dm` propagation tag against an
allowlist derived from the **local** `SamplingMechanism` enum, so any mechanism id the installed
version didn't know about was treated as corrupt: the tag was deleted, `_dd.propagation_error:
decoding_error` was written to the context (and thus onto the chunk-root span), and a warning was
logged. Because the inbound sampling priority is already set, `TraceSamplingProcessor` skips the
sampler, so no replacement `_dd.p.dm` is generated — the trace segment permanently loses its
ingestion-reason attribution, and nothing is propagated downstream.
This replaces the membership check with a **syntax and range** check: accept `-N` where `N` is an
ASCII decimal integer in `0..255`, matching libdatadog's `u8` encoding. Genuinely malformed values
(`-`, `--1`, `-1.0`, `-1a`, out-of-range, the legacy service-hash form `934086a6-4`) keep the
existing `decoding_error` behaviour.
Why loosen it rather than add the missing id to the enum:
- **The propagation spec treats the mechanism as an opaque integer.** dd-trace-cpp documents this as
an explicit design requirement in
[`include/datadog/sampling_mechanism.h`](https://github.com/DataDog/dd-trace-cpp/blob/main/include/datadog/sampling_mechanism.h):
tracers that only decode locally-enumerated values make adding new values infeasible, so the
mechanism "is treated as just an integer when being deserialized or serialized". libdatadog's
`SamplingMechanism::from_str` accepts any value in `0..=255`.
- **dd-trace-py is the only tracer that validates the mechanism *value*.** Java validates syntax
only (`PTagsCodec.validateDecisionMakerTag`); Node, Go, .NET and Ruby apply generic tagset/charset
decoding; PHP and C++ don't inspect the tag on extract. Full per-tracer breakdown in #19335.
- **The current check couples the library to a registry it doesn't own.** Any component adopting a
newly registered mechanism silently breaks decision-maker propagation for every dd-trace-py
service downstream until those services upgrade. This is the third occurrence on this line:
#3797's single-digit regex broke once the enum passed 9, #13554 replaced it with the enum-derived
allowlist (#13516, `-11`), and #19335 is the same failure for `-15`.
- **dd-trace-py is already inconsistent about it.** The W3C `tracestate` extract path applies no
validation to `t.dm`, and `Span._set_sampling_decision_maker()` writes `"-%d" % mechanism` for any
integer, so the same value is accepted or rejected depending on which header carried it.
Implementation notes:
- The valid set is a precomputed `frozenset` of 256 short strings, so the hot-path cost is identical
to today's `in` check — no regex, no allocation, no `int()` parse. This also avoids
`str.isdigit()`, which is `True` for non-ASCII digits.
- An `AIDEV-NOTE:` anchor above the constant records why the check is deliberately loose, so this
doesn't get "tightened" back into an enum allowlist a fourth time.
- `SAMPLING_MECHANISM_CONSTANTS` is now unused but **deliberately kept**. Adding the rejected id to
it (`SAMPLING_MECHANISM_CONSTANTS.add("-15")`) is the workaround users are applying on released
versions, so removing the name would raise `AttributeError` at their startup on upgrade. Mutating
it is now a harmless no-op. Happy to drop it if you'd rather not carry it — `ddtrace/internal` has
no compatibility guarantee, so it's your call.
## Testing
- `tests/tracer/test_propagation.py::test_extract_dm` — trimmed to 6 non-overlapping cases: `-0`
and `-255` (boundaries), `-15` (unenumerated mid-range id, the real-world Synthetics/#19335
repro), and one case each for the distinct decoding-error modes (malformed syntax `-1a`,
out-of-range `-256`, legacy service-hash form `934086a6-4`). The previous 12-case list had 7
malformed-input variants that all asserted the same `decoding_error` outcome; this keeps the
meaningfully distinct failure modes without the redundant coverage. 6/6 pass.
- `tests/integration/test_sampling.py` left as-is (already a reasonable, non-overlapping list):
`test_malformed_sampling_mechanism` (parametrized over six malformed forms) and
`test_supported_sampling_mechanism` (regression guard — every enum value is `<= 255`).
- Run via `scripts/run-tests` in the `tracer` and `integration_testagent` venvs. Lint clean
(`fmt`, `style`, `typing`).
## Risks
Low, and one-directional: the change only **widens** what is accepted, so no value that propagated
before stops propagating.
- Behaviour change: well-formed mechanism ids outside the local enum are now forwarded instead of
being dropped, and no longer set `_dd.propagation_error: decoding_error`. That is the fix.
- The trade-off is that a well-formed but meaningless id in `0..255` supplied by an untrusted client
now propagates rather than being stripped. This matches every other tracer, and the value is
already opaque metadata — but flagging it explicitly since it's the one thing this loosens.
- No public API change. No configuration change. Sampling and retention are unaffected (the
priority travels in its own header).
- This also removes the spurious `log.warning("failed to decode _dd.p.dm: %r", value)` that
used to fire for every valid-but-unenumerated mechanism id, not just genuinely malformed ones.
In production that warning was noisy and misleading — it fired continuously for the Synthetics
`-15` case above even though nothing was actually corrupt.
## Additional Notes
- Reported in #19335, which includes the production repro, the per-tracer comparison table, and the
git archaeology. Prior rounds on this same line: #3797, #13516, #13554.
- **Backport:** on 3.19.x this also rejects `-13` (AI Guard), which *is* a registered mechanism, so
the case for `backport 3.19` looks stronger than for 4.x. I can't apply labels — please add them
if you agree, or tell me and I'll open manual backport PRs for `3.19` / `4.13`.
- **Out of scope:** the W3C `tracestate` extract path still applies no validation to `t.dm`. The
asymmetry is pre-existing and adding validation there would *increase* strictness, so I've left it
alone. Worth noting if the extractors are ever unified.
- **Separate from this PR:** mechanism `15` appears to be emitted by Datadog Synthetics but isn't in
the shared cross-tracer registry, libdatadog, or system-tests `dd_constants.py`. Probably worth
registering upstream regardless of this fix.
Co-authored-by: mabdinur <munir.abdinur@datadoghq.com>1 parent 0ed515e commit 2d5e7cb
4 files changed
Lines changed: 27 additions & 12 deletions
File tree
- ddtrace/internal
- releasenotes/notes
- tests
- integration
- tracer
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
45 | 51 | | |
46 | 52 | | |
47 | 53 | | |
| |||
77 | 83 | | |
78 | 84 | | |
79 | 85 | | |
80 | | - | |
| 86 | + | |
81 | 87 | | |
82 | 88 | | |
83 | 89 | | |
| |||
Lines changed: 7 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
57 | 57 | | |
58 | 58 | | |
59 | 59 | | |
60 | | - | |
| 60 | + | |
| 61 | + | |
61 | 62 | | |
62 | | - | |
| 63 | + | |
63 | 64 | | |
64 | 65 | | |
65 | 66 | | |
66 | 67 | | |
67 | | - | |
| 68 | + | |
68 | 69 | | |
69 | 70 | | |
70 | 71 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
935 | 935 | | |
936 | 936 | | |
937 | 937 | | |
938 | | - | |
939 | | - | |
940 | | - | |
941 | | - | |
942 | | - | |
943 | | - | |
944 | | - | |
945 | | - | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
| 942 | + | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
946 | 947 | | |
947 | 948 | | |
948 | 949 | | |
| |||
0 commit comments