feat(tracing): add keyset pagination to spans query endpoint#60737
Merged
jonmcwest merged 1 commit intoJun 1, 2026
Merged
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
DanielVisca
approved these changes
May 29, 2026
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/tracing/backend/presentation/views.py:348-358
**Trace start may be wrong for prefetched pages without a root span**
`trace_start[tid]` is computed as the minimum timestamp across the prefetched spans returned for each trace. The outer query orders prefetched spans by `is_root_span DESC, matched_filter DESC, timestamp {order_dir}`, so when a root span exists it is always fetched first and is the temporal minimum — the assumption holds. However, when a trace has no span with `is_root_span = 1` (e.g., a partial/incomplete trace ingested without root-span detection) **and** `prefetchSpans > 1` **and** `orderBy = "latest"`, the N fetched spans are the *newest* ones for that trace, not the oldest. The Python `min(timestamp)` of those N spans can be later than the SQL `min(timestamp)` used in the HAVING clause, causing the cursor for that boundary trace to encode the wrong timestamp. On the next page the HAVING would still be `< cursor_ts` but `sql_min(timestamp) < cursor_ts` would be true, so the trace would reappear — a duplicate across pages.
Reviews (1): Last reviewed commit: "feat(tracing): add keyset pagination to ..." | Re-trigger Greptile |
7553e8e to
81c534e
Compare
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
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.

Problem
Trace list pagination was broken: the
aftercursor was applied as a span-level keyset filter insidewhere(), which is shared by the sparkline, aggregation, and tree runners. This caused child spans of valid traces to be incorrectly filtered out, andhasMore/nextCursorwere never populated (hardcoded toFalse/None).Changes
Cursor moved to the trace level
Pagination now operates on traces rather than individual spans. The inner query switches from
LIMIT 1 BY trace_idtoGROUP BY trace_id, ordering bymin(timestamp)(the root span's start time) withtrace_idas a tiebreaker. The keysetHAVINGclause is applied only to this grouped query, so child spans of kept traces are never filtered.where()no longer touches the cursorThe
aftercursor logic is removed fromwhere()entirely. A comment explains why:where()is shared across runners that never paginate, and a span-level keyset would incorrectly drop spans belonging to traces that straddle the page boundary.Cursor encoding uses
trace_id(hex → base64)The cursor now stores
trace_idin hex (the human-readable form used by the rest of the API) and re-encodes it to the table's base64 storage form for SQL comparison, keeping the cursor format consistent with the public API surface.hasMoreandnextCursorare now realThe view layer computes the actual trace order from the prefetched spans, trims the overflow trace, and emits a cursor pointing at the last kept trace.
hasMorereflects whether the runner returned more traces than the requested limit.time_sliced_resultsremovedThe view no longer wraps the runner in
time_sliced_results; a single blockingrunner.run()call is used instead, consistent with how the cursor-based approach needs a single stable result set.How did you test this code?
Automated tests in the tracing backend test suite cover the cursor encoding/decoding, the
GROUP BYquery shape, and thehasMore/nextCursorresponse fields.Docs update