Skip to content

refactor: harden and generalise sampling integrity harness#236

Merged
andrewh merged 2 commits into
mainfrom
claude/issue-74-h7nvj5
Jul 8, 2026
Merged

refactor: harden and generalise sampling integrity harness#236
andrewh merged 2 commits into
mainfrom
claude/issue-74-h7nvj5

Conversation

@andrewh

@andrewh andrewh commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Follow-up to the merged issue-74 work (#235), applying an adversarial review of commit 54b4d0f that 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 real otelcol build with the sampler processors: the sampling suite, the conservation round-trip suite, make test, and make lint all pass.

Correctness and flakiness

  • Cross-draw contamination. Both reused-collector property tests (sampling integrity and conservation round-trip) now 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.
  • Deterministic test isolation. 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 compares full span-id sets (premise and keep/drop), not just counts.
  • Tail-sampling settle margin. 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 on a loaded machine.
  • Timeout signal. 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 — instead of the assertions living unexported in package synth test files. The always-on unit coverage moved with them to invariants_test.go. 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.

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 WaitSettled and 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

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

Copilot AI 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.

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 into pkg/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 WaitFor timeout).
  • Improves SupportsComponent by parsing otelcol components output as YAML, matching by component name, and caching per binary; adds TracesConfig to 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.

Comment thread pkg/synth/replay.go Outdated
Comment thread pkg/synth/replay.go Outdated
Comment thread pkg/pipelinetest/invariants.go
Comment thread pkg/pipelinetest/invariants.go Outdated
Comment thread pkg/pipelinetest/collector.go Outdated
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
andrewh merged commit 0c1c366 into main Jul 8, 2026
2 checks passed
@andrewh
andrewh deleted the claude/issue-74-h7nvj5 branch July 8, 2026 15:29
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]>
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.

3 participants