Skip to content

Draft: feat: migrate gRPC middleware to v2, replace go-grpc-prometheus#21

Closed
ankurs wants to merge 7 commits intomainfrom
chore/phase4-middleware-migration
Closed

Draft: feat: migrate gRPC middleware to v2, replace go-grpc-prometheus#21
ankurs wants to merge 7 commits intomainfrom
chore/phase4-middleware-migration

Conversation

@ankurs
Copy link
Copy Markdown
Member

@ankurs ankurs commented Mar 24, 2026

Summary

  • Remove go-grpc-middleware v1 chaining functions — replaced with local chainUnaryServer/chainUnaryClient/chainStreamClient helpers
  • Remove grpc_ctxtags — trace ID propagation simplified to direct loggers.AddToLogContext calls
  • Migrate grpc_retry from v1 to go-grpc-middleware/v2/interceptors/retry
  • Replace go-grpc-prometheus (archived) with go-grpc-middleware/providers/prometheus using instance-based ServerMetrics/ClientMetrics
  • Add EnablePrometheusHandlingTimeHistogram() exported function for core to configure histogram settings
  • Keep grpc_opentracing from v1 — public API GRPCClientInterceptor(options ...grpc_opentracing.Option) depends on it; migration to otelgrpc deferred to OpenTracing removal phase

go-grpc-middleware v1 remains as indirect dependency for opentracing interceptors only.

Test plan

  • make build passes
  • make test passes (go test -race ./...)
  • make lint passes (golangci-lint 0 issues + govulncheck 0 vulnerabilities)

Summary by CodeRabbit

  • New Features

    • Added Prometheus metrics configuration APIs: enable custom handling-time histograms, get/set client and server metrics, and configure interceptor options at runtime.
  • Tests

    • Added tests covering interceptor chaining behavior (ordering, nil-filtering, and invocation guarantees).
  • Documentation

    • Updated README and docs to document the new public metrics configuration APIs.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the interceptor stack to go-grpc-middleware v2 APIs and replaces the archived go-grpc-prometheus integration with the newer instance-based Prometheus provider, while keeping the existing OpenTracing interceptors for compatibility.

Changes:

  • Replace v1 chaining helpers with local chainUnaryServer / chainUnaryClient / chainStreamClient.
  • Migrate retry interceptor to go-grpc-middleware/v2/interceptors/retry.
  • Swap go-grpc-prometheus for go-grpc-middleware/providers/prometheus and add EnablePrometheusHandlingTimeHistogram().

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
interceptors.go Updates interceptor wiring (retry/prometheus/tags) and introduces local chaining + histogram configuration.
go.mod Adds v2 middleware + Prometheus provider deps; removes go-grpc-prometheus.
go.sum Updates sums to reflect new middleware/prometheus dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread interceptors.go
Comment thread interceptors.go Outdated
Comment thread interceptors.go
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 24, 2026

📝 Walkthrough

Walkthrough

Switched Prometheus integration to the grpc-middleware v2 Prometheus provider, added exported APIs to get/set client/server metrics and interceptor options plus handling-time histogram toggle, implemented local interceptor-chaining helpers that filter nils, added stream context wrapping, and added tests and README updates documenting the new APIs.

Changes

Cohort / File(s) Summary
Dependency Migration
go.mod
Replaced github.com/grpc-ecosystem/go-grpc-prometheus with github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus and added github.com/grpc-ecosystem/go-grpc-middleware/v2.
Prometheus & API surface
interceptors.go
Added exported APIs: EnablePrometheusHandlingTimeHistogram(buckets []float64), GetClientMetrics() *grpcprom.ClientMetrics, GetServerMetrics() *grpcprom.ServerMetrics, SetClientInterceptorOptions(opts ...grpcprom.Option), SetServerInterceptorOptions(opts ...grpcprom.Option), SetClientMetrics(m *grpcprom.ClientMetrics), SetServerMetrics(m *grpcprom.ServerMetrics); register/unregister metrics on the Prometheus registerer; replaced old provider usage with v2 provider.
Interceptor chaining & streams
interceptors.go
Introduced local chainUnaryServer, chainUnaryClient, chainStreamClient helpers that filter nil interceptors; removed grpc_ctxtags usage; added wrappedServerStream to ensure derived Context is used by stream handlers and stream interceptors.
Tests
chain_test.go
New tests for chaining helpers covering ordering, nil filtering, single/nil-only cases for unary server, unary client, and stream client chaining.
Documentation
README.md
Added documentation entries for the new Prometheus/metrics APIs and updated function anchor/line references.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ClientInterceptors as Client Interceptors
  participant ServerInterceptors as Server Interceptors
  participant PromReg as Prometheus Registerer

  Client->>ClientInterceptors: call chained unary/stream interceptors (nil-filtering)
  ClientInterceptors->>ServerInterceptors: forward request
  ServerInterceptors->>ServerInterceptors: wrap stream / derive Context
  ServerInterceptors->>PromReg: register/unregister or observe via srvMetrics/cltMetrics
  ServerInterceptors->>ServerInterceptors: invoke handler/streamer
  ServerInterceptors-->>Client: return response/error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 I hop through code and plant a seed,

Prometheus wakes and counts each deed,
Chains sit tidy, nils brushed away,
Streams find context to light the way,
I twitch my nose and skip away.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 74.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: migrating gRPC middleware to v2 and replacing the go-grpc-prometheus library with the newer alternative.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/phase4-middleware-migration

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
interceptors.go (2)

139-165: ⚠️ Potential issue | 🟠 Major

Filter nil interceptors when registering them.

The new chain* helpers only sanitize the convenience path. These four setters still persist nil entries verbatim, so the slice-returning Default*Interceptors() APIs can hand nils back to callers while the convenience chain helpers silently filter them. That makes the public API inconsistent and leaves the server-side path exposed.

♻️ Normalize inputs at registration time
 func AddUnaryServerInterceptor(ctx context.Context, i ...grpc.UnaryServerInterceptor) {
-	unaryServerInterceptors = append(unaryServerInterceptors, i...)
+	unaryServerInterceptors = append(unaryServerInterceptors, filterNilServerInterceptors(i)...)
 }
 
 func AddStreamServerInterceptor(ctx context.Context, i ...grpc.StreamServerInterceptor) {
-	streamServerInterceptors = append(streamServerInterceptors, i...)
+	streamServerInterceptors = append(streamServerInterceptors, filterNilStreamServerInterceptors(i)...)
 }
 
 func AddUnaryClientInterceptor(ctx context.Context, i ...grpc.UnaryClientInterceptor) {
-	unaryClientInterceptors = append(unaryClientInterceptors, i...)
+	unaryClientInterceptors = append(unaryClientInterceptors, filterNilClientInterceptors(i)...)
 }
 
 func AddStreamClientInterceptor(ctx context.Context, i ...grpc.StreamClientInterceptor) {
-	streamClientInterceptors = append(streamClientInterceptors, i...)
+	streamClientInterceptors = append(streamClientInterceptors, filterNilStreamClientInterceptors(i)...)
 }

You'll also need a filterNilStreamServerInterceptors helper next to the other private filter helpers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@interceptors.go` around lines 139 - 165, The four registration functions
AddUnaryServerInterceptor, AddStreamServerInterceptor,
AddUnaryClientInterceptor, and AddStreamClientInterceptor should filter out nil
interceptors before appending them to their package-level slices (similar to the
existing chain* helpers) so callers don't end up with nil entries in
Default*Interceptors(); implement the filtering by calling existing helper(s) or
add new helpers that remove nil entries and use them in each Add* function, and
create a new private filterNilStreamServerInterceptors helper (matching naming
of the other private filter helpers) to normalize stream-server interceptor
inputs at registration time.

499-507: ⚠️ Potential issue | 🟠 Major

Wrap the stream before invoking the handler to propagate the derived context.

The context modifications at lines 503-505 (trace ID and log context) are not visible to stream handlers because the original grpc.ServerStream is passed unchanged at line 507. This differs from the unary ServerErrorInterceptor, which passes the modified context directly to its handler. Stream handlers that call stream.Context() will not see the trace ID or log context, breaking distributed tracing and logging correlation for stream RPCs.

Implement a wrapper to override Context():

🔁 Pass the derived context into the stream handler
 func ServerErrorStreamInterceptor() grpc.StreamServerInterceptor {
 	return func(srv interface{}, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) (err error) {
 		ctx := stream.Context()
 		ctx = notifier.SetTraceId(ctx)
 		traceID := notifier.GetTraceId(ctx)
 		ctx = loggers.AddToLogContext(ctx, "trace", traceID)
+		stream = &contextServerStream{ServerStream: stream, ctx: ctx}
 		start := time.Now()
 		err = handler(srv, stream)
type contextServerStream struct {
	grpc.ServerStream
	ctx context.Context
}

func (s *contextServerStream) Context() context.Context { return s.ctx }

Note: ResponseTimeLoggingStreamInterceptor has the same issue and should be fixed similarly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@interceptors.go` around lines 499 - 507, ServerErrorStreamInterceptor is
modifying ctx (via notifier.SetTraceId and loggers.AddToLogContext) but still
passes the original grpc.ServerStream to handler so stream.Context() won't see
those changes; create a small wrapper type (e.g., contextServerStream that
embeds grpc.ServerStream and overrides Context() to return the derived ctx) and
pass an instance of that wrapper to handler(srv, wrappedStream) instead of the
original stream; apply the same fix in ResponseTimeLoggingStreamInterceptor so
both interceptors propagate the modified context into stream handlers that call
stream.Context(); reference the functions ServerErrorStreamInterceptor,
ResponseTimeLoggingStreamInterceptor, notifier.SetTraceId, and
loggers.AddToLogContext when locating where to wrap the stream.
🧹 Nitpick comments (1)
chain_test.go (1)

91-98: Assert that the invoker actually runs in the all-nil case.

This test only checks for a nil error. A broken implementation that returns nil without calling invoker would still pass, so it doesn't lock in the pass-through behavior you're relying on here.

🧪 Small test hardening
 func TestChainUnaryClientNilFiltered(t *testing.T) {
 	chain := chainUnaryClient([]grpc.UnaryClientInterceptor{nil, nil})
+	called := false
 	err := chain(context.Background(), "/test", nil, nil, nil, func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, opts ...grpc.CallOption) error {
+		called = true
 		return nil
 	})
 	if err != nil {
 		t.Errorf("expected nil error, got %v", err)
 	}
+	if !called {
+		t.Error("expected invoker to be called directly when all interceptors are nil")
+	}
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@chain_test.go` around lines 91 - 98, The test TestChainUnaryClientNilFiltered
should assert that the provided invoker is actually executed (not just that nil
is returned); modify the test to pass an invoker function to chain (the invoker
parameter in the call to chain) that records a side-effect (e.g., set a local
boolean like invokerCalled or populate reply) and then add an assertion that
this side-effect occurred after invoking
chainUnaryClient([]grpc.UnaryClientInterceptor{nil, nil}); keep the existing
nil-error assertion but also check invokerCalled (or the mutated reply) to
ensure the invoker was run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@interceptors.go`:
- Around line 75-80: Update the lifecycle note on the Prometheus client-side
setter to state it must be called before the first client RPC/dial (not "before
the server starts") and clarify it's only relevant to client-side interceptors;
specifically edit the comment on SetClientInterceptorOptions to reference
"before the first client RPC/dial" and mention it's not safe for concurrent use,
and make the analogous correction for the related comment on
SetServerInterceptorOptions (lines referenced in review) so the README and doc
comments no longer say "before the server starts" for client APIs.

---

Outside diff comments:
In `@interceptors.go`:
- Around line 139-165: The four registration functions
AddUnaryServerInterceptor, AddStreamServerInterceptor,
AddUnaryClientInterceptor, and AddStreamClientInterceptor should filter out nil
interceptors before appending them to their package-level slices (similar to the
existing chain* helpers) so callers don't end up with nil entries in
Default*Interceptors(); implement the filtering by calling existing helper(s) or
add new helpers that remove nil entries and use them in each Add* function, and
create a new private filterNilStreamServerInterceptors helper (matching naming
of the other private filter helpers) to normalize stream-server interceptor
inputs at registration time.
- Around line 499-507: ServerErrorStreamInterceptor is modifying ctx (via
notifier.SetTraceId and loggers.AddToLogContext) but still passes the original
grpc.ServerStream to handler so stream.Context() won't see those changes; create
a small wrapper type (e.g., contextServerStream that embeds grpc.ServerStream
and overrides Context() to return the derived ctx) and pass an instance of that
wrapper to handler(srv, wrappedStream) instead of the original stream; apply the
same fix in ResponseTimeLoggingStreamInterceptor so both interceptors propagate
the modified context into stream handlers that call stream.Context(); reference
the functions ServerErrorStreamInterceptor,
ResponseTimeLoggingStreamInterceptor, notifier.SetTraceId, and
loggers.AddToLogContext when locating where to wrap the stream.

---

Nitpick comments:
In `@chain_test.go`:
- Around line 91-98: The test TestChainUnaryClientNilFiltered should assert that
the provided invoker is actually executed (not just that nil is returned);
modify the test to pass an invoker function to chain (the invoker parameter in
the call to chain) that records a side-effect (e.g., set a local boolean like
invokerCalled or populate reply) and then add an assertion that this side-effect
occurred after invoking chainUnaryClient([]grpc.UnaryClientInterceptor{nil,
nil}); keep the existing nil-error assertion but also check invokerCalled (or
the mutated reply) to ensure the invoker was run.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2417a10d-315e-4730-ae96-e917b889375b

📥 Commits

Reviewing files that changed from the base of the PR and between c70c80a and 91f2403.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (4)
  • README.md
  • chain_test.go
  • go.mod
  • interceptors.go

Comment thread interceptors.go
ankurs added 3 commits March 25, 2026 13:12
- Remove go-grpc-middleware v1 chaining (ChainUnaryServer/Client/StreamClient)
  in favor of local chain helper functions
- Remove grpc_ctxtags — trace ID propagation now done directly via loggers
- Migrate grpc_retry from v1 to go-grpc-middleware/v2/interceptors/retry
- Replace go-grpc-prometheus (archived) with go-grpc-middleware/providers/prometheus
  using instance-based ServerMetrics/ClientMetrics
- Add EnablePrometheusHandlingTimeHistogram() for core to configure histograms
- Keep grpc_opentracing from v1 (public API GRPCClientInterceptor depends on it)

go-grpc-middleware v1 remains as indirect dep for opentracing interceptors only.
…us options

- Add nil interceptor filtering to chainUnaryServer/Client/StreamClient
- Split promInterceptorOpt into srvInterceptorOpts and cltInterceptorOpts
  (WithLabelsFromContext is server-only, exemplar logic differs)
- Add SetServerInterceptorOptions/SetClientInterceptorOptions
- Add chain helper unit tests (order, empty, nil filtering)
- Regenerate README.md
The old go-grpc-prometheus auto-registered metrics via init(). The new
providers/prometheus does not. Add init() to register srvMetrics/cltMetrics
with prometheus.DefaultRegisterer automatically.

- EnablePrometheusHandlingTimeHistogram now unregisters old and registers new
- SetServerMetrics/SetClientMetrics do the same unregister/register swap
- No explicit MustRegister needed by consumers (backward compatible)
- Regenerate README.md
@ankurs ankurs force-pushed the chore/phase4-middleware-migration branch from 91f2403 to 38f4052 Compare March 25, 2026 05:13
@ankurs ankurs changed the title feat: migrate gRPC middleware to v2, replace go-grpc-prometheus Draft: feat: migrate gRPC middleware to v2, replace go-grpc-prometheus Mar 25, 2026
ankurs added 2 commits March 25, 2026 13:16
- Add wrappedServerStream to propagate modified context (trace ID, log
  fields) into stream handlers via stream.Context()
- Fix ServerErrorStreamInterceptor and ResponseTimeLoggingStreamInterceptor
  to pass wrapped stream instead of original
- Fix SetClientInterceptorOptions doc: "before the first client RPC"
- Improve TestChainUnaryClientNilFiltered to assert invoker is called
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread interceptors.go
Comment on lines +54 to +57
func init() {
prometheus.MustRegister(srvMetrics)
prometheus.MustRegister(cltMetrics)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registering srvMetrics/cltMetrics with the global default Prometheus registry in init() creates import-time side effects and can cause startup panics if the application (or another dependency) registers the same gRPC metrics, or if the app uses a custom registry. Consider removing init() registration and instead exposing a Register(reg prometheus.Registerer) helper (or documenting that the caller must register Get*Metrics() with their chosen registry).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The init() auto-registration matches the old go-grpc-prometheus behavior (which also auto-registered in init()). Added doc comment on init() and Get*Metrics() clarifying callers should NOT call MustRegister — they're already registered. SetServerMetrics/SetClientMetrics handle unregister/re-register automatically.

Comment thread interceptors.go
Comment on lines +54 to +57
func init() {
prometheus.MustRegister(srvMetrics)
prometheus.MustRegister(cltMetrics)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description/README mention downstream can call MustRegister on GetServerMetrics/GetClientMetrics, but the package already MustRegisters both collectors in init(). This mismatch is likely to lead to duplicate-registration panics or confusion; please align the public docs/PR description with the actual registration behavior (either caller registers, or the package does).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f632747 — docs now clarify metrics are auto-registered and callers should not MustRegister the result of Get*Metrics().

Comment thread interceptors.go Outdated

// SetClientMetrics sets custom client metrics for gRPC Prometheus instrumentation.
// The new metrics are automatically registered with the default Prometheus registerer.
// Must be called during initialization, before the server starts. Not safe for concurrent use.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SetClientMetrics docstring says it must be called "before the server starts", but this config is for client-side metrics. Please update wording to "before the first client RPC" / "before the client is used" to avoid misleading consumers.

Suggested change
// Must be called during initialization, before the server starts. Not safe for concurrent use.
// Must be called during initialization, before the first client RPC. Not safe for concurrent use.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f632747 — now says "before the first client RPC".

Comment thread interceptors.go Outdated
Comment on lines 324 to 325
srvMetrics.StreamServerInterceptor(srvInterceptorOpts...),
ServerErrorStreamInterceptor(),
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In DefaultStreamInterceptors, Prometheus stream interceptors run before ServerErrorStreamInterceptor sets the trace ID/log context. If srvInterceptorOpts includes WithExemplarFromContext/WithLabelsFromContext, stream metrics won’t be able to extract those values. Consider moving the trace/context setup into an earlier stream interceptor (or reordering ServerErrorStreamInterceptor before srvMetrics.StreamServerInterceptor).

Suggested change
srvMetrics.StreamServerInterceptor(srvInterceptorOpts...),
ServerErrorStreamInterceptor(),
ServerErrorStreamInterceptor(),
srvMetrics.StreamServerInterceptor(srvInterceptorOpts...),

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f632747ServerErrorStreamInterceptor (and ServerErrorInterceptor for unary) now runs before prometheus interceptors so trace ID and log context are available for WithExemplarFromContext/WithLabelsFromContext.

Comment thread README.md Outdated
func SetClientMetrics(m *grpcprom.ClientMetrics)
```

SetClientMetrics sets custom client metrics for gRPC Prometheus instrumentation. The new metrics are automatically registered with the default Prometheus registerer. Must be called during initialization, before the server starts. Not safe for concurrent use.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated docs for SetClientMetrics state it must be called "before the server starts", but this is a client-side configuration. Please update the underlying Go doc comment so the README regenerates with correct guidance (e.g., "before the first client RPC").

Suggested change
SetClientMetrics sets custom client metrics for gRPC Prometheus instrumentation. The new metrics are automatically registered with the default Prometheus registerer. Must be called during initialization, before the server starts. Not safe for concurrent use.
SetClientMetrics sets custom client metrics for gRPC Prometheus instrumentation. The new metrics are automatically registered with the default Prometheus registerer. Must be called during initialization, before the first client RPC. Not safe for concurrent use.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — source doc updated, README regenerated in f632747.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
interceptors.go (1)

445-446: ⚠️ Potential issue | 🟡 Minor

Fix the typo in the public doc comment.

cleint should be client. This also propagates into the generated README and pkg docs.

📝 Proposed doc fix
-// GRPCClientInterceptor is the interceptor that intercepts all cleint requests and adds tracing info to them
+// GRPCClientInterceptor is the interceptor that intercepts all client requests and adds tracing info to them
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@interceptors.go` around lines 445 - 446, Update the public doc comment above
the GRPCClientInterceptor function to fix the typo "cleint" -> "client" so
generated README/pkg docs are correct; locate the comment immediately preceding
the GRPCClientInterceptor(options ...grpc_opentracing.Option)
grpc.UnaryClientInterceptor declaration and replace the misspelled word while
keeping the rest of the comment intact.
🧹 Nitpick comments (1)
interceptors.go (1)

603-712: Move nil filtering up to the exported Default*Interceptors builders.

Right now only the locally chained paths normalize nils. The slice-returning public APIs can still surface nil entries, so callers that use those exports directly get different behavior than DefaultClientInterceptor, DefaultClientStreamInterceptor, and DoHTTPtoGRPC.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@interceptors.go` around lines 603 - 712, The public builder functions that
return interceptor slices should normalize nils at construction so callers never
receive nil entries; update the exported builders (e.g.,
DefaultClientInterceptor, DefaultClientStreamInterceptor, and DoHTTPtoGRPC) to
call the existing helpers filterNilClientInterceptors,
filterNilStreamClientInterceptors, and filterNilServerInterceptors (as
appropriate) on the slices before returning them, ensuring the returned slices
have nils removed; you can then rely on or remove the duplicate filtering inside
chainUnaryClient/chainStreamClient/chainUnaryServer as desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@interceptors.go`:
- Around line 108-110: The doc comment for SetClientMetrics incorrectly says
“before the server starts”; update the lifecycle note to reflect this is a
client-side API by changing the sentence to something like: "Must be called
during client initialization, before creating any gRPC clients; not safe for
concurrent use." Locate the comment above the SetClientMetrics declaration and
replace the misleading "before the server starts" phrasing with the corrected
client-focused wording.

---

Outside diff comments:
In `@interceptors.go`:
- Around line 445-446: Update the public doc comment above the
GRPCClientInterceptor function to fix the typo "cleint" -> "client" so generated
README/pkg docs are correct; locate the comment immediately preceding the
GRPCClientInterceptor(options ...grpc_opentracing.Option)
grpc.UnaryClientInterceptor declaration and replace the misspelled word while
keeping the rest of the comment intact.

---

Nitpick comments:
In `@interceptors.go`:
- Around line 603-712: The public builder functions that return interceptor
slices should normalize nils at construction so callers never receive nil
entries; update the exported builders (e.g., DefaultClientInterceptor,
DefaultClientStreamInterceptor, and DoHTTPtoGRPC) to call the existing helpers
filterNilClientInterceptors, filterNilStreamClientInterceptors, and
filterNilServerInterceptors (as appropriate) on the slices before returning
them, ensuring the returned slices have nils removed; you can then rely on or
remove the duplicate filtering inside
chainUnaryClient/chainStreamClient/chainUnaryServer as desired.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7d2d2a2f-1ff1-42c4-8b22-5a2486b2d21f

📥 Commits

Reviewing files that changed from the base of the PR and between 91f2403 and 48b8e47.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (4)
  • README.md
  • chain_test.go
  • go.mod
  • interceptors.go
✅ Files skipped from review due to trivial changes (2)
  • go.mod
  • chain_test.go

Comment thread interceptors.go Outdated
ankurs added 2 commits March 25, 2026 13:50
- Fix SetClientMetrics doc: "before the first client RPC" (not server)
- Reorder interceptors: ServerErrorInterceptor before prometheus so
  trace ID/log context is available for WithExemplarFromContext/WithLabelsFromContext
- Clarify init() auto-registers metrics — callers should not MustRegister
- Update GetServerMetrics/GetClientMetrics docs accordingly
- Regenerate README
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
README.md (1)

181-188: Clarify interaction with SetServerMetrics.

The documentation states that EnablePrometheusHandlingTimeHistogram "re-creates the server metrics," but it's unclear how this interacts with SetServerMetrics (which allows setting custom server metrics). Users may be confused about:

  • If SetServerMetrics is called first, will EnablePrometheusHandlingTimeHistogram discard the custom metrics?
  • If EnablePrometheusHandlingTimeHistogram is called first, will SetServerMetrics lose the histogram configuration?
  • Are these functions mutually exclusive?

Consider adding guidance on the proper ordering or whether these functions can be used together.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 181 - 188, Clarify README behavior: update the docs
for EnablePrometheusHandlingTimeHistogram to explain its interaction with
SetServerMetrics by stating whether EnablePrometheusHandlingTimeHistogram
mutates or replaces the global/server metrics and which call wins when both are
used; explicitly document the recommended ordering (e.g., call
EnablePrometheusHandlingTimeHistogram first to configure histogram buckets, then
call SetServerMetrics with your custom metrics that will include the histogram,
or conversely state that SetServerMetrics will be overwritten) and note if they
are mutually exclusive or not, referencing the function names
EnablePrometheusHandlingTimeHistogram and SetServerMetrics so users know exactly
how to sequence calls during initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@README.md`:
- Around line 181-188: Clarify README behavior: update the docs for
EnablePrometheusHandlingTimeHistogram to explain its interaction with
SetServerMetrics by stating whether EnablePrometheusHandlingTimeHistogram
mutates or replaces the global/server metrics and which call wins when both are
used; explicitly document the recommended ordering (e.g., call
EnablePrometheusHandlingTimeHistogram first to configure histogram buckets, then
call SetServerMetrics with your custom metrics that will include the histogram,
or conversely state that SetServerMetrics will be overwritten) and note if they
are mutually exclusive or not, referencing the function names
EnablePrometheusHandlingTimeHistogram and SetServerMetrics so users know exactly
how to sequence calls during initialization.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c298b315-3cda-4852-9e5d-586dd5adc5fa

📥 Commits

Reviewing files that changed from the base of the PR and between 48b8e47 and 4d18b9e.

📒 Files selected for processing (2)
  • README.md
  • interceptors.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • interceptors.go

@ankurs ankurs closed this Mar 25, 2026
@ankurs ankurs deleted the chore/phase4-middleware-migration branch March 25, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants