Conversation
There was a problem hiding this comment.
Pull request overview
Adds per-config/per-rule dataplane counter export for the forward module and refactors controlplane FFI dataplane config access behind interfaces.
Changes:
- Extend
forwardcontrolplane with aGetMetricsgRPC method and metric collection logic based on dataplane counters. - Introduce
ffi.DPConfig/ related interfaces and switchDPConfig()APIs to return the interface rather than concrete pointers. - Update dependent code (gateway inspect service, balancer agent) to use the new
ffi.DPConfiginterface.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/forward/controlplane/service_test.go | Updates mock backend to satisfy new Backend interface (currently with a compile-breaking return type). |
| modules/forward/controlplane/service.go | Adds Backend.Agent() requirement and implements GetMetrics RPC handler. |
| modules/forward/controlplane/metrics.go | New forward metrics collection (module counter aggregation into commonpb.Metric). |
| modules/forward/controlplane/forwardpb/forward.proto | Adds GetMetrics RPC + request/response messages and imports metric proto. |
| modules/forward/controlplane/backend.go | Implements new Backend.Agent() accessor for real backend. |
| modules/balancer/agent/go/ffi/agent.go | Adjusts DPConfig() return type to match new interface-based FFI API. |
| controlplane/internal/gateway/inspect_service.go | Switches helper methods to accept ffi.DPConfig interface instead of pointer type. |
| controlplane/ffi/shm.go | Introduces DPConfig/observer/counter aggregator interfaces and renames concrete impl to DPConfigImpl. |
| controlplane/ffi/agent.go | Introduces FFIAgent and related agent capability interfaces; updates Agent.DPConfig() signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (m *mockBackend) Agent() forward.FFIAgent { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
mockBackend's Agent() method has the wrong return type. The forward.Backend interface now requires Agent() ffi.FFIAgent (from github.com/yanet-platform/yanet2/controlplane/ffi), but this mock returns forward.FFIAgent (which doesn't exist in the forward controlplane package), so the test won't compile. Update the signature to return ffi.FFIAgent and add the appropriate import in the test.
| result = append(result, | ||
| makeCounter(counter.Name+"_packets", cntPackets, baseLabels...), | ||
| makeCounter(counter.Name+"_bytes", bytes, baseLabels...), |
There was a problem hiding this comment.
Metric names are being generated dynamically from counter.Name (e.g. counter.Name+"_packets"). This can create unbounded metric name cardinality and makes it hard to query/aggregate across configs; it also diverges from the established pattern used by ACL metrics (see modules/acl/controlplane/metrics.go:216-220) which uses stable metric names and puts the counter name into a counter label. Consider switching to fixed names like forward_rule_packets / forward_rule_bytes (or similar) and adding a counter label with counter.Name (while keeping the existing base labels).
| result = append(result, | |
| makeCounter(counter.Name+"_packets", cntPackets, baseLabels...), | |
| makeCounter(counter.Name+"_bytes", bytes, baseLabels...), | |
| labels := append([]*commonpb.Label{}, baseLabels...) | |
| labels = append(labels, &commonpb.Label{Name: "counter", Value: counter.Name}) | |
| result = append(result, | |
| makeCounter("forward_rule_packets", cntPackets, labels...), | |
| makeCounter("forward_rule_bytes", bytes, labels...), |
| func (m *ForwardService) GetMetrics( | ||
| ctx context.Context, | ||
| req *forwardpb.GetMetricsRequest, | ||
| ) (*forwardpb.GetMetricsResponse, error) { | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
|
|
||
| metrics, err := m.collectMetrics() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &forwardpb.GetMetricsResponse{ | ||
| Metrics: metrics, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The new GetMetrics handler introduces non-trivial behavior (reading dataplane counters and encoding them into commonpb.Metric), but there are no tests exercising it. Since this module already has Go tests, adding a unit/integration test that calls GetMetrics/Metrics and asserts emitted metric names/labels/values (e.g., using a mock ffi.DPConfig / Backend.Agent() or the existing forward test harness) would help prevent regressions.
|
|
||
| func NewDPConfigFromRaw(ptr unsafe.Pointer) *DPConfig { | ||
| return &DPConfig{ptr: (*C.struct_dp_config)(ptr)} | ||
| func NewDPConfigFromRaw(ptr unsafe.Pointer) *DPConfigImpl { |
There was a problem hiding this comment.
Now that DPConfig is an exported interface, NewDPConfigFromRaw returning *DPConfigImpl (and DPConfigImpl being exported) leaks the implementation type into the public API. To keep the interface abstraction consistent for callers and reduce surface area, consider making the implementation type unexported and/or changing NewDPConfigFromRaw to return DPConfig instead of *DPConfigImpl.
| func NewDPConfigFromRaw(ptr unsafe.Pointer) *DPConfigImpl { | |
| func NewDPConfigFromRaw(ptr unsafe.Pointer) DPConfig { |
No description provided.