refactor: extract request interceptor registry#19828
Draft
parametalol wants to merge 6 commits intomichael/fix-grpc-ua-orderfrom
Draft
refactor: extract request interceptor registry#19828parametalol wants to merge 6 commits intomichael/fix-grpc-ua-orderfrom
parametalol wants to merge 6 commits intomichael/fix-grpc-ua-orderfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
pkg/grpc/common/requestinterceptor/interceptor.gothe helpersetHeaderis currently unused and can be removed to keep the new interceptor package minimal and focused. - In
RequestInterceptor.dispatch, handlers are called while holding the read lock; if handlers ever add/remove other handlers or perform long-running work, it may be safer to snapshot the handler funcs under lock and then invoke them after releasing the lock to avoid potential contention or deadlock patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pkg/grpc/common/requestinterceptor/interceptor.go` the helper `setHeader` is currently unused and can be removed to keep the new interceptor package minimal and focused.
- In `RequestInterceptor.dispatch`, handlers are called while holding the read lock; if handlers ever add/remove other handlers or perform long-running work, it may be safer to snapshot the handler funcs under lock and then invoke them after releasing the lock to avoid potential contention or deadlock patterns.
## Individual Comments
### Comment 1
<location path="pkg/grpc/common/requestinterceptor/interceptor.go" line_range="56-59" />
<code_context>
+ delete(ri.handlers, name)
+}
+
+func (ri *RequestInterceptor) dispatch(rp *RequestParams) {
+ ri.mu.RLock()
+ defer ri.mu.RUnlock()
+ for _, h := range ri.handlers {
+ h(rp)
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling handlers while holding the read lock can deadlock if a handler modifies handlers (Add/Remove).
In `dispatch`, the RWMutex is held with `RLock` while invoking handlers. If any handler calls `ri.Add`, `ri.Remove`, or any method that takes the write lock, this can deadlock (handler runs under `RLock` and then tries to take `Lock`). Instead, snapshot the handlers under the read lock, release the lock, and then invoke the snapshot:
```go
func (ri *RequestInterceptor) dispatch(rp *RequestParams) {
ri.mu.RLock()
handlers := make([]RequestHandler, 0, len(ri.handlers))
for _, h := range ri.handlers {
handlers = append(handlers, h)
}
ri.mu.RUnlock()
for _, h := range handlers {
h(rp)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request refactors telemetry and request interception infrastructure by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/grpc/common/requestinterceptor/interceptor_test.go (1)
73-84: Strengthen remove test with behavioral assertion (not only internal state).
TestRequestInterceptor_removecurrently validateshasHandlers()but does not prove that dispatch is actually skipped on interceptor invocation afterRemove. Consider asserting non-invocation via a real unary call.Proposed test strengthening
func TestRequestInterceptor_remove(t *testing.T) { ri := NewRequestInterceptor() var called atomic.Bool ri.Add("temp", func(rp *RequestParams) { called.Store(true) }) ri.Remove("temp") // With no handlers, dispatch should be skipped entirely. assert.False(t, ri.hasHandlers()) + + interceptor := ri.UnaryServerInterceptor() + _, err := interceptor(context.Background(), "req", + &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"}, + func(ctx context.Context, req any) (any, error) { + return "ok", nil + }) + require.NoError(t, err) + assert.False(t, called.Load()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/grpc/common/requestinterceptor/interceptor_test.go` around lines 73 - 84, The test TestRequestInterceptor_remove only checks internal state; update it to assert behavior by invoking the interceptor after Remove and ensuring the handler is not called: create the interceptor via NewRequestInterceptor, Add a handler that sets an atomic flag, Remove that handler, then call the public invocation method used elsewhere in tests (the interceptor's dispatch/invocation method - same one other tests use to trigger handlers) with a dummy RequestParams and assert the atomic flag remains false; keep the existing hasHandlers() assertion as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/grpc/common/requestinterceptor/interceptor.go`:
- Around line 100-104: The code currently defaults status to 0 when
wrappedWriter.GetStatusCode() is nil; change this to use HTTP default 200 by
initializing status to http.StatusOK (or 200) and only override it if sptr !=
nil (keep the wrappedWriter.GetStatusCode() check), and ensure the file imports
net/http if not already; update the block around ri.hasHandlers(),
wrappedWriter.GetStatusCode(), and the status variable accordingly so
telemetry/matching sees 200 for handlers that never explicitly write a status.
- Around line 56-61: The dispatch method runs each handler while holding
ri.mu.RLock(), which can deadlock if a handler calls Add/Remove (which acquire
the write lock); fix by copying ri.handlers into a local slice while holding the
read lock inside RequestInterceptor.dispatch, then release the lock and iterate
over the copied slice calling each handler (so the lock is not held during
handler execution and Add/Remove can proceed).
---
Nitpick comments:
In `@pkg/grpc/common/requestinterceptor/interceptor_test.go`:
- Around line 73-84: The test TestRequestInterceptor_remove only checks internal
state; update it to assert behavior by invoking the interceptor after Remove and
ensuring the handler is not called: create the interceptor via
NewRequestInterceptor, Add a handler that sets an atomic flag, Remove that
handler, then call the public invocation method used elsewhere in tests (the
interceptor's dispatch/invocation method - same one other tests use to trigger
handlers) with a dummy RequestParams and assert the atomic flag remains false;
keep the existing hasHandlers() assertion as well.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a6fe9b9f-4eef-456e-82db-74d7c64d74e3
📒 Files selected for processing (25)
central/main.gocentral/telemetry/centralclient/client.gocentral/telemetry/centralclient/interceptors.gocentral/telemetry/centralclient/interceptors_test.gopkg/clientprofile/campaign.gopkg/clientprofile/campaign_test.gopkg/clientprofile/headers.gopkg/clientprofile/headers_test.gopkg/grpc/common/requestinterceptor/interceptor.gopkg/grpc/common/requestinterceptor/interceptor_test.gopkg/grpc/common/requestinterceptor/request_details_test.gopkg/grpc/common/requestinterceptor/request_params.gopkg/grpc/requestinfo/requestinfo_test.gopkg/telemetry/phonehome/client.gopkg/telemetry/phonehome/client_test.gopkg/telemetry/phonehome/download_config.gopkg/telemetry/phonehome/download_config_test.gopkg/telemetry/phonehome/examples_test.gopkg/telemetry/phonehome/interceptor.gopkg/telemetry/phonehome/interceptor_test.gopkg/telemetry/phonehome/request_params.gopkg/telemetry/phonehome/request_params_test.goroxctl/common/client.goroxctl/common/client_test.gosensor/common/centralproxy/authorizer.go
💤 Files with no reviewable changes (3)
- pkg/telemetry/phonehome/interceptor.go
- pkg/telemetry/phonehome/interceptor_test.go
- pkg/telemetry/phonehome/request_params_test.go
Contributor
🚀 Build Images ReadyImages are ready for commit 248b8a0. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-566-g248b8a0427 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## michael/fix-grpc-ua-order #19828 +/- ##
=============================================================
+ Coverage 49.60% 49.61% +0.01%
=============================================================
Files 2763 2764 +1
Lines 208335 208298 -37
=============================================================
+ Hits 103337 103349 +12
+ Misses 97331 97283 -48
+ Partials 7667 7666 -1
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:
|
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.
Description
Extract the generic request interceptor infrastructure from
pkg/telemetry/phonehomeinto dedicated packages:pkg/clientprofile/— campaign matching logic (APICallCampaign,Headers,GlobMap, glob-based header matching)pkg/grpc/common/requestinterceptor/— genericRequestInterceptorregistry that computesRequestParamsonce per API request and fans out to registered handlers; ownsRequestParams,NewHeaders, andGetGRPCRequestBodyphonehomeretains only the telemetry-specificInterceptortype andClientmethods. Type aliases (phonehome.RequestParams,clientprofile.RequestParams) keep the diff minimal for consumers.The
RequestParams.Headersfield is now plainhttp.Header, breaking the dependency fromrequestinterceptoronclientprofile. Campaign code converts toHeaders(rp.Headers)for glob matching.Also includes fixes from the
michael/fix-grpc-ua-orderbase branch: correct User-Agent value ordering (HTTP client first, gRPC gateway second) and deduplication.User-facing documentation
Testing and quality
Automated testing
How I validated my change
central/...,roxctl/...,sensor/...