refactor: harden and generalise sampling integrity harness#236
Merged
Conversation
Follow-up to the merged issue-74 work, applying an adversarial review of commit 54b4d0f. Fixes two flake windows in the timing design, moves the invariant checks to the right altitude, and clears the cleanup findings. Correctness and flakiness: - Reworked both reused-collector property tests (sampling integrity and conservation round-trip) to accumulate sent identities instead of resetting the sink between draws. A reset races an in-flight export from the previous draw; a late span could land after the reset and be misattributed to the next draw, failing CheckNoFabrication as a load-dependent flake. Over a running set, a late span is still a span that was sent. - TestSampling_DeterministicDecisions now runs each of its two identical passes against its own collector and sink, so a late export from the first pass cannot bleed into the second. It also compares full span-id sets (premise and keep/drop), not just counts. - Raised tailSettleIdle from 2s to 4s: the tail sampler's release latency is decision_wait (1s) plus its internal policy tick (~1s), so 2s left no margin for scheduler jitter and could truncate the capture. - waitForSpans now fails the test on a WaitFor timeout instead of silently returning a short read, so a hang is distinguishable from span loss. Altitude and reuse: - Moved the invariant checks into pkg/pipelinetest as exported, framework-agnostic functions returning errors: CheckNoFabrication, CheckParentsKept, CheckWholeTraces, plus CheckConservation for the round-trip tests, keyed by SpanKey and a Sent accumulator. External users can now compose their own pipeline tests on the harness, which the design doc promised; the always-on unit coverage moved with them to invariants_test.go. The synth test files now assert through this API. CheckWholeTraces derives trace grouping straight from received spans rather than re-parsing composite keys, and no caller rebuilds a received-key map the check also builds. - Added pipelinetest.TracesConfig, a shared config skeleton parameterised by the processors block. The four hand-copied collector configs (pass-through, and the three samplers) now derive from it. - Extracted randomTraceID/randomSpanID in replay.go over a next func() uint64 source; the replay generator and the test's seeded generator share the validity-retry loop instead of duplicating it. Robustness: - SupportsComponent now parses the collector's `components` output as YAML and matches component names only (not module paths), caching the set per binary path instead of spawning the binary and recompiling a regexp on every call. Docs: the how-to's worked example now uses the exported pipelinetest API and synth.GenerateTraces, so it is composable outside package synth; config-building and tail-timing notes updated to match. Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_017mb8JHoaMXNfA6f7sncP4A
There was a problem hiding this comment.
Pull request overview
This PR hardens and generalises the OTLP pipeline sampling integrity harness by reducing flake windows in the collector/sink timing model, lifting invariant checks into pkg/pipelinetest as reusable exported APIs, and improving collector feature detection for optional components.
Changes:
- Extracts exported invariant checks (
CheckNoFabrication,CheckParentsKept,CheckWholeTraces,CheckConservation) plus supporting types intopkg/pipelinetest, and updates sampling/round-trip tests to use them. - Refactors pipeline tests to avoid cross-draw contamination and improves settle/timeout behaviour (including increased tail-sampler idle margin and fail-fast on
WaitFortimeout). - Improves
SupportsComponentby parsingotelcol componentsoutput as YAML, matching by component name, and caching per binary; addsTracesConfigto share a collector-config skeleton.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/synth/replay.go | Refactors random trace/span ID generation to share a validity-retry loop with seeded generators. |
| pkg/synth/pipeline_test.go | Updates round-trip tests to use pipelinetest conservation checks and safer span waiting behaviour. |
| pkg/synth/pipeline_sampling_test.go | Updates sampling invariant tests to use exported checks, improves deterministic replay isolation, and tunes tail settle timing. |
| pkg/pipelinetest/invariants.go | Adds exported invariant-checking API and Sent accumulator for comparing sent vs received spans. |
| pkg/pipelinetest/invariants_test.go | Adds unit tests for invariant failure paths without requiring a collector binary. |
| pkg/pipelinetest/collector.go | Improves component detection (YAML parsing + caching) and adds TracesConfig config-skeleton helper. |
| docs/how-to/test-sampling-integrity.md | Updates the how-to to use the exported pipelinetest API and shared config builder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on the sampling integrity harness: - randomTraceID/randomSpanID now lay out their bytes with a fixed BigEndian order instead of NativeEndian, so a seeded ID source yields the same identities on any architecture. - componentNames releases the cache mutex while it runs the collector `components` subprocess and unmarshals its output, re-taking the lock only to store, so a slow probe no longer blocks concurrent callers. The shared set is documented read-only. - CheckNoFabrication and CheckConservation doc comments now state that they compare identity sets, not exactly-once counts: OTLP delivery is at-least-once, so a correct pipeline may redeliver a span on retry. Treating redelivery as a violation would flake against real collectors, so the set-based semantics are deliberate; only a missing or fabricated identity fails the check. Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_017mb8JHoaMXNfA6f7sncP4A
andrewh
added a commit
that referenced
this pull request
Jul 8, 2026
Back-fill the 0.9.1, 0.9.2, and 0.10.0 sections that were released via GoReleaser but never recorded in CHANGELOG.md, and move already-shipped work out of [Unreleased]: replay mode (#209, 0.10.0), PRODUCER span kind (#214, 0.9.2), and reader-based replay entry points (#216, 0.9.2). Promote the genuinely-unreleased work to [0.11.0]: the INTERNAL span-kind change (#213/#231), the public pkg/synth trace-generation API (#199/#234), and the sampling trace-integrity invariants (#74/#235/#236), plus the OTLP exporter dependency bumps (#228, #229). Note that 0.9.2 was tagged after 0.10.0 from a later commit, so the next minor release is 0.11.0, and fix the compare links. Claude-Session: https://claude.ai/code/session_01G1HxLtj1Uu2Y3K6Ea5P9zR Co-authored-by: Claude <[email protected]>
This was referenced Jul 14, 2026
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.
Follow-up to the merged issue-74 work (#235), applying an adversarial review of commit
54b4d0fthat was skipped before merge. Fixes two flake windows in the timing design, moves the invariant checks to the right altitude, and clears the cleanup findings. Verified against a realotelcolbuild with the sampler processors: the sampling suite, the conservation round-trip suite,make test, andmake lintall pass.Correctness and flakiness
CheckNoFabricationas a load-dependent flake. Over a running set, a late span is still a span that was sent.TestSampling_DeterministicDecisionsnow runs each of its two identical passes against its own collector and sink, so a late export from the first pass cannot bleed into the second. It compares full span-id sets (premise and keep/drop), not just counts.tailSettleIdlefrom 2s to 4s: the tail sampler's release latency isdecision_wait(1s) plus its internal policy tick (~1s), so 2s left no margin for scheduler jitter and could truncate the capture on a loaded machine.waitForSpansnow fails the test on aWaitFortimeout instead of silently returning a short read, so a hang is distinguishable from span loss.Altitude and reuse
pkg/pipelinetestas exported, framework-agnostic functions returning errors:CheckNoFabrication,CheckParentsKept,CheckWholeTraces, plusCheckConservationfor the round-trip tests, keyed bySpanKeyand aSentaccumulator. External users can now compose their own pipeline tests on the harness — which the design doc promised — instead of the assertions living unexported inpackage synthtest files. The always-on unit coverage moved with them toinvariants_test.go.CheckWholeTracesderives trace grouping straight from received spans rather than re-parsing composite keys, and no caller rebuilds a received-key map the check also builds.pipelinetest.TracesConfig, a shared config skeleton parameterised by the processors block. The four hand-copied collector configs (pass-through, and the three samplers) now derive from it.randomTraceID/randomSpanIDinreplay.goover anext func() uint64source; the replay generator and the test's seeded generator share the validity-retry loop instead of duplicating it.Robustness
SupportsComponentnow parses the collector'scomponentsoutput as YAML and matches component names only (not module paths), caching the set per binary path instead of spawning the binary and recompiling a regexp on every call.Docs
The how-to's worked example now uses the exported
pipelinetestAPI andsynth.GenerateTraces, so it is composable outsidepackage synth; config-building and tail-timing notes updated to match.Known limitation
The residual intra-draw stall risk (a span delayed mid-delivery past the idle window) is inherent to a settle-based wait and now documented on
WaitSettledand in the how-to. It is far rarer than the cross-draw case fixed here, since the head sampler decides synchronously with no batching.🤖 Generated with Claude Code
Generated by Claude Code