[client] Add local Prometheus metrics endpoint - #6689
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughAdds a local Prometheus ChangesLocal metrics feature
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The PR adds an opt-in local metrics endpoint and related configuration without any identified merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CLI
participant Server
participant LocalMetrics
participant ClientEngine
participant Prometheus
CLI->>Server: Configure local metrics
Server->>LocalMetrics: Reconcile enabled state and address
LocalMetrics->>ClientEngine: Request Prometheus gatherer
Prometheus->>LocalMetrics: GET /metrics
LocalMetrics->>ClientEngine: Gather engine metrics
LocalMetrics-->>Prometheus: Return local and engine metrics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the feature, implementation scope, tests, and documentation. However, the required issue ticket or approved discussion link is missing, and the checklist contains a contradictory claim that the change does not introduce a new feature or modify CLI flags.
✨ Finishing Touches 💡 1📝 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 |
![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 (1)
client/internal/localmetrics/localmetrics.go (1)
60-63: 🔒 Security & Privacy | 🔵 TrivialConsider warning when binding to a non-loopback address.
The endpoint exposes peer FQDNs, latency, and connectivity state without authentication. Default is loopback-only, but
addris fully user-controlled; a log warning (or explicit opt-in) when the host isn't loopback would help users avoid accidental exposure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/localmetrics/localmetrics.go` around lines 60 - 63, The local metrics endpoint in Manager.Reconcile can bind to any user-supplied addr, which may accidentally expose sensitive peer and connectivity data. Update Reconcile to detect when addr is not loopback after defaulting to DefaultListenAddress, and emit a clear warning (or require explicit opt-in) before starting the listener. Keep the check near the address selection logic so the behavior is obvious and easy to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/localmetrics/localmetrics.go`:
- Line 94: The local metrics server created in `Start` uses `http.Server`
without any timeout protections, leaving it vulnerable to slow client
connections. Update the `srv := &http.Server{...}` construction to set
appropriate `ReadTimeout`, `ReadHeaderTimeout`, `WriteTimeout`, and
`IdleTimeout` values, keeping the existing `Addr` and `Handler` behavior intact.
Use the `Start` function and the `http.Server` setup as the place to apply these
limits.
---
Nitpick comments:
In `@client/internal/localmetrics/localmetrics.go`:
- Around line 60-63: The local metrics endpoint in Manager.Reconcile can bind to
any user-supplied addr, which may accidentally expose sensitive peer and
connectivity data. Update Reconcile to detect when addr is not loopback after
defaulting to DefaultListenAddress, and emit a clear warning (or require
explicit opt-in) before starting the listener. Keep the check near the address
selection logic so the behavior is obvious and easy to maintain.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f956574-47e5-405e-a5e6-4581c2baf2ed
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
client/cmd/root.goclient/cmd/up.goclient/internal/localmetrics/localmetrics.goclient/internal/localmetrics/localmetrics_test.goclient/internal/metrics/influxdb.goclient/internal/metrics/metrics.goclient/internal/metrics/metrics_default.goclient/internal/metrics/prometheus.goclient/internal/peer/status.goclient/internal/profilemanager/config.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.gogo.modinfrastructure_files/observability/grafana/dashboards/client.json
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
cd0ecae to
6d9cb90
Compare
35e157d to
e93fd6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/server/mdm.go`:
- Around line 304-305: Preserve nil-versus-explicit-empty presence when checking
LocalMetricsAddress by replacing the conflictString calls using
msg.GetLocalMetricsAddress() with a pointer-aware comparison. Apply this change
at client/server/mdm.go lines 304-305 and 431-432, while retaining the existing
managed-field conflict behavior for both absent and supplied values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e464b486-7137-49d6-a214-0c83007fc044
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
client/cmd/root.goclient/cmd/up.goclient/internal/debug/debug.goclient/internal/localmetrics/localmetrics.goclient/internal/localmetrics/localmetrics_test.goclient/internal/metrics/influxdb.goclient/internal/metrics/metrics.goclient/internal/metrics/metrics_default.goclient/internal/metrics/prometheus.goclient/internal/peer/status.goclient/internal/profilemanager/config.goclient/internal/profilemanager/config_mdm_test.goclient/mdm/policy.goclient/proto/daemon.protoclient/server/mdm.goclient/server/server.goclient/server/setconfig_mdm_test.goclient/server/setconfig_test.gogo.modinfrastructure_files/observability/grafana/dashboards/client.json
🚧 Files skipped from review as they are similar to previous changes (18)
- client/internal/localmetrics/localmetrics_test.go
- infrastructure_files/observability/grafana/dashboards/client.json
- client/server/setconfig_mdm_test.go
- client/server/setconfig_test.go
- client/internal/profilemanager/config_mdm_test.go
- client/internal/metrics/metrics.go
- client/cmd/up.go
- client/internal/localmetrics/localmetrics.go
- client/internal/metrics/metrics_default.go
- client/internal/metrics/influxdb.go
- client/internal/profilemanager/config.go
- client/internal/debug/debug.go
- client/cmd/root.go
- client/internal/metrics/prometheus.go
- client/proto/daemon.proto
- client/internal/peer/status.go
- go.mod
- client/server/server.go
# Conflicts: # client/internal/debug/debug.go
|
❌ The last analysis has failed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 similar comment
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
theodorsm
left a comment
theodorsm
left a comment
There was a problem hiding this comment.
Foreground mode accepts the local metrics flags, but does not serve the endpoint. Do we want to support this feature for foreground mode?
| ch <- prometheus.MustNewConstMetric(c.signalConnected, prometheus.GaugeValue, boolToFloat(c.status.GetSignalState().Connected)) | ||
|
|
||
| peers := c.status.GetPeerStates() | ||
| ch <- prometheus.MustNewConstMetric(c.peersTotal, prometheus.GaugeValue, float64(len(peers))) |
There was a problem hiding this comment.
This does not account for offline peers, which is also known peers. We should export Status.numOfPeers and use it here (or add offline peers to GetPeerStatus) if the intention of this metric is to mirror status "peers count: x/y connected".
Currently this is what status would show:
And this is what grafana would show:
| d.mux.RLock() | ||
| defer d.mux.RUnlock() | ||
|
|
||
| states := make([]State, 0, len(d.peers)) |
There was a problem hiding this comment.
Should we include offline peers too, like GetFullStatus ? Ref: https://github.com/netbirdio/netbird/pull/6689/changes#r3863137373
| KeyEnableLocalMetrics = "enableLocalMetrics" | ||
| KeyLocalMetricsAddress = "localMetricsAddress" |
There was a problem hiding this comment.
KeyEnableLocalMetrics and KeyLocalMetricsAddress are never added to allKeys (client/mdm/canonical_loaders.go), so the Windows registry and macOS plist loaders discard them as unknown.
| return nil, fmt.Errorf("failed to get default profile config: %w", err) | ||
| } | ||
|
|
||
| s.config = config |
There was a problem hiding this comment.
Missing localMetrics.Reconcile. The profile we switch to might have different metrics settings.
| return nil, fmt.Errorf("failed to get active profile config: %w", err) | ||
| } | ||
| s.mutex.Lock() | ||
| s.config = config |
There was a problem hiding this comment.
Missing localMetrics.Reconcile. The profile might have changed.
| log.Infof("serving local metrics on http://%s/metrics", addr) | ||
| go func() { | ||
| if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
| log.Errorf("failed to serve local metrics on %s: %v", addr, err) |
There was a problem hiding this comment.
We should reset the m.srv and m.addr fields like we do in stop().
| if addr == "" { | ||
| addr = DefaultListenAddress | ||
| } | ||
| warnIfNotLoopback(addr) |
There was a problem hiding this comment.
We should gate it behind a privilege check like we do for ssh rather than just logging.
|
2 participants
Adds an opt-in Prometheus
/metricsendpoint to the client daemon so deployments can scrape connection health locally (requested for a RedHat evaluation: latency, connected peers, P2P vs relay).netbird up --enable-local-metricsand--local-metrics-address(default127.0.0.1:9191); the setting persists in the profile config and travels through SetConfig/Login like other config flags--disable-update-settingsand MDM policy (enableLocalMetrics,localMetricsAddresskeys)All labels are bounded enums except the per-peer latency series, which carries the peer FQDN only.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#952
Summary by CodeRabbit
/metricsendpoint for client connectivity, peer states, latency, and connection performance.