Skip to content

fix(intake): bound trace hydration by page timestamps - #1039

Merged
BrianNewsom merged 3 commits into
mainfrom
fix-trace-hydration-primary-key/brnewsom
Aug 3, 2026
Merged

fix(intake): bound trace hydration by page timestamps#1039
BrianNewsom merged 3 commits into
mainfrom
fix-trace-hydration-primary-key/brnewsom

Conversation

@BrianNewsom

@BrianNewsom BrianNewsom commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • carry each selected trace root timestamp through the page reference
  • bound second-phase trace_index hydration by the page timestamp range before FINAL
  • preserve the flat trace-id bloom predicate and source-format identity predicate

Production profile

For the reported 50-row page:

  • deployed hydration: 7.6s, 13.09 GiB read, 28.65 GiB memory, MEMORY_LIMIT_EXCEEDED
  • same hydration with the timestamp bound: 112ms, 62.89 MiB read, 59.77 MiB memory, all 50 rows returned

Count and page selection were already fast at 53ms and 90ms respectively.

Validation

  • 12 focused trace repository tests passed
  • 2 ClickHouse trace-read integration tests passed
  • Ruff check and format check passed
  • ty check passed

Summary by CodeRabbit

  • Bug Fixes
    • Improved trace retrieval accuracy by incorporating precise start-time ranges when loading trace details.
    • Ensured trace timestamps are handled consistently across timezone-aware and timezone-naive values.
    • Updated preview results to preserve microsecond-level timestamp precision.

Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
@github-actions github-actions Bot added the fix label Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 30234/38242 79.1% 63.7%
Integration Tests 17880/36911 48.4% 20.9%

@BrianNewsom
BrianNewsom marked this pull request as ready for review August 3, 2026 16:46
@BrianNewsom
BrianNewsom requested review from a team as code owners August 3, 2026 16:46
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Trace page references now retain Unix-microsecond start times. Hydration queries apply inclusive minimum and maximum start-time bounds. Hydration reconciliation matches source-format and trace-ID keys. Tests cover the updated query parameters and row schemas.

Changes

Trace hydration timestamp bounds

Layer / File(s) Summary
Timestamp propagation and reconciliation
services/intake/src/nmp/intake/repository/clickhouse/trace.py
_TracePageRef stores started_at_us. Page parameters include timestamp bounds. Hydrated rows match by (source_format, trace_id).
Bounded root hydration
services/intake/src/nmp/intake/repository/clickhouse/trace.py, services/intake/tests/test_traces_clickhouse_repository.py
Hydration filters root_started_at with inclusive page bounds. Tests cover microsecond timestamps, query selection, row schemas, and hydration misses.

Possibly related PRs

Suggested reviewers: asutermo, shanaiabuggy

🚥 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 and concisely describes the main change: bounding trace hydration by the selected page's timestamps.
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-trace-hydration-primary-key/brnewsom

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

Comment thread services/intake/src/nmp/intake/repository/clickhouse/trace.py Outdated
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
@BrianNewsom
BrianNewsom enabled auto-merge August 3, 2026 18:49

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
services/intake/tests/test_traces_clickhouse_repository.py (1)

133-140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test a non-empty timestamp range.

The fixture contains one page row, so the minimum and maximum bounds are always equal. Add a second row with a different started_at_us value and assert distinct page_started_at_min_us and page_started_at_max_us values.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/intake/tests/test_traces_clickhouse_repository.py` around lines 133
- 140, Extend the fixture in the test using _Client and _QueryResult with a
second page row whose started_at_us differs from the existing page_row value.
Update the assertions for page_started_at_min_us and page_started_at_max_us to
verify the distinct lower and upper timestamps produced by the non-empty range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@services/intake/tests/test_traces_clickhouse_repository.py`:
- Around line 133-140: Extend the fixture in the test using _Client and
_QueryResult with a second page row whose started_at_us differs from the
existing page_row value. Update the assertions for page_started_at_min_us and
page_started_at_max_us to verify the distinct lower and upper timestamps
produced by the non-empty range.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c3081e2c-6e30-4bfb-9a99-b098da3815f9

📥 Commits

Reviewing files that changed from the base of the PR and between f013d9c and 15553f6.

📒 Files selected for processing (2)
  • services/intake/src/nmp/intake/repository/clickhouse/trace.py
  • services/intake/tests/test_traces_clickhouse_repository.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • services/intake/src/nmp/intake/repository/clickhouse/trace.py

@BrianNewsom
BrianNewsom added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit 86c0d88 Aug 3, 2026
52 checks passed
@BrianNewsom
BrianNewsom deleted the fix-trace-hydration-primary-key/brnewsom branch August 3, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants