Decouple MetricsMiddleware from downstream handlers#206
Conversation
Remove ls-real-model-name optimization. Within proxyOAIHandler the request body's bytes are required for various rewriting features anyways. This negated any benefits from trying not to parse it twice.
WalkthroughThe error handling and model name resolution logic in the proxy middleware and handler were refactored. The middleware now aborts the Gin context after sending error responses and no longer sets the real model name in the context. The handler now independently parses the request body to extract and validate the model name, removing its dependency on middleware context keys. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/metrics_middleware.go (1)
15-54: Consider consolidating model validation logic.While the current approach works correctly, both the middleware and handler now perform identical model validation steps:
- Parse request body and extract model field
- Validate model exists in configuration
- Look up real model name
This duplication could lead to maintenance issues if validation logic needs to change. Consider extracting the validation into a shared helper function that both can use.
Example approach:
func (pm *ProxyManager) validateAndExtractModel(bodyBytes []byte) (requestedModel, realModelName string, err error) { requestedModel = gjson.GetBytes(bodyBytes, "model").String() if requestedModel == "" { return "", "", fmt.Errorf("missing or invalid 'model' key") } realModelName, found := pm.config.RealModelName(requestedModel) if !found { return "", "", fmt.Errorf("could not find real modelID for %s", requestedModel) } return requestedModel, realModelName, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/metrics_middleware.go(1 hunks)proxy/proxymanager.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
proxy/metrics_middleware.go (1)
proxy/proxymanager_test.go (2)
TestProxyManager_MiddlewareWritesMetrics_Streaming(724-757)TestProxyManager_MiddlewareWritesMetrics_NonStreaming(689-722)
🔇 Additional comments (3)
proxy/metrics_middleware.go (1)
20-20: LGTM: Proper middleware error handling with c.Abort().The addition of
c.Abort()calls after error responses correctly prevents downstream handlers from executing when the middleware encounters errors. This is essential middleware behavior and aligns with the decoupling objective.Also applies to: 28-28, 35-35
proxy/proxymanager.go (2)
17-17: LGTM: Added gjson import for request body parsing.The addition of the gjson import is necessary for the new model validation logic in the handler.
371-381: LGTM: Handler now independently validates model.The handler correctly extracts and validates the model from the request body, removing its dependency on the middleware's context key. The validation logic is identical to what the middleware performs, ensuring consistency.
However, note that this creates duplicate parsing and validation between middleware and handler (see comment on middleware file).
1 participant
Remove ls-real-model-name optimization. Within proxyOAIHandler the request body's bytes are required for various rewriting features anyways. This negated any benefits from trying not to parse it twice.
Summary by CodeRabbit