feat: implement MetricsGet to support V1 and V2 data engines in ProxyOps #899
Conversation
WalkthroughThe changes update the metrics retrieval functionality across both client and proxy components. The client’s Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyClient
participant Proxy
participant V1Ops
participant V2Ops
Client->>ProxyClient: MetricsGet(dataEngine, engineName, volumeName, serviceAddress)
ProxyClient->>Proxy: Create and validate request with dataEngine
Proxy->>Proxy: Validate dataEngine in ops map
alt Data engine is V1
Proxy->>V1Ops: MetricsGet(request)
V1Ops-->>Proxy: Return metrics
else Data engine is V2
Proxy->>V2Ops: MetricsGet(request)
V2Ops-->>Proxy: Return metrics
end
Proxy-->>ProxyClient: Return aggregated metrics
ProxyClient-->>Client: Metrics
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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
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 (3)
pkg/client/proxy_metrics.go (2)
9-9: Parameter expansion in function signature.Adding
dataEngineto theMetricsGetparameters is a coherent way to support multiple engine variants. Consider adding doc comments for clarity, indicating the valid values and usage of thedataEngineparameter.
20-23: Data engine validation.Great job verifying that the provided
dataEnginecorresponds to a valid enum. IfgetDataEngine(dataEngine)might be case-sensitive, consider normalizing or clarifying expectations to prevent unexpected user input errors.pkg/proxy/metrics.go (1)
54-76: Implementation for V2 engine metrics retrieval.Fetching a specialized SPDK client for the engine address aligns well with the V2 approach. The error handling with
grpcstatus.Errorf(grpccodes.Internal, ...)is also consistent. Consider caching or pooling SPDK clients if performance or connection overhead becomes a concern under heavy load.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/client/proxy_metrics.go(2 hunks)pkg/proxy/metrics.go(2 hunks)pkg/proxy/proxy.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/proxy/proxy.go (1)
55-56: New interface method for metrics retrieval added.Including a
MetricsGetmethod inProxyOpsaligns with the existing design pattern, ensuring that metrics retrieval is formally part of the proxy operations. Please verify that all concrete implementations of this interface are tested to maintain coverage for your new feature.pkg/client/proxy_metrics.go (2)
4-6: Import changes appear correct.The addition of
"fmt"and"github.com/pkg/errors"is consistent with the usage below. No concerns here.
14-14: New map entry fordataEngine.This entry harmonizes with the existing parameter validations. Looks good.
pkg/proxy/metrics.go (3)
15-20: Enhanced logging with more context.Including
dataEngineand other fields provides valuable insights for debugging. Consider adding log levels or additional correlation information if deeper tracing is needed.
23-28: Data engine routing check.Returning
Unimplementedwhenreq.DataEngineis missing in the operations map is a clear, explicit way to indicate unsupported engines. This is well-handled and maintains a clean architecture.
30-52: Implementation for V1 engine metrics retrieval.Creating a new controller client, fetching metrics, and closing it is straightforward. Ensure concurrency scenarios are handled properly if multiple requests come in at once, though the current approach of opening/closing clients on demand is typically acceptable. Looks good overall.
longhorn 10472 Signed-off-by: jinhong.kim0 <[email protected]>
Which issue(s) this PR fixes:
longhorn/longhorn#10472
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context