test(server): assert the caller's sampling decision governs (RFC 0039 slice 4) - #630
Merged
Conversation
… slice 4) Closes the last §5 criterion, so RFC 0039 moves to `green`. RFC0039.4 is test-only: RFC 0038 slice 4 already made the SDK resolve `OTEL_TRACES_SAMPLER`, so nothing in production changes. Two arms differing only in the sampled flag -- `-01` exports the query span into the caller's trace, `-00` exports nothing at all. They are mutually validating, which is what stops the negative arm being vacuous: had extraction failed outright, the `-00` span would be a root, `ParentBased` delegates roots to `AlwaysOn`, and it would export. Zero exported spans therefore proves extraction produced a valid-but-unsampled parent. Both arms also assert the response succeeds. Sampling is a telemetry decision and must never decide a request's outcome. The sampler is pinned explicitly rather than inherited from `SdkTracerProvider`'s default (`ParentBased(AlwaysOn)` today): a test should state the regime it asserts under. Two gaps found while checking that `green` is actually earned rather than approximately true: - Nothing asserted RFC0039.2 for the *ingest* path -- that an untraced batch is a fresh root with no parent. It was only inferable from "no traceparent was sent", which a bug inventing a parent would defeat. Added a third arm to the RFC0039.3 test. - `tests/it/main.rs` still described two harness-exempt binaries; slice 3 made it three. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRFC0039 coverage now validates an untraced ingest root, inbound parent-based sampling for HTTP queries, test-harness registration, and the RFC’s completed status. ChangesInbound trace propagation
Inbound sampling coverage
RFC completion status
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes RFC 0039’s final acceptance criterion (RFC0039.4) by adding integration tests that assert the caller’s sampling decision (via traceparent sampled flag) governs whether Ourios exports request spans, end-to-end. It also tightens RFC0039.2 coverage for the ingest path and updates the RFC’s status to green.
Changes:
- Add a new server integration test (
rfc0039_4_sampling) with two arms (-01exports,-00exports nothing) under an explicitly pinnedParentBased(AlwaysOn)sampler. - Strengthen ingest propagation coverage by adding an explicit “no traceparent → fresh root” arm to the existing ingest propagation test.
- Mark RFC 0039 as
greenand document the slice history; update the consolidateditharness docs and module list.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/rfcs/0039-inbound-trace-context-propagation.md | Updates RFC 0039 status to green and summarizes the slices/tests that satisfy §5. |
| crates/ourios-server/tests/it/rfc0039_4_sampling.rs | New integration test asserting sampled vs unsampled inbound parents control span export. |
| crates/ourios-server/tests/it/main.rs | Updates harness documentation (three exempt binaries) and wires in the new test module. |
| crates/ourios-ingester/tests/rfc0039_3_ingest_propagation.rs | Adds an explicit “no traceparent → fresh root span” assertion for ingest (RFC0039.2). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
RFC 0039 slice 4 — RFC0039.4: the caller's sampling decision governs, end to end. This closes the last §5 criterion, so RFC 0039 moves to
green.Test-only. RFC 0038 slice 4 already made the SDK resolve
OTEL_TRACES_SAMPLER(rather than a bespoke config knob), so no production behaviour changes here — what needed proving is Ourios's wiring: that the context extracted at the ingress actually reaches the sampler.Why the negative arm isn't vacuous
Two arms differing only in the sampled flag:
-01→ the query span exports, inside the caller's trace-00→ nothing exports at allAsserting "zero spans" is normally a weak test, since a broken harness passes it trivially. Here it's load-bearing, because of how
ParentBasedresolves: had extraction failed outright, the-00span would be a root,ParentBaseddelegates roots toAlwaysOn, and it would export. Zero exported spans therefore proves extraction produced a valid-but-unsampled parent. The two arms validate each other under one harness.Both arms also assert the response succeeds. Sampling is a telemetry decision and must never decide a request's outcome — a regression making extraction fallible would show up here.
The sampler is pinned explicitly rather than inherited from
SdkTracerProvider's default (ParentBased(AlwaysOn)today,provider.rs:93): a test should state the regime it asserts under, not silently depend on an upstream default.Lives in the consolidated
itharness — a scoped subscriber suffices, so no new binary (RFC0028).Two gaps found while checking
greenwas earnedClaiming green means every §5 criterion has an asserting test, so I walked them. Two things didn't hold up:
tests/it/main.rsstill said two harness-exempt binaries. Slice 3 made it three. Fixed, with the reason each is exempt.Criteria coverage (RFC 0039 §5, complete)
rfc0039_1_query_propagation.rs(query) +rfc0039_3_ingest_propagation.rs(gRPC + HTTP)rfc0039_3_ingest_propagation.rs, both transports past the spawnrfc0039_4_sampling.rs(this slice)rfc0039_1_query_propagation.rs+propagation.rsunit tests, both carriersrfc0039_6_mcp_propagation.rs(SERVER kind, remote parent, local tool parent)Invariants (CLAUDE.md §3)
None touched — no production code in this slice.
Verification
cargo fmt --all --check✅cargo clippy --all-targets --all-features -- -D warnings✅cargo nextest run --all-features✅ 1252 passed, 38 skippedmdbook build✅Next
RFC 0040 — the
ourios-df-otelcrate anddatafusion.operator.*spans. Two items remain open for the maintainer, neither blocking: the upstreamSpanKindgrammar fix (would retire four[sic]markers), and the redundantopentelemetrydev-dependency inourios-server.Summary by CodeRabbit
Observability
Documentation