Skip to content

fix(test): remove non-deterministic tiebreaks in otel snapshot ordering#9793

Open
aaronArinder wants to merge 1 commit into
devfrom
fix/otel-trace-snapshot-ordering-flake
Open

fix(test): remove non-deterministic tiebreaks in otel snapshot ordering#9793
aaronArinder wants to merge 1 commit into
devfrom
fix/otel-trace-snapshot-ordering-flake

Conversation

@aaronArinder

Copy link
Copy Markdown
Contributor

Summary

  • test_batch_trace_id failed intermittently in CI (insta snapshot mismatch, confirmed on amd_linux_test 2026-07-09) due to sort_spans_for_snapshot not fully canonicalizing span order.
  • Two independent, compounding bugs, both matching the "non-deterministic collection ordering in assertions" pattern:
    1. Root ordering leaked HashMap iteration order. Roots were collected via spans_by_root.keys().cloned().collect(). Rust's default hasher is randomly seeded per process, so whenever two roots tied on (start_time, end_time, name) — which test_batch_trace_id hits routinely, since its two batched operations produce two structurally-identical supergraph root spans that can start/end within the same nanosecond — the pre-sort order (and therefore the stable-sort tiebreak outcome) depended on hash-iteration order, not content.
    2. Sibling tiebreak used raw arrival order. The final tiebreak was original_position_within_parent — exactly the tokio-scheduling arrival sequence the function exists to cancel out.
  • Confirmed this is not a single-flush issue: get_traces already polls up to 10s for a report matching a shape filter, not reports[0], so that part was already handled correctly.

Fix

  • Kept (start_time_unix_nano, end_time_unix_nano) as the primary sort key, so non-tied siblings (the overwhelming common case) retain today's order and existing snapshots don't need to be regenerated.
  • Replaced only the tiebreak with a bottom-up content signature (name + own attributes with volatile keys stripped + children's signatures), instead of anything positional.
  • Root/child lookups are now a direct index scan + keyed lookup — no more wholesale HashMap iteration influencing order.
  • Unified the volatile-attribute list into a single VOLATILE_ATTRIBUTE_KEYS const shared with assert_report!'s redaction list, closing a drift risk between "redacted for display" and "excluded from sort key."

An earlier draft made the entire key content-based (no timestamps). That's a valid total order but reorders every non-tied sibling too, churning nearly every snapshot in the file — rejected in favor of the hybrid above.

Test plan

  • test_batch_trace_id alone: 40/40 passes
  • Full apollo_otel_traces suite (12 tests): 12/12 full-suite runs (144 individual executions), via cargo nextest per this file's serialization requirement
  • cargo clippy --test apollo_otel_traces and cargo fmt --check clean
  • No .snap files needed regeneration — the hybrid design preserves existing checked-in snapshots exactly

Test-only change; no production code touched.

🤖 Generated with Claude Code

sort_spans_for_snapshot had two independent sources of run-to-run
nondeterminism: root ordering came from spans_by_root.keys() straight
out of a HashMap (random hasher seed per process), and the sibling
tiebreak was raw batch-arrival position — exactly the tokio scheduling
noise this function exists to cancel out. Both only mattered when
spans tied on (start_time, end_time, name), which test_batch_trace_id
hits routinely since its two batched operations produce structurally
identical supergraph root spans.

Keep (start_time, end_time) as the primary sort key so non-tied
siblings retain today's order, and replace the tiebreak with a
bottom-up content signature (name + own attributes minus volatile
keys + children's signatures) instead of anything positional. The
volatile-attribute list is now a single VOLATILE_ATTRIBUTE_KEYS const
shared with assert_report!'s redaction list, so the two can't drift
apart.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@aaronArinder
aaronArinder requested a review from a team as a code owner July 9, 2026 20:51
@apollo-librarian

apollo-librarian Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 5addf779add524081cb1dd4c
Build Logs: View logs


✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@aaronArinder, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

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.

1 participant