Expose metrics for connected node versions #698
Conversation
WalkthroughThis pull request updates several authentication-related components by modifying method signatures to include a new cancellation function type, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as AuthInterceptor
participant V as Verifier
participant M as Metrics/Tracker
C->>A: Send request with token
A->>V: Verify(token)
V-->>A: (nodeId, cancel, error)
alt Verification Failed
A->>C: Return error
note right of A: defer cancel() called
else Verification Succeeded
A->>M: Emit new connection request (by version)
M-->>A: Tracker opened (increment metric)
A->>C: Process request
note right of A: defer cancel() to close tracker
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/authn/claims.go (1)
41-65:Details
❓ Verification inconclusive
Validate the correct usage of the returned CloseFunc.
- The approach of returning
emptyCloseon errors avoids nil checks in callers. Good use of a no-op function.- Creating and opening a tracker, then returning a closing function is a solid pattern for pairing resource allocation and deallocation.
- Confirm that all callers of
ValidateVersionClaimIsCompatiblecorrectly invoke the returnedCloseFuncto prevent leaking resources.Consider adding a test to ensure that when an error is returned, subsequent calls on the
CloseFuncdo not cause issues. This can surface any edge cases around resource management.
🏁 Script executed:
#!/bin/bash # Description: Search for all usage sites of ValidateVersionClaimIsCompatible and confirm they invoke the returned CloseFunc. rg -A 5 "ValidateVersionClaimIsCompatible(.*)"Length of output: 791
Action Required: Verify that all call sites properly invoke the returned CloseFunc
- The implementation in
pkg/authn/claims.goproperly creates a cleanup function—even on error—using a no-op (emptyClose), which is a good design choice to avoid nil checks.- The pattern of resource allocation (tracker creation/opening) paired with returning a closure for cleanup is sound.
- However, our grep results show that in
pkg/authn/verifier.gothe call toValidateVersionClaimIsCompatibleis simply returned, meaning the actual invocation of the CloseFunc depends on the caller further up the chain. Please verify that every consumer of this API eventually calls the returned CloseFunc to prevent resource leaks.- Additionally, consider adding a test case to cover scenarios where an error is returned—ensure that subsequent invocations of the CloseFunc (including repeated calls on the no-op function) do not cause issues.
pkg/metrics/api.go (1)
26-26: Fix incorrect help text for the counter metric.The help text for
apiNodeConnectionRequestsByVersionCounterincorrectly states "Number of incoming node connections by version" which is the same as the gauge metric. This should be updated to reflect that it counts connection requests rather than active connections.- Help: "Number of incoming node connections by version", + Help: "Number of incoming node connection requests by version",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/authn/claims.go(2 hunks)pkg/authn/claims_test.go(3 hunks)pkg/authn/interface.go(1 hunks)pkg/authn/verifier.go(5 hunks)pkg/authn/verifier_test.go(8 hunks)pkg/interceptors/server/auth.go(2 hunks)pkg/interceptors/server/auth_test.go(4 hunks)pkg/metrics/api.go(2 hunks)pkg/metrics/metrics.go(1 hunks)pkg/mocks/authn/mock_JWTVerifier.go(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pkg/mocks/authn/mock_JWTVerifier.go (1)
pkg/authn/interface.go (1)
CloseFunc(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Code Review
🔇 Additional comments (30)
pkg/authn/interface.go (2)
3-3: Introducing a dedicated CloseFunc is a clear approach.
By defining a no-op function type for resource cleanup, you reduce the risk of nil-pointer checks for callers. Just ensure that the implementation of any returned CloseFunc fully closes or releases associated resources.
6-6:Details
✅ Verification successful
Be mindful of downstream breakage from the changed signature.
ChangingVerify(tokenString string) (uint32, CloseFunc, error)may break existing callers. Verify that each invocation ofVerifyproperly handles the newCloseFuncreturn.Run this script to locate and confirm that all calls handle the
CloseFunc:
🏁 Script executed:
#!/bin/bash rg -A 3 $'Verify\\(.*\\)'Length of output: 6602
Downstream Usage Verified: Updated Verify Signature Handled Correctly
All invocations of
Verifyacross the codebase have been updated to properly handle the newCloseFuncreturn value. The grep results confirm that:
- In the mocks (e.g.,
pkg/mocks/authn/mock_JWTVerifier.go), the return value now supplies a placeholder (func() {}) forCloseFunc.- In test files (e.g.,
pkg/interceptors/server/auth_test.go,pkg/authn/verifier_test.go, andpkg/authn/claims_test.go), callers capturecanceland appropriately defer its execution.- The changes in
pkg/authn/interface.goand corresponding production code (e.g.,pkg/interceptors/server/auth.go) consistently unpack and handle the new signature.It appears that no additional modifications are needed to manage the
CloseFuncreturn, and downstream breakage risks have been mitigated.pkg/metrics/metrics.go (1)
87-88: New version-based metrics look good.
RegisteringapiIncomingNodeConnectionByVersionGaugeandapiNodeConnectionRequestsByVersionCounterwill help track node versions effectively. Ensure you’ve documented these metrics in your monitoring guide for easy reference.pkg/authn/claims.go (1)
6-7: New metrics import is consistent with the code changes.
Importingmetricsis appropriate now that the function logs version usage and instantiates a connection tracker.pkg/interceptors/server/auth_test.go (4)
39-39: LGTM: Updated mock expectation to match new method signature.The mock expectation now includes an empty function as the second return value to match the updated
Verifymethod signature.
74-74: LGTM: Consistency with the new method signature.The mock expectation for the invalid token case correctly includes the new function parameter in the return values.
134-134: LGTM: Updated mock expectation in stream interceptor test.The mock expectations for the stream interceptor tests have been correctly updated to match the new method signature.
159-159: LGTM: Consistent implementation across all test cases.The invalid token case for stream interceptor has been properly updated to match the new method signature.
pkg/authn/claims_test.go (3)
57-58: LGTM: Properly handling the new CloseFunc return value.Good implementation of capturing and deferring the cancellation function to ensure resources are properly cleaned up after verification.
120-121: LGTM: Consistent resource cleanup pattern.The test correctly captures and defers the cancel function, maintaining a consistent pattern across all verification calls.
206-207: LGTM: Proper resource cleanup in validation tests.The test correctly handles the new cancel function return value with appropriate deferred execution.
pkg/interceptors/server/auth.go (4)
98-99: LGTM: Updated Unary interceptor to handle the new return value.The Unary interceptor now correctly extracts the cancellation function from the Verify method call.
106-106: LGTM: Proper resource cleanup with defer.Good practice to defer the cancellation function to ensure resources are properly cleaned up after verification.
129-130: LGTM: Updated Stream interceptor for consistent behavior.The Stream interceptor has been properly updated to handle the new return value from the Verify method.
137-138: LGTM: Consistent resource cleanup pattern in Stream interceptor.Matching the pattern in the Unary interceptor, the cancellation function is properly deferred for cleanup.
pkg/authn/verifier.go (9)
26-26: LGTM: Useful empty closure function.This empty function serves as a placeholder for cases where a CloseFunc is required but no actual cleanup is needed.
46-46: LGTM: Updated method signature to include CloseFunc.The Verify method now returns a CloseFunc as its second return value, consistent with the PR's objective of exposing metrics for connected node versions.
55-56: LGTM: Consistent error handling with emptyClose.All error paths now return the emptyClose function alongside the error, ensuring that callers can safely defer the returned function without nil checks.
Also applies to: 58-59, 62-63
65-68: LGTM: Propagating the CloseFunc from validateClaims.The method now correctly propagates both the CloseFunc and error from validateClaims, maintaining proper resource management across the validation chain.
70-73: LGTM: Consistent error handling for subject node ID.The error path for getSubjectNodeId now correctly returns emptyClose as the second return value.
75-75: LGTM: Updated return values to include the CloseFunc.The success path now returns the nodeId and the CloseFunc obtained from validateClaims.
118-122: LGTM: Updated validateClaims to return a CloseFunc.The validateClaims method signature has been updated to return a CloseFunc, with emptyClose returned for type-related errors.
126-127: LGTM: Consistent error handling for invalid tokens.This is consistent with the other error paths, returning emptyClose alongside the error.
144-145: LGTM: Simplified return in getSubjectNodeId.The method now directly returns nodeId without unnecessary type conversion, improving code readability.
pkg/authn/verifier_test.go (1)
81-82: Good implementation of cancellation pattern.The addition of a
cancelfunction return value fromVerifyand deferring it immediately is a good practice for resource cleanup. This pattern is consistently implemented across all test cases.pkg/metrics/api.go (3)
15-29: The new metrics for tracking node connections by version look good.Adding these metrics will provide valuable insights into the distribution of versions connecting to the system.
51-69: Well-designed connection tracker implementation.The
IncomingConnectionTrackerimplementation follows good design patterns by encapsulating the version-specific metrics operations. The Open/Close methods make the code more readable and maintainable.
71-74: Good helper function for emitting connection request metrics.The
EmitNewConnectionRequestVersionfunction provides a clean API for incrementing the connection request counter.pkg/mocks/authn/mock_JWTVerifier.go (2)
24-58: Updated mock implementation for the new Verify signature.The mock has been correctly updated to handle the new
CloseFuncreturn type in theVerifymethod. The implementation handles all the different return value scenarios appropriately.
78-86: Helper method signatures updated consistently.The
ReturnandRunAndReturnmethods have been correctly updated to match the new signature of theVerifymethod.
Add version tracking metrics to
ValidateVersionClaimIsCompatiblemethod inClaimValidatorto expose metrics for connected node versionsImplements connection metrics tracking by adding new Prometheus metrics and modifying the authentication flow. The changes center around:
JWTVerifierinterface in interface.go to return aCloseFuncfor connection cleanupValidateVersionClaimIsCompatiblein claims.go to track metrics usingmetrics.EmitNewConnectionRequestVersion📍Where to Start
Start with the
ValidateVersionClaimIsCompatiblemethod in claims.go which implements the core version tracking functionality and returns the newCloseFunc.Macroscope summarized 397e495.
Summary by CodeRabbit
New Features
Tests