feat(intake): sort span groups by time - #1242
Merged
Merged
Conversation
Span groups could only be sorted by size, and carried no timestamp at all, so "which traces did this agent run lately" had no answer. Callers had to page spans newest first and collect distinct trace ids, which is slower and only ever finds the traces that fit inside the scanned window. Group rows now carry started_at, the earliest matching span in the group, and sort accepts started_at and -started_at. Only matching spans count toward the time, so a filter reports when the filtered work began rather than when the whole trace or session began. The integration test pins that distinction: every span in it shares one trace, yet the two session groups report different start times. The sort whitelist is now a named set that the API enum is tested against, so the two cannot drift apart. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughSpan-group responses now include the earliest matching span start time. ClickHouse grouped queries support ascending and descending ChangesSpan-group chronological sorting
Sequence Diagram(s)sequenceDiagram
participant Client
participant SpanGroupsAPI
participant ClickHouseSpanRepository
participant ClickHouse
Client->>SpanGroupsAPI: Request grouped spans with sort
SpanGroupsAPI->>ClickHouseSpanRepository: Pass group sort field
ClickHouseSpanRepository->>ClickHouse: Run min(start_time) and ORDER BY
ClickHouse-->>ClickHouseSpanRepository: Return grouped rows with started_at
ClickHouseSpanRepository-->>SpanGroupsAPI: Return sorted span groups
SpanGroupsAPI-->>Client: Return grouped response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/intake/tests/integration/spans/test_spans_read_filters.py (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the group payload by shape.
dict[str, str]is incorrect. The payload containsgroup: dict[str, str],span_count: int, andstarted_at: str. Define aTypedDictfor this payload.🤖 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/integration/spans/test_spans_read_filters.py` around lines 11 - 14, Define a TypedDict for the group payload with group as dict[str, str], span_count as int, and started_at as str, then update _group_started_at to accept that TypedDict instead of dict[str, str].Source: Coding guidelines
🤖 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/integration/spans/test_spans_read_filters.py`:
- Around line 11-14: Define a TypedDict for the group payload with group as
dict[str, str], span_count as int, and started_at as str, then update
_group_started_at to accept that TypedDict instead of dict[str, str].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fdb40441-812d-4378-9065-af685aada693
📒 Files selected for processing (10)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/repository/clickhouse/span.pyservices/intake/src/nmp/intake/spans/api/spans.pyservices/intake/src/nmp/intake/spans/api/spans_schemas.pyservices/intake/src/nmp/intake/spans/domain.pyservices/intake/tests/integration/spans/test_spans_read_filters.pyservices/intake/tests/test_spans_clickhouse_repository.pyservices/intake/tests/test_spans_schemas.py
6 tasks
Contributor
|
Adding started_at to SpanGroup and to the sort enum changed the published contract, so the checked-in SDK and CLI went stale and CI's sync check failed. Regenerating gives SpanGroup a started_at field, widens SpanGroupSortField to include started_at and -started_at, and carries the sort guidance into both CLIs and the reference docs. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/groups.py`:
- Line 96: Update the help text for the --by option in the groups command to
describe -started_at as descending order by each group’s earliest matching span
timestamp, without claiming it always returns the most recently active traces or
sessions first. Then regenerate docs/cli/reference.mdx from the CLI help.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43d23d81-baa3-4933-a06b-3fead2890088
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/pyproject.tomlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/groups.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/spans/groups.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/group_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/span_group.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/spans/span_group_sort_field.pyis excluded by!sdk/**
📒 Files selected for processing (2)
docs/cli/reference.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/groups.py
SpanGroup now requires started_at, so the fakes that stand in for a server reply no longer type-check or match. The Analyst dumps group rows straight through, so its list_span_groups output carries the new field as well. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
A group's time is its earliest matching span, so -started_at returns the groups that began most recently, not the ones most recently active. A long trace that started hours ago sorts low even while it is still running. The old wording said "most recent", which reads as the second meaning. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
BrianNewsom
approved these changes
Aug 11, 2026
aleckhoury
added a commit
that referenced
this pull request
Aug 12, 2026
Per review on #1212: the workaround should not need to exist, and it no longer does, so the notes describing it go too. The docstring no longer warns the agent off the five dataset and prompt span filters, and _explain no longer has a 500 branch naming them. #1225 unpublished those fields, so they read as unknown filter fields like any other typo, and the 400 branch already says where the real vocabulary lives. A 500 now falls through to the generic branch, which still names the status, since an Intake fault leaves the caller nothing to correct. The module docstring also still said span groups sort only by count, which #1242 changed and this branch already relies on. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
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
2 participants
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.
Summary
Span groups could only be sorted by size, and carried no timestamp at all. That left a common question unanswerable in one call: which traces did this agent run lately?
Callers had to work around it by paging spans newest first and collecting distinct trace ids. That is slower, and it is not even correct: it only ever finds the traces that happen to fit inside the scanned window.
This adds:
started_aton each group row, the earliest matching span in the group.started_atand-started_atas sort options.Only matching spans count toward the time, so a filter reports when the filtered work began rather than when the whole trace or session began.
Why this came up
Raised by @BrianNewsom while reviewing #1212, on a helper that had to drain spans to find an agent's recent traces:
He was right. Once this lands, that helper collapses to a single grouped call.
Validation against a running instance
Ran a second Intake on port 8081 against the shared ClickHouse holding real trace data, and compared it with an unpatched instance on 8080.
Patched,
sort=-started_at, newest first:The span counts are unordered, so time is genuinely driving the sort. The default
-span_countstill returns biggest first (64, 45, 45, 43, 41) and picks an entirely different set of traces, so the two orders are not accidentally the same.Unpatched, same request:
Generated clients
make update-sdkcarries the change into the checked-in SDK and CLI, which CI's sync check requires.SpanGroupgainsstarted_at,SpanGroupSortFieldwidens to the four values, and the sort guidance reaches both CLIs anddocs/cli/reference.mdx.started_atis required rather than optional, because every group of spans has an earliest one and an optional field would push a case that cannot happen onto every caller. That does make it a breaking change for anyone constructing aSpanGroupby hand, which in this repo is the Analyst's test fakes. They now pass a stamp, and the Analyst'slist_span_groupsoutput carries the new field through to its model.Test plan
test_list_span_groups_sorts_by_time_so_recent_work_is_one_callpins the SQL and the resulting order.test_group_order_by_*pin the whitelist and the rejection of an injected sort key.test_group_sort_enum_matches_repository_sort_fieldskeeps the API enum and the repository whitelist from drifting.services/intakesuite passes: 276 unit, 108 spans integration.plugins/nemo-insights: 146 passed. Thetestbedfailures aretar --zstdon macOS, identical onmain.tools/lint/lint-python-types.sh, the exact script CI runs: no errors, only pre-existing warnings.ruff check,ruff format --checkclean.Summary by CodeRabbit
New Features
started_at) and descending (-started_at) sort options.Documentation
Tests