feat(querysvc): Compute trace summaries natively or fall back to full-trace aggregation#8638
Merged
yurishkuro merged 13 commits intoMay 26, 2026
Conversation
…ntegration test When a SummaryReader (e.g. the future gRPC remote storage adapter) yields errors.ErrUnsupported, QueryService.FindTraceSummaries now transparently falls back to FindTraces + computeSummaries rather than propagating the error. This allows the adapter to discover at runtime that the remote backend has not implemented FindTraceSummaries and degrade gracefully. Also adds end-to-end integration test coverage for FindTraceSummaries: - traceReader in cmd/jaeger/internal/integration now implements tracestore.SummaryReader by calling the v3 gRPC FindTraceSummaries RPC. - StorageIntegration gains an optional SummaryReader field; RunSpanStoreTests includes a FindTraceSummaries sub-test when it is set. - E2EStorageIntegration wires the SummaryReader from the same traceReader, so TestJaegerQueryService exercises the full stack end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
…rn (iter, error) The previous design returned only iter.Seq2, requiring callers to iterate before discovering that the backend does not support the operation. By returning a direct error for the unsupported case, the QueryService fallback triggers immediately without starting the iterator loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Assert that service name and time bounds are non-empty after iterating the trace fixture, so a broken fixture fails fast rather than polling for 100 seconds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
- Parse proto trace_id directly via encoding/hex instead of routing through the legacy model.TraceID type in FindTraceSummaries - Remove SummaryReader field from StorageIntegration; testFindTraceSummaries now casts TraceReader to tracestore.SummaryReader and skips if unsupported, consistent with the standard opt-out mechanism via Capabilities.SkipList - Remove explicit s.SummaryReader assignment in e2e_integration.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
… comment fix Update SummaryReader interface signature, section 6 description, and code snippet to reflect FindTraceSummaries returning (iter.Seq2, error) rather than yielding ErrUnsupported inside the iterator. Also fix the test function comment that incorrectly said "yields" instead of "returns as direct error". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Replace the runtime interface-cast skip with the Capabilities.SkipList mechanism: all existing backends opt out via FindTraceSummariesTest, and the test uses require.True so a misconfigured backend fails loudly. The e2e test no longer needs special wiring since TraceReader implements SummaryReader and the backend does not appear in any skip list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
…st design SummaryReader is no longer a separate field on StorageIntegration; the test casts TraceReader to SummaryReader and backends opt out via SkipList. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
- Initialize Attributes with pcommon.NewMap() in testFindTraceSummaries to prevent nil-pointer panic in PcommonMapToPlainMap - Add Memory() capabilities preset with FindTraceSummaries skipped, and apply it in memstore_test.go (in-process memory store does not implement tracestore.SummaryReader) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
The gRPC remote storage adapter's TraceReader does not yet implement tracestore.SummaryReader (planned for a later PR). Add a GRPC() capabilities preset and apply it in GRPCStorageIntegrationTestSuite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
yurishkuro
added
the
changelog:experimental
yurishkuro
changed the title
jaegertracingbot
approved these changes
May 26, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates trace summary reading so unsupported native summary implementations can signal fallback before iteration, and adds integration coverage for the trace summary path.
Changes:
- Changes
tracestore.SummaryReader.FindTraceSummariesto return(iterator, error)and updates QueryService fallback behavior. - Updates mocks/tests and adds a storage integration sub-test for trace summaries.
- Adds an integration
traceReaderadapter for the api_v3FindTraceSummariesstream and updates ADR documentation.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/storage/v2/api/tracestore/summary.go |
Updates SummaryReader contract to include direct unsupported errors. |
internal/storage/v2/api/tracestore/mocks/mocks.go |
Regenerates SummaryReader mock for the new signature. |
cmd/jaeger/internal/extension/jaegerquery/querysvc/service.go |
Adds ErrUnsupported fallback handling in QueryService. |
cmd/jaeger/internal/extension/jaegerquery/querysvc/summary_test.go |
Updates tests and adds ErrUnsupported fallback coverage. |
cmd/jaeger/internal/integration/trace_reader.go |
Implements SummaryReader over the api_v3 streaming RPC. |
internal/storage/integration/integration.go |
Adds FindTraceSummaries integration test. |
internal/storage/integration/capabilities/capabilities.go |
Adds capability opt-outs for unsupported backends. |
internal/storage/integration/memstore_test.go |
Applies memory backend capabilities. |
internal/storage/integration/grpc_test.go |
Applies gRPC backend capabilities. |
docs/adr/010-trace-summary-api.md |
Documents the updated contract and integration coverage. |
Files not reviewed (1)
- internal/storage/v2/api/tracestore/mocks/mocks.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8638 +/- ##
=======================================
Coverage 96.57% 96.57%
=======================================
Files 334 334
Lines 17716 17721 +5
=======================================
+ Hits 17109 17114 +5
Misses 455 455
Partials 152 152
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CI Summary ReportMetrics ComparisonView changed metricsFor label-level diff details, open the CI run and expand the "Compare metrics and generate summary" step logs. metrics_snapshot_clickhouse — ⬇️ download diff
metrics_snapshot_elasticsearch_9.x_e2e — ⬇️ download diff
metrics_snapshot_grpc_e2e — ⬇️ download diff
metrics_snapshot_memory — ⬇️ download diff
Code Coverage✅ Coverage 97.3% (baseline 97.3%) ➡️ View CI run | View publish logs |
…maryReader Covers the path where FindTraceSummaries returns an error that is not ErrUnsupported — it should be propagated to the caller, not swallowed or used to trigger the fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
summary_test.go holds tests for helper functions (computeSummaries, summarizeTrace); service_test.go holds tests for QueryService API methods. Move the five TestFindTraceSummaries_* tests and their supporting types (mockSummaryReader, wrappingReader) accordingly. Also translate codes.Unimplemented to errors.ErrUnsupported in the integration traceReader.FindTraceSummaries so QueryService can fall back correctly when the remote server has not implemented the RPC. Update ADR-010 to fold the ErrUnsupported design into section 5 (Fallback Logic) and tighten section 6 to describe only the gRPC adapter's Unimplemented translation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Comment on lines
+180
to
+182
| if err != nil { | ||
| yield(nil, err) | ||
| return |
Comment on lines
+312
to
+316
| The gRPC-based remote storage adapter wraps the remote `TraceReader` gRPC client. Its | ||
| `FindTraceSummaries` implementation calls the remote RPC and, if the server returns | ||
| `codes.Unimplemented`, translates it to an error wrapping `errors.ErrUnsupported` before | ||
| returning. This translation happens before an iterator is returned, so `QueryService` | ||
| detects the unsupported case immediately and falls back to `FindTraces` + `computeSummaries`. |
vic-comm
pushed a commit
to vic-comm/jaeger
that referenced
this pull request
Jun 11, 2026
…-trace aggregation (jaegertracing#8638) ## What this PR does Implements the query-service layer for the Trace Summary API (tracking issue jaegertracing#8606, part C of the PR sequence in [ADR-010](https://github.com/jaegertracing/jaeger/blob/main/docs/adr/010-trace-summary-api.md)). **Core behaviour:** - `QueryService.FindTraceSummaries` delegates to `tracestore.SummaryReader` when the underlying storage implements it (native path — efficient, storage-computed summaries). - If no `SummaryReader` is present, or if it returns `errors.ErrUnsupported` as a direct error (e.g. a remote backend that has not yet implemented the RPC), it transparently falls back to `FindTraces` + client-side aggregation via `jptrace.AggregateTraces`. - `findSummaryReader` walks the reader chain via `Unwrap()` so decorators wrapping the underlying reader remain transparent. **Interface design:** - `tracestore.SummaryReader.FindTraceSummaries` returns `(iter.Seq2[[]TraceSummary, error], error)` — the direct error lets callers detect "not supported" immediately before starting iteration. **End-to-end integration test:** - `traceReader` in `cmd/jaeger/internal/integration` implements `tracestore.SummaryReader` by calling the `api_v3.QueryService.FindTraceSummaries` gRPC RPC. - `StorageIntegration.RunSpanStoreTests` always runs `FindTraceSummaries`; backends opt out by adding `"FindTraceSummaries"` to their `Capabilities.SkipList`. ## Testing - Unit tests for `computeSummaries`, fallback path, native path, `Unwrap` chain, and `ErrUnsupported` fallback: `querysvc/summary_test.go` - End-to-end coverage via `TestJaegerQueryService`, `TestGRPCStorage`, `TestMemoryStorage` ## Related - Tracking issue: jaegertracing#8606 - ADR-010: [docs/adr/010-trace-summary-api.md](https://github.com/jaegertracing/jaeger/blob/main/docs/adr/010-trace-summary-api.md) - Prerequisite PRs: jaegertracing#8604 (proto), jaegertracing#8633 (gRPC handler), jaegertracing#8634 (HTTP handler) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Victor Chidera Obiezue <obiezuechidera@gmail.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
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.
What this PR does
Implements the query-service layer for the Trace Summary API (tracking issue #8606, part C of the PR sequence in ADR-010).
Core behaviour:
QueryService.FindTraceSummariesdelegates totracestore.SummaryReaderwhen the underlying storage implements it (native path — efficient, storage-computed summaries).SummaryReaderis present, or if it returnserrors.ErrUnsupportedas a direct error (e.g. a remote backend that has not yet implemented the RPC), it transparently falls back toFindTraces+ client-side aggregation viajptrace.AggregateTraces.findSummaryReaderwalks the reader chain viaUnwrap()so decorators wrapping the underlying reader remain transparent.Interface design:
tracestore.SummaryReader.FindTraceSummariesreturns(iter.Seq2[[]TraceSummary, error], error)— the direct error lets callers detect "not supported" immediately before starting iteration.End-to-end integration test:
traceReaderincmd/jaeger/internal/integrationimplementstracestore.SummaryReaderby calling theapi_v3.QueryService.FindTraceSummariesgRPC RPC.StorageIntegration.RunSpanStoreTestsalways runsFindTraceSummaries; backends opt out by adding"FindTraceSummaries"to theirCapabilities.SkipList.Testing
computeSummaries, fallback path, native path,Unwrapchain, andErrUnsupportedfallback:querysvc/summary_test.goTestJaegerQueryService,TestGRPCStorage,TestMemoryStorageRelated
🤖 Generated with Claude Code