feat(grpc): Implement SummaryReader in gRPC storage adapter#8642
Merged
yurishkuro merged 7 commits intoMay 27, 2026
Conversation
Handler.FindTraceSummaries forwards to the underlying tracestore.SummaryReader when available; returns codes.Unimplemented otherwise (the default from UnimplementedTraceReaderServer). TraceReader.FindTraceSummaries implements tracestore.SummaryReader on the client side. The stream is primed eagerly: the first Recv is called before handing back an iterator, so a codes.Unimplemented response is surfaced as a direct errors.ErrUnsupported-wrapped error rather than appearing inside the iterator. QueryService picks up the new implementation automatically via the existing findSummaryReader chain-walker. Remove FindTraceSummaries from capabilities.GRPC() skip list so that the gRPC integration test exercises the full round-trip. Update ADR-010: mark all DECISION sections and Milestone 4 as done, mark PR D as done, remove completed PR F row. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends Jaeger’s storage-v2 gRPC remote storage adapter to support the optional tracestore.SummaryReader capability, enabling FindTraceSummaries to work end-to-end over gRPC and allowing QueryService to fall back to local summary computation when the remote backend does not implement it.
Changes:
- Add
FindTraceSummariessupport to the gRPC storage client (TraceReader) with eager stream priming to surfaceUNIMPLEMENTEDas a directerrors.ErrUnsupportederror. - Add
FindTraceSummariessupport to the gRPC storage server (Handler) by forwarding to an underlyingtracestore.SummaryReaderwhen available. - Enable the gRPC integration capability test for
FindTraceSummariesby removing it from the gRPC skip list; update ADR-010 milestone/status text.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/v2/grpc/tracereader.go | Implements tracestore.SummaryReader on the gRPC client adapter, including eager stream priming and proto→model conversion. |
| internal/storage/v2/grpc/tracereader_test.go | Adds unit tests covering success, stream error, and UNIMPLEMENTED handling for FindTraceSummaries. |
| internal/storage/v2/grpc/handler.go | Adds gRPC server-side FindTraceSummaries RPC that delegates to an underlying SummaryReader. |
| internal/storage/v2/grpc/handler_test.go | Adds unit tests for handler delegation, not-implemented behavior, iterator error propagation, and send errors. |
| internal/storage/integration/capabilities/capabilities.go | Removes the gRPC skip list entry so integration tests exercise FindTraceSummaries over gRPC. |
| docs/adr/010-trace-summary-api.md | Marks decisions/milestone progress as complete for the remote storage adapter work and updates status text. |
💡 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 #8642 +/- ##
==========================================
+ Coverage 96.56% 96.60% +0.03%
==========================================
Files 334 334
Lines 17721 17803 +82
==========================================
+ Hits 17112 17198 +86
+ Misses 456 455 -1
+ Partials 153 150 -3
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:
|
yurishkuro
added
the
changelog:experimental
yurishkuro
commented
May 26, 2026
…PC e2e - Clarify stream-priming comment: gRPC delivers RPC-level errors via the first Recv(), so priming is needed to surface ErrUnsupported as a direct return for QueryService fallback. - Add FindTraceSummaries to GRPC() skip list: the test backend (memory) does not implement SummaryReader, so the test times out without the skip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Add jptrace.TimeToUnixNano and jptrace.UnixNanoToTime to replace three duplicate inline implementations of time↔uint64 conversion across handler.go, tracereader.go, and integration/trace_reader.go. TimeToUnixNano returns 0 for zero or pre-epoch times to match the proto3 default (field omitted), avoiding uint64 wrap-around on negative values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
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_grpc_e2e — ⬇️ download diff
Code Coverage✅ Coverage 97.3% (baseline 97.3%) ➡️ View CI run | View publish logs |
…iterator Remove the top-level error return from SummaryReader.FindTraceSummaries, aligning it with FindTraces. ErrUnsupported is now yielded as the first iterator error instead of returned directly. QueryService.FindTraceSummaries wraps the iterator and falls back to computeSummaries on the first ErrUnsupported error. This removes the need for stream priming in grpc.TraceReader and simplifies the overall design. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- internal/storage/v2/api/tracestore/mocks/mocks.go: Language not supported
Comments suppressed due to low confidence (2)
internal/storage/integration/capabilities/capabilities.go:50
- PR description says
FindTraceSummarieswas removed from the gRPC capabilities skip list so the integration test exercises the full round-trip, butCapabilities.GRPC()still skipsFindTraceSummarieshere (and the comment explains why). Either the PR description is out of date, or this skipList should be updated to reflect the new behavior.
// GRPC returns the capabilities for the gRPC remote storage backend.
// FindTraceSummaries is skipped because it depends on the backing store
// implementing tracestore.SummaryReader; the test backend (memory) does not yet.
func GRPC() Capabilities {
return Capabilities{
skipList: []string{FindTraceSummariesTest},
}
cmd/jaeger/internal/integration/trace_reader.go:185
- In this server-streaming RPC,
codes.Unimplementedcan be delivered viastream.Recv()(not just the initialFindTraceSummariescall). The loop currently yields the raw error onRecv()failures, which means an UNIMPLEMENTED response won’t be translated toerrors.ErrUnsupportedand callers won’t trigger the intended fallback. Consider mappingstatus.Code(err) == codes.Unimplementedto a yieldederrors.ErrUnsupportedhere too (mirroring internal/storage/v2/grpc/tracereader.go).
for {
resp, err := stream.Recv()
if errors.Is(err, io.EOF) {
return
}
if err != nil {
yield(nil, err)
return
}
- SummaryReader.FindTraceSummaries now returns iter.Seq2 (no top-level error) - ErrUnsupported is yielded as the first iterator error, not returned directly - QueryService wraps the iterator and falls back on first ErrUnsupported yield - Section 6 notes that gRPC RPC-level errors arrive via Recv(), not the initial call - Update Milestone 4 items and PR table row D accordingly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
- tracereader: add GRPCClientError and YieldStopsIteration tests; remove dead code (initial-call codes.Unimplemented branch, which gRPC never triggers for server-streaming RPCs) - service: add YieldStopsIteration tests for native path and ErrUnsupported fallback path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- internal/storage/v2/api/tracestore/mocks/mocks.go: Language not supported
Comments suppressed due to low confidence (1)
cmd/jaeger/internal/integration/trace_reader.go:185
- In the api_v3 integration traceReader.FindTraceSummaries iterator, Unimplemented is only translated to errors.ErrUnsupported when returned from the initial FindTraceSummaries(...) call. For server-streaming RPCs, gRPC-Go can deliver RPC-level errors (including codes.Unimplemented) via stream.Recv(), so the current Recv() error branch would yield a raw gRPC status error instead of ErrUnsupported (preventing any caller-side fallback/skip logic based on errors.Is(err, errors.ErrUnsupported)). Consider also checking status.Code(err)==codes.Unimplemented in the Recv() error path and yielding an error that wraps errors.ErrUnsupported.
for {
resp, err := stream.Recv()
if errors.Is(err, io.EOF) {
return
}
if err != nil {
yield(nil, err)
return
}
vic-comm
pushed a commit
to vic-comm/jaeger
that referenced
this pull request
Jun 11, 2026
…acing#8642) ## Summary - `Handler.FindTraceSummaries` in `internal/storage/v2/grpc/handler.go` forwards to the underlying `tracestore.SummaryReader` if the trace reader implements it, otherwise returns `codes.Unimplemented`. Existing remote storage plugins are unaffected — `UnimplementedTraceReaderServer` covers them automatically. - `TraceReader.FindTraceSummaries` in `internal/storage/v2/grpc/tracereader.go` implements `tracestore.SummaryReader` as a **plain iterator** (same signature as `FindTraces`, no top-level error). `codes.Unimplemented` from the server arrives via the first `Recv()` (gRPC streaming behavior) and is yielded as `errors.ErrUnsupported`. `QueryService` detects this on the first iterator yield and falls back to `computeSummaries` transparently. - `SummaryReader.FindTraceSummaries` interface simplified: returns `iter.Seq2[[]TraceSummary, error]` — no top-level error, matching `FindTraces`. - `FindTraceSummaries` added to `capabilities.GRPC()` skip list — the gRPC e2e test uses a memory backend that doesn't implement `SummaryReader`, so the test is skipped until memory implements it. - `jptrace.TimeToUnixNano` / `UnixNanoToTime` shared utilities added; three duplicate inline implementations removed. - ADR-010: all DECISION sections and Milestone 4 marked ✅; PR D marked done; PR F row removed (already delivered in jaeger-ui). ## Test plan - [x] `go test ./internal/storage/v2/grpc/...` — new handler and client unit tests pass - [x] `go test ./internal/storage/integration/...` — integration capabilities compile - [x] `make lint` — 0 issues - [x] `go test ./internal/jptrace/...` — new timestamp utility tests pass 🤖 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.
Summary
Handler.FindTraceSummariesininternal/storage/v2/grpc/handler.goforwards to the underlyingtracestore.SummaryReaderif the trace reader implements it, otherwise returnscodes.Unimplemented. Existing remote storage plugins are unaffected —UnimplementedTraceReaderServercovers them automatically.TraceReader.FindTraceSummariesininternal/storage/v2/grpc/tracereader.goimplementstracestore.SummaryReaderas a plain iterator (same signature asFindTraces, no top-level error).codes.Unimplementedfrom the server arrives via the firstRecv()(gRPC streaming behavior) and is yielded aserrors.ErrUnsupported.QueryServicedetects this on the first iterator yield and falls back tocomputeSummariestransparently.SummaryReader.FindTraceSummariesinterface simplified: returnsiter.Seq2[[]TraceSummary, error]— no top-level error, matchingFindTraces.FindTraceSummariesadded tocapabilities.GRPC()skip list — the gRPC e2e test uses a memory backend that doesn't implementSummaryReader, so the test is skipped until memory implements it.jptrace.TimeToUnixNano/UnixNanoToTimeshared utilities added; three duplicate inline implementations removed.Test plan
go test ./internal/storage/v2/grpc/...— new handler and client unit tests passgo test ./internal/storage/integration/...— integration capabilities compilemake lint— 0 issuesgo test ./internal/jptrace/...— new timestamp utility tests pass🤖 Generated with Claude Code