Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMoved OpenTelemetry trace-context extraction out of handlers into Connect RPC interceptors; added otelconnect-based client/server interceptors, integrated them into server and outbound client chains, and added tests for trace-context propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CCI as Connect Client Trace Interceptor
participant Network as Network / HTTP
participant SCI as Connect Server Trace Interceptor
participant Handler as Service Handler
participant OTel as OpenTelemetry
Client->>CCI: start client span and invoke RPC
CCI->>OTel: read global propagator & span context
CCI->>Network: inject trace headers and send request
Network->>SCI: deliver request with trace headers
SCI->>OTel: extract context via global propagator
SCI->>Handler: invoke handler with extracted/linked ctx
Handler->>Handler: perform work (span active)
Handler-->>SCI: return response
SCI-->>CCI: response flows back
CCI-->>Client: client receives response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a centralized approach to distributed tracing for Connect RPC services. By moving trace context extraction and propagation into global interceptors, it ensures consistent trace continuity across service boundaries while significantly reducing boilerplate code in individual handlers. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The headers arrive with a trace, / We capture them all in one place. / No more manual calls, / As the context now falls, / Into place with a elegant grace. Footnotes
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request centralizes OpenTelemetry trace context extraction by replacing manual extraction in individual authorization service methods with a global Connect RPC interceptor. A new ConnectServerTraceInterceptor is introduced and integrated into the server's interceptor chain, and outbound IPC calls are updated to propagate trace context. Feedback suggests expanding the new interceptor to support streaming RPCs by implementing the full connect.Interceptor interface, ensuring complete trace continuity across all request types.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Add client and server Connect interceptors for OTel trace propagation: - ConnectServerTraceInterceptor: extracts traceparent/tracestate from incoming requests; wired as the first handler interceptor globally - ConnectClientTraceInterceptor: injects trace context into outbound IPC requests; wired as the first IPC client interceptor Remove redundant per-handler trace extraction from authorization service handlers. This also fixes an ordering bug where spans were started before trace context was extracted, causing them to be root spans instead of children of the incoming trace. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
pflynn-virtru
force-pushed
the
feat/service-trace-context-interceptor
branch
from
214095c to
d81c299
Compare
pflynn-virtru
changed the title
pflynn-virtru
changed the base branch from
feat/sdk-trace-context-interceptor
to
main
pflynn-virtru
changed the title
pflynn-virtru
changed the title
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/internal/server/server_test.go`:
- Around line 557-583: The test currently only checks interceptor counts (fields
like wantIntLen, wantDescription, authInt, extraInts, noopInterceptor()) but not
ordering; add an ordering assertion in the interceptor-ordering test by
replacing or augmenting the authInt with a test auth interceptor that asserts
the trace context extracted earlier is present in ctx (e.g., inspect whatever
key the trace interceptor sets or use the trace-extraction API the code uses)
and fails the test if it's missing, then invoke the interceptor chain (using
connect.Interceptor semantics) so the trace interceptor runs before auth and the
auth interceptor can validate the extracted trace context; this proves auth sees
the extracted trace and prevents regressions if order changes.
In `@service/tracing/connect_interceptor_test.go`:
- Around line 110-139: TestTraceContextPropagation_NoTraceContext currently
calls client.CallUnary with context.Background(), so even if a real propagator
existed the server would still see an invalid TraceID; to make the test actually
assert that a no-op propagator prevents propagation, create an active parent
span on the client side before calling the RPC (use otel.Tracer(...).Start to
get a ctx with a span) and call client.CallUnary with that ctx; then verify
serverTraceID is invalid (SpanContextFromContext(ctx).TraceID() remains empty)
while keeping the test wrapped with setting
otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator()) and using
tracing.ConnectClientTraceInterceptor()/tracing.ConnectServerTraceInterceptor()
to exercise the interceptors.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95bb8ade-686e-4414-a25c-57ad0437797d
📒 Files selected for processing (6)
service/authorization/authorization.goservice/authorization/v2/authorization.goservice/internal/server/server.goservice/internal/server/server_test.goservice/tracing/connect_interceptor.goservice/tracing/connect_interceptor_test.go
💤 Files with no reviewable changes (2)
- service/authorization/authorization.go
- service/authorization/v2/authorization.go
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…t, fix no-op test Address review feedback: - Implement full connect.Interceptor interface (WrapUnary, WrapStreamingClient, WrapStreamingHandler) instead of UnaryInterceptorFunc, so trace context propagates for streaming RPCs too - Fix NoTraceContext test to start a real span on the client side, proving the no-op propagator specifically blocks propagation rather than relying on the absence of a span Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
pflynn-virtru
force-pushed
the
feat/service-trace-context-interceptor
branch
from
2b42a60 to
6b8c37b
Compare
Contributor
The remote entity resolution connection (setupERSConnection) was built without any interceptors, so outbound ERS calls from GetDecision had no trace context propagation. Add ConnectClientTraceInterceptor to close this gap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Replace the hand-rolled propagation-only interceptors with connectrpc.com/otelconnect, which provides per-RPC spans, metrics (duration, message size, in-flight count), and trace propagation. Server interceptor uses WithTrustRemote so incoming spans are children of the remote trace, and WithoutServerPeerAttributes to reduce cardinality. Both use WithoutTraceEvents to keep spans lean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/pkg/server/start.go`:
- Around line 391-396: The tracing interceptor is being appended after the ERS
auth credentials, making tracing inner to auth; call
tracing.ConnectClientTraceInterceptor() and append its result to
ersConnectRPCConn.Options before configureERSAuthentication() (or before any
call that adds interceptor.AddCredentialsConnect()) so the tracing interceptor
is the outermost wrapper; update the order where ersConnectRPCConn.Options are
mutated (refer to ersConnectRPCConn.Options,
tracing.ConnectClientTraceInterceptor(), and
interceptor.AddCredentialsConnect()) to ensure tracing is prepended ahead of the
auth interceptor.
In `@service/tracing/connect_interceptor_test.go`:
- Around line 191-225: The test reads/writes serverTraceID concurrently without
synchronization so the assertion can pass if serverTraceID remains zero (no
server span); protect serverTraceID with a mutex (or atomic) and update the
handler to set serverTraceID under that lock, then before asserting IDs add
require.True(t, serverTraceID.IsValid()) while holding the lock (or loading
atomically) to ensure a server span was created, then assert.NotEqual(t,
clientTraceID, serverTraceID) after releasing/reading the guarded value;
reference serverTraceID and the connect.NewUnaryHandler callback to locate the
changes.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab17a902-333e-4fba-b462-4d9d39b5819b
⛔ Files ignored due to path filters (1)
service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
service/go.modservice/internal/server/server.goservice/pkg/server/start.goservice/tracing/connect_interceptor.goservice/tracing/connect_interceptor_test.go
Move ERS trace interceptor before auth configuration so tracing is the outermost wrapper (spans cover auth latency). Add mutex and validity assertion to no-propagator test per review feedback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/pkg/server/start.go`:
- Around line 375-379: The code currently returns an error if
tracing.ConnectClientTraceInterceptor() fails, which should be non-fatal;
instead log the failure and continue without tracing. Modify the block that
calls ConnectClientTraceInterceptor() so that on error you emit a warning (using
the existing logger used elsewhere in this file or fmt.Printf if no logger is
available), set ersTraceInt to nil (or leave it unset) and proceed rather than
returning the error—mirroring the non-fatal handling used around
ConnectClientTraceInterceptor() elsewhere (see the handling in server.go lines
that call ConnectClientTraceInterceptor()).
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cdbe2d1-cdc6-4661-960d-733ad1c4b679
📒 Files selected for processing (2)
service/pkg/server/start.goservice/tracing/connect_interceptor_test.go
Contributor
dmihalcik-virtru
previously approved these changes
Apr 10, 2026
Match the IPC path's error handling: log and continue without tracing rather than preventing the service from starting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
dmihalcik-virtru
approved these changes
Apr 14, 2026
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
ConnectClientTraceInterceptor()andConnectServerTraceInterceptor()inservice/tracing/for OTel trace propagation via W3Ctraceparent/tracestateheadersconnect.Interceptorinterface (unary + streaming)newConnectRPC()— applies globally to both external and IPC Connect serversinProcessServer.Conn())setupERSConnection())propagator.Extract()calls from authorization service handlers (now handled globally by the server interceptor)Context
Distributed traces break at Connect RPC boundaries because no trace context headers were injected/extracted. This PR closes the gap for all server-side inbound requests, IPC outbound calls, and remote ERS outbound calls. External SDK callers can opt in via
WithExtraClientOptions(connect.WithInterceptors(...)).Test plan
cd service && go build ./...compilesgo test ./tracing/— end-to-end unary + server-streaming propagation tests, no-op propagator safety testgo test ./internal/server/— interceptor count and ordering tests updatedgo test ./authorization/ ./authorization/v2/— authorization tests passgolangci-lint run ./tracing/ ./internal/server/ ./authorization/ ./authorization/v2/ ./pkg/server/— 0 issues🤖 Generated with Claude Code
Summary by CodeRabbit