Skip to content

feat(otel): scaffold OTel consistent probability sampling (ot.th/ot.rv) conformance tests - #7372

Open
genesor wants to merge 21 commits into
mainfrom
ben.db/feat-otel-tracestate-sampling-scaffolding
Open

feat(otel): scaffold OTel consistent probability sampling (ot.th/ot.rv) conformance tests#7372
genesor wants to merge 21 commits into
mainfrom
ben.db/feat-otel-tracestate-sampling-scaffolding

Conversation

@genesor

@genesor genesor commented Jul 23, 2026

Copy link
Copy Markdown
Member

Motivation

RFC for cross-tracer support of OTel consistent probability sampling (ot.th/ot.rv) tracestate members across all 9 dd-trace-* SDKs (APMAPI-2171 distributed tracing, APMAPI-2172 trace export).

Changes

Scaffolding only — every new test class is landed disabled for all 9 tracers via manifest entries (missing_feature). No tracer implements this yet.

  • tests/test_otel_tracestate_sampling.py (new): A1-A6 from APMAPI-2171
    • Test_EmitOtOnProbabilityDecision_Rate0_{01,1,2,5,99} — golden-vector rv/th derivation at 5 fixed rates, crossed with the 23 trace IDs from tests/fixtures/sampling_rates.csv, verified against the Go Knuth-hash sampler formula
    • Test_ForwardInboundOtUnchanged — inbound th/rv forwarded unchanged, never re-derived
    • Test_PreserveDdAndOtherVendors — ot= handling doesn't disturb dd= or unrelated vendor tracestate members
    • Test_ForceKeepClearsTh — a non-probability (force-keep, via ASM /waf) decision erases th but forwards an inherited rv
    • Test_SampledWithoutOtNotFabricated — an inherited sampling decision with no ot= must not fabricate th/rv
    • Test_MalformedOtHandling — malformed th/rv treated as absent, dd=/vendors preserved, trace never rejected
  • tests/otel/test_tracing_otlp.py: B1/B2 from APMAPI-2172 — Test_Otlp_Carries_Ot, Test_Otlp_Forwards_Inherited_Ot, asserting on the exported OTLP span's traceState
  • utils/_context/_scenarios/__init__.py: 4 new fixed-rate scenarios (otel_sampling_rate_0_{01,1,2,99}); rate 0.5 reuses the existing sampling scenario
  • manifests/*.yml (all 9 languages): missing_feature (APMAPI-2171) / (APMAPI-2172) entries for every new class

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

Reviewer checklist

  • Anything but tests/ or manifests/ is modified ? I have the approval from R&P team (note: utils/_context/_scenarios/__init__.py is modified to add the 4 new scenarios)
  • A docker base image is modified? N/A
  • A scenario is added, removed or renamed? Yes — 4 new scenarios added, needs R&P team review

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

CODEOWNERS have been resolved as:

tests/test_otel_tracestate_sampling.py                                  @DataDog/system-tests-core
.github/workflows/run-end-to-end.yml                                    @DataDog/system-tests-core
manifests/c.yml                                                         @DataDog/system-tests-core
manifests/cpp.yml                                                       @DataDog/dd-trace-cpp
manifests/cpp_httpd.yml                                                 @DataDog/dd-trace-cpp
manifests/cpp_kong.yml                                                  @DataDog/system-tests-core
manifests/cpp_nginx.yml                                                 @DataDog/dd-trace-cpp
manifests/dotnet.yml                                                    @DataDog/apm-dotnet @DataDog/asm-dotnet
manifests/golang.yml                                                    @DataDog/dd-trace-go-guild
manifests/java.yml                                                      @DataDog/asm-java @DataDog/apm-java
manifests/nodejs.yml                                                    @DataDog/dd-trace-js
manifests/php.yml                                                       @DataDog/apm-php @DataDog/asm-php
manifests/python.yml                                                    @DataDog/apm-python @DataDog/asm-python
manifests/ruby.yml                                                      @DataDog/ruby-guild @DataDog/asm-ruby
manifests/rust.yml                                                      @DataDog/apm-rust
tests/otel/test_tracing_otlp.py                                         @DataDog/system-tests-core
tests/parametric/test_headers_precedence.py                             @DataDog/system-tests-core @DataDog/apm-sdk-capabilities
utils/_context/_scenarios/__init__.py                                   @DataDog/system-tests-core

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Jul 23, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 84fa876 | Docs | Datadog PR Page | Give us feedback!

Comment thread tests/test_otel_tracestate_sampling.py Outdated
Comment thread tests/test_otel_tracestate_sampling.py
Comment thread tests/test_otel_tracestate_sampling.py
Comment thread tests/test_otel_tracestate_sampling.py

@vpellan vpellan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also believe that we should add tests for imprecision workaround while going from 64 to 56 bit (see the annex of the RFC about 64->56 bit imprecision)


@scenarios.otel_sampling_rate_0_1
@features.w3c_headers_injection_and_extraction
class Test_MalformedOtHandling:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these cases, as we generate new th and rv, should th be derived from the sampling rate, and rv from the trace id last 56 bits?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we do not do anything. We have no way of knowing how the decision was made upstream. We just to cleanup

Comment thread tests/test_otel_tracestate_sampling.py
@vpellan

vpellan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

There are a bunch of tests intests/parametric/test_headers_precedence.py that needs to be fixed, because they were checking that we had exactly 1 vendor in the tracestate which is now not true anymore, and I believe we should do it on this PR as well.
Basically in all these tests we need to change

assert len(tracestate_1_arr) == 1
assert tracestate_1_arr[0].startswith("dd=")

into

assert any(item.startswith("dd=") for item in tracestate_1_arr)

The test are:

  • test_headers_precedence_propagationstyle_tracecontext_datadog: Only part 1, with no headers
  • test_headers_precedence_propagationstyle_tracecontext: Part 1, with no headers, but also part 5, with Only Datadog headers, as the datadog headers should be ignored because DD_TRACE_PROPAGATION_STYLE is set to tracecontext only. I believe for 6 too, with an invalid tracecontext, we should generate a new trace id, which means we're the root span, and we should add ot to the tracestate
  • test_headers_precedence_propagationstyle_datadog_tracecontext: Only part 1, with no headers as well

@genesor

genesor commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

Good catch, fixed in 52e0d3a. Updated the 5 assertions across the 3 tests to assert any(item.startswith("dd=") for item in tracestate_arr).

Part 6 already falls into the "new Datadog span context" path, so the same fix covers it.

@genesor
genesor marked this pull request as ready for review July 29, 2026 13:03
@genesor
genesor requested review from a team as code owners July 29, 2026 13:03
@genesor
genesor requested review from avara1986, christophe-papazian, dromanol, jandro996, mhlidd and tabgok and removed request for a team July 29, 2026 13:03

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: efe7f14b08

ℹ️ 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".

Comment thread utils/_context/_scenarios/__init__.py
Comment thread utils/_context/_scenarios/__init__.py
Comment thread tests/otel/test_tracing_otlp.py
Comment thread tests/parametric/test_headers_precedence.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants