refactor: filter endpoint durations through endpointFilterClause - #318
Closed
FrameAutomata wants to merge 1 commit into
Closed
refactor: filter endpoint durations through endpointFilterClause#318FrameAutomata wants to merge 1 commit into
FrameAutomata wants to merge 1 commit into
Conversation
FrameAutomata
added
the
ci
Builds on 95a65d2, which fixed the endpoint percentile/filter split while this branch was in review. That commit threads rootFilter into fetchSortedDurations and splices shared.RootFilterClause directly. Two follow-ups. First, the duration query now goes through endpointFilterClause like the queries that select the endpoint, instead of picking out the one leg that constrains rows today. There is no behaviour change: with the endpoint pinned by `endpoint = :endpoint`, the search and method legs cannot alter the row set, so only rootFilter is load-bearing. What it buys is that a row-level predicate added to endpointFilterClause reaches the percentile query automatically. Splicing one leg by hand is how the original split happened, and it is the shape the task and AI-trace repositories now share. Second, a regression test for the chart ranking. 95a65d2 fixed getTopEndpointsByMetric along with the grouped list, but only the grouped list got a test. The chart ranks its top 5 on percentiles and then plots only the filtered rows, so ranking on the unfiltered population puts endpoints on the chart that are not slow under the filter. This test pins that; it passes on main, so it guards the fix rather than reporting a live bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FrameAutomata
force-pushed
the
fix/endpoint-filter-percentile-parity
branch
from
August 26, 2026 21:27
080afce to
b4c0a97
Compare
FrameAutomata
changed the title
FrameAutomata
added
ci
FrameAutomata
added a commit
that referenced
this pull request
Aug 26, 2026
The same split #318 fixes for endpoints was live in two sibling repositories on the SQLite telemetry backend. `FindGroupedByTaskName` and `FindGroupedByTraceName` both apply their search + root filter to the grouping query, then compute percentiles from a second query that carried no filter at all. So a filtered `count` was reported next to unfiltered P50/P95 -- and because both lists sort and paginate on those values (`orderBy=p50_duration|p95_duration|impact` takes the Go-sort path in tasks, and the AI-traces list orders on them directly), the page was ordered by the population the user had just filtered out, not only mislabelled. DuckDB and ClickHouse compute the percentiles inline in the filtered query and were already correct, so this was also a storage-backend parity break: identical data, different numbers and different row order. Both files now route the selecting query and the duration query through one clause builder -- `taskFilterClause` / `aiTraceFilterClause`, mirroring `endpointFilterClause` -- so the two cannot drift apart again, and a predicate added there is picked up by both. In tasks that also removes a second, hand-maintained copy of the search parameter: the group query's parameter map is now copied from the filtered one rather than re-deriving one leg of it. Verified end-to-end, not just in tests: two binaries on the same seeded database populated over real OTLP ingest, queried over HTTP. tasks rootFilter=root main: count=4 p50=5050ms p95=10000ms branch: count=4 p50=100ms p95=100ms ai-traces rootFilter=root main: count=4 p50=15100ms p95=30000ms branch: count=4 p50=200ms p95=200ms `rootFilter=all` is identical on both. Refs #313 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Folded into #319 and closing. What was left here after Both make more sense there. The argument for the first is that all three entities — tasks, AI traces, endpoints — should route their duration query through one clause builder so the selecting query and the percentile query cannot drift apart; that claim is worth showing in a single diff rather than split across two PRs referencing each other. #319 now does all three. No work is lost: the commit is cherry-picked onto #319 unchanged ( |
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
1 participant
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.
Rewritten. This PR originally fixed the endpoint percentile/filter split described in #313. While it was in review,
95a65d24landed that fix onmainindependently. The branch has been rebuilt on top ofmainand now contains only what is left over.What
mainalready has95a65d24threadsrootFilterintofetchSortedDurationsand splicesshared.RootFilterClausedirectly, covering bothFindGroupedByEndpointandgetTopEndpointsByMetric, plus a test for the grouped list. That is the bug fixed. Nothing here re-fixes it.What is left
1. Route the duration query through
endpointFilterClause.No behaviour change today: with the endpoint pinned by
endpoint = :endpoint, thesearchandmethodlegs cannot alter the row set, sorootFilteris the only load-bearing one. What it buys is that a row-level predicate added toendpointFilterClausereaches the percentile query automatically instead of silently applying to the selecting queries only.Picking one leg out by hand is precisely how the original split happened — the grouping query used the full clause builder and the duration query used none. It is also the shape
sqlite/task.repository.goandsqlite/ai_trace.repository.goland on in #319, so all three entities read the same way.2. A regression test for the chart ranking.
95a65d24fixedgetTopEndpointsByMetricalongside the grouped list, but only the grouped list got a test. The chart ranks its top 5 on percentiles and then plots only the filtered rows, so ranking on the unfiltered population puts endpoints on the chart that are not slow under the filter — a distinct symptom from the grouped list's wrong numbers.This test passes on
main. It guards the existing fix; it does not report a live bug. Flagging that explicitly so the green run is not mistaken for "reproduced and fixed".Verification
go test -race -count=1 ./app/..., the DuckDB-tagged repository suite,go vet ./app/...,gofmt— all clean. Test placement is the untaggedtelemetrypackage, so it runs against all three backends.Relationship to the others
95a65d24did not touch. Still a live fix, unaffected by this rewrite.🤖 Generated with Claude Code