Skip to content

test(intake): seed ATIF ingest spans relative to now to stay within TTL#587

Merged
SandyChapman merged 1 commit into
mainfrom
fix-flaky-atif-ingest-ttl/schapman
Jul 7, 2026
Merged

test(intake): seed ATIF ingest spans relative to now to stay within TTL#587
SandyChapman merged 1 commit into
mainfrom
fix-flaky-atif-ingest-ttl/schapman

Conversation

@SandyChapman

@SandyChapman SandyChapman commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

test_atif_ingest_accepts_example_trajectory_and_reconstructs_read_side_data intermittently fails assert 0 == 7 (read-side spans empty), and has been blocking the merge queue for unrelated PRs (e.g. #561, which passed only on a lucky re-queue).

Root cause

Two issues, both introduced by #580 (9d16a122c4) when it swapped the test's relative now() - timedelta(hours=2) seed for a fixed _BASE_TIME = datetime(2026, 1, 15) (µs=0):

  1. TTL time-bomb (the flakiness). The spans/traces ClickHouse tables carry TTL toDate(start_time) + INTERVAL 90 DAY (services/intake/src/nmp/intake/spans/clickhouse_migrations.py:142, :295). The fixed 2026-01-15 seed is now ~170+ days in the past — beyond the 90-day TTL — so the rows are eligible for eviction. TTL eviction runs on async background merges, so the read returns 7 or 0 depending on whether a merge fired first → flaky red.
  2. Whole-second formatting (latent). With a µs=0 base, the first step lands on a whole second; the span read-side returns ...:12 (no fractional part) while _span_started_at formats ...:12.000000, failing the exact-timestamp assertion. The pre-fix(intake): remove implicit 30-day lookback from span/trace lists; s… #580 now()-2h seed always had sub-second µs and avoided this.

Repro: same payload dated now()-2hsession_only=7, gte=7; dated 2026-01-150/0 (verified locally). Querying by session_id only (no time filter) is also 0, confirming it's storage-side TTL, not the removed list lookback.

Fix

Seed _BASE_TIME relative to now() (~45 days back) with a fixed .500000 µs offset:

  • ~45 days is past the (removed) 30-day list lookback this test exercises, yet safely inside the 90-day TTL — so it no longer rots.
  • .500000 µs ensures no derived timestamp lands on a whole second.

Everything derived from the base moves with it, so all reconstructed-timestamp assertions stay exact. Keeps #580's explicit-gte query behavior.

Testing

services/intake/tests/integration/spans/test_atif_ingest.py16 passed, on two consecutive runs (stable). ruff check / ruff format clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved timestamp handling so generated span times remain precise and consistent, including fractional microseconds.
    • Updated time-based validation to better reflect retention and TTL behavior, reducing the chance of flaky time-dependent results.
  • Tests

    • Strengthened integration coverage for span ingestion scenarios with deterministic, relative timestamps.

test_atif_ingest_accepts_example_trajectory_and_reconstructs_read_side_data
seeded spans at a fixed `_BASE_TIME = 2026-01-15`. The spans/traces ClickHouse
tables carry `TTL toDate(start_time) + INTERVAL 90 DAY`, so once that fixed date
is >90 days in the past the rows fall out of retention and the read returns
nothing (assert 0 == 7). Because TTL eviction runs on async background merges,
the failure is flaky — it passes only when no merge evicts the rows before the
read, which is why it intermittently blocked the merge queue.

Seed relative to `now()` (~45 days back): beyond the removed 30-day list
lookback that this test exercises, yet safely inside the 90-day TTL, and no
longer a fixed date that rots. Use a .500000 microsecond offset so no derived
timestamp lands on a whole second, which the span read-side returns without a
fractional part and would fail the exact-timestamp assertions (the pre-#580
`now()-2h` seed avoided this; the fixed µs=0 base introduced in #580 did not).

Full module passes repeatedly.

Signed-off-by: Sandy Chapman <[email protected]>
@SandyChapman
SandyChapman requested review from a team as code owners July 7, 2026 15:56
@github-actions github-actions Bot added the test conventional-commit type label Jul 7, 2026
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c7e61b55-2268-4304-b92e-53f6383eef6a

📥 Commits

Reviewing files that changed from the base of the PR and between 016ba5d and 8431ada.

📒 Files selected for processing (1)
  • services/intake/tests/integration/spans/test_atif_ingest.py

📝 Walkthrough

Walkthrough

Changed _BASE_TIME in an ATIF ingest integration test from a fixed calendar datetime to a value computed as current UTC time minus 45 days, with microseconds fixed at 500000, to align with ClickHouse TTL/retention behavior and preserve fractional timestamp assertions.

Changes

Test time basis update

Layer / File(s) Summary
_BASE_TIME dynamic computation
services/intake/tests/integration/spans/test_atif_ingest.py
_BASE_TIME now derived from now minus 45 days with .500000 microseconds, keeping timestamp-dependent assertions deterministic while respecting retention/TTL eviction and fractional-second formatting.

Possibly related PRs

  • NVIDIA-NeMo/nemo-platform#174: Also modifies test_atif_ingest.py's time basis to make timestamp-dependent ATIF ingest assertions deterministic.

Suggested reviewers: BrianNewsom, matthewgrossman

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: seeding ATIF ingest span test data relative to now to avoid TTL eviction.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-flaky-atif-ingest-ttl/schapman

Comment @coderabbitai help to get the list of available commands.

@SandyChapman
SandyChapman enabled auto-merge July 7, 2026 16:13
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 23272/30437 76.5% 61.2%
Integration Tests 13615/29117 46.8% 20.0%

@SandyChapman
SandyChapman added this pull request to the merge queue Jul 7, 2026
Merged via the queue into main with commit 64a8888 Jul 7, 2026
55 checks passed
@SandyChapman
SandyChapman deleted the fix-flaky-atif-ingest-ttl/schapman branch July 7, 2026 16:31
arpitsardhana pushed a commit that referenced this pull request Jul 9, 2026
…TL (#587)

test_atif_ingest_accepts_example_trajectory_and_reconstructs_read_side_data
seeded spans at a fixed `_BASE_TIME = 2026-01-15`. The spans/traces ClickHouse
tables carry `TTL toDate(start_time) + INTERVAL 90 DAY`, so once that fixed date
is >90 days in the past the rows fall out of retention and the read returns
nothing (assert 0 == 7). Because TTL eviction runs on async background merges,
the failure is flaky — it passes only when no merge evicts the rows before the
read, which is why it intermittently blocked the merge queue.

Seed relative to `now()` (~45 days back): beyond the removed 30-day list
lookback that this test exercises, yet safely inside the 90-day TTL, and no
longer a fixed date that rots. Use a .500000 microsecond offset so no derived
timestamp lands on a whole second, which the span read-side returns without a
fractional part and would fail the exact-timestamp assertions (the pre-#580
`now()-2h` seed avoided this; the fixed µs=0 base introduced in #580 did not).

Full module passes repeatedly.

Signed-off-by: Sandy Chapman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test conventional-commit type

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants