[management, client] Add management-controlled client metrics push#5886
Open
[management, client] Add management-controlled client metrics push#5886
Conversation
Allow enabling/disabling client metrics push from the dashboard via account settings instead of requiring env vars on every client. - Add MetricsConfig proto message to NetbirdConfig - Add MetricsPushEnabled to account Settings (DB-persisted) - Expose metrics_push_enabled in OpenAPI and dashboard API handler - Populate MetricsConfig in sync and login responses - Client dynamically starts/stops push based on management config - NB_METRICS_PUSH_ENABLED env var overrides management when explicitly set - Add activity events for metrics push enable/disable
Contributor
📝 WalkthroughWalkthroughThis PR implements account-level metrics push configuration. The management server persists a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Mgmt as Management Server
participant Metrics as ClientMetrics
Client->>Mgmt: Login Request
Mgmt->>Client: LoginResponse + NetbirdConfig.Metrics
Client->>Metrics: UpdatePushFromMgm(enabled)
alt enabled && push not running
Metrics->>Metrics: startPushLocked()
else disabled && push running
Metrics->>Metrics: stopPushLocked()
end
sequenceDiagram
participant Engine as Engine
participant Mgmt as Management Server
participant Metrics as ClientMetrics
Engine->>Mgmt: Request Sync Update
Mgmt->>Engine: SyncResponse + NetbirdConfig.Metrics
Engine->>Engine: handleMetricsUpdate(config)
Engine->>Metrics: UpdatePushFromMgm(config.Enabled)
alt enabled && push not running
Metrics->>Metrics: startPushLocked()
else disabled && push running
Metrics->>Metrics: stopPushLocked()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Update TestUpdateAccountPeers assertions: NetbirdConfig is no longer nil in peer update responses since it now carries MetricsConfig even when STUN/TURN config is absent.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/types/settings.go (1)
82-103:⚠️ Potential issue | 🟡 MinorClone
JWTAllowGroupsinCopy()too.The copy still shares the original backing array for
JWTAllowGroups, so mutating the clone can leak back into the source settings.PeerExposeGroupsis already cloned; this slice should be handled the same way.♻️ Proposed fix
settings := &Settings{ PeerLoginExpirationEnabled: s.PeerLoginExpirationEnabled, PeerLoginExpiration: s.PeerLoginExpiration, JWTGroupsEnabled: s.JWTGroupsEnabled, JWTGroupsClaimName: s.JWTGroupsClaimName, GroupsPropagationEnabled: s.GroupsPropagationEnabled, - JWTAllowGroups: s.JWTAllowGroups, + JWTAllowGroups: slices.Clone(s.JWTAllowGroups), RegularUsersViewBlocked: s.RegularUsersViewBlocked,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/settings.go` around lines 82 - 103, The copied Settings still shares the backing slice for JWTAllowGroups, so update the Settings copy logic (where settings := &Settings{...} in the Settings.Copy() routine) to clone JWTAllowGroups the same way PeerExposeGroups is cloned (use slices.Clone or equivalent) to avoid mutations of the copy affecting the original; reference the JWTAllowGroups and PeerExposeGroups fields and the Settings.Copy() creation block when making the change.
🧹 Nitpick comments (3)
management/server/peer_test.go (1)
1065-1069: AssertNetbirdConfig.Metricsexplicitly to lock in the new contract.This validates
NetbirdConfigpresence, but it won’t catch regressions whereMetricsis missing or mismatched. Add a direct assertion forMetrics.Enabled.Suggested test hardening
for _, channel := range peerChannels { update := <-channel - assert.NotNil(t, update.Update.NetbirdConfig) + require.NotNil(t, update.Update) + require.NotNil(t, update.Update.NetbirdConfig) + require.NotNil(t, update.Update.NetbirdConfig.Metrics) + require.NotNil(t, account.Settings) + assert.Equal(t, account.Settings.MetricsPushEnabled, update.Update.NetbirdConfig.Metrics.Enabled) assert.Nil(t, update.Update.NetbirdConfig.Stuns) assert.Nil(t, update.Update.NetbirdConfig.Turns) assert.Nil(t, update.Update.NetbirdConfig.Signal) assert.Nil(t, update.Update.NetbirdConfig.Relay)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/peer_test.go` around lines 1065 - 1069, The test currently checks NetbirdConfig presence and several nil fields but doesn't assert Metrics; add an explicit assertion that update.Update.NetbirdConfig.Metrics is not nil and that update.Update.NetbirdConfig.Metrics.Enabled (or the boolean field name used) equals the expected value to lock in the new contract; update the test in peer_test.go around the existing assertions for update.Update.NetbirdConfig to include checks for Metrics and Metrics.Enabled to catch regressions.management/server/http/handlers/accounts/accounts_handler_test.go (1)
107-286: Add one round-trip case withmetrics_push_enabled=true.All new assertions only cover the default
falsevalue, so these tests still pass if the handler silently drops the incoming field. A PUT case that setsmetrics_push_enabled: trueand asserts it in the response would exercise the new mapping end to end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/accounts/accounts_handler_test.go` around lines 107 - 286, Add a PUT test case that round-trips metrics_push_enabled=true: in the test cases slice used by the accounts handler tests add a new entry (similar to the "PutAccount OK" cases) with requestType http.MethodPut, requestPath "/api/accounts/"+accountID, requestBody containing {"settings": {"metrics_push_enabled": true, ...}} and expectedStatus http.StatusOK, and set expectedSettings.MetricsPushEnabled to br(true) so the assertion on the response verifies the handler preserves the metrics_push_enabled field; locate the table by the fields requestBody and expectedSettings to add this new case next to the other PutAccount cases.client/internal/engine.go (1)
969-975: Lower this to debug or log only on transitions.
handleSynccan run often, so logging every received metrics config at info level will get noisy quickly.Debugf, or logging only whenUpdatePushFromMgmactually changes state, would keep routine syncs out of normal logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine.go` around lines 969 - 975, Change the noisy Info-level log in handleMetricsUpdate to a Debug-level log or make it conditional on an actual state transition: before calling e.clientMetrics.UpdatePushFromMgm(ctx, enabled) read the current push state (via whatever getter exists on clientMetrics) and call UpdatePushFromMgm only if the enabled value differs; if you keep the call, change log.Infof to log.Debugf and/or log only when the pre- and post-update states differ so UpdatePushFromMgm is the reference point for whether to emit a log message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@management/server/types/settings.go`:
- Around line 82-103: The copied Settings still shares the backing slice for
JWTAllowGroups, so update the Settings copy logic (where settings :=
&Settings{...} in the Settings.Copy() routine) to clone JWTAllowGroups the same
way PeerExposeGroups is cloned (use slices.Clone or equivalent) to avoid
mutations of the copy affecting the original; reference the JWTAllowGroups and
PeerExposeGroups fields and the Settings.Copy() creation block when making the
change.
---
Nitpick comments:
In `@client/internal/engine.go`:
- Around line 969-975: Change the noisy Info-level log in handleMetricsUpdate to
a Debug-level log or make it conditional on an actual state transition: before
calling e.clientMetrics.UpdatePushFromMgm(ctx, enabled) read the current push
state (via whatever getter exists on clientMetrics) and call UpdatePushFromMgm
only if the enabled value differs; if you keep the call, change log.Infof to
log.Debugf and/or log only when the pre- and post-update states differ so
UpdatePushFromMgm is the reference point for whether to emit a log message.
In `@management/server/http/handlers/accounts/accounts_handler_test.go`:
- Around line 107-286: Add a PUT test case that round-trips
metrics_push_enabled=true: in the test cases slice used by the accounts handler
tests add a new entry (similar to the "PutAccount OK" cases) with requestType
http.MethodPut, requestPath "/api/accounts/"+accountID, requestBody containing
{"settings": {"metrics_push_enabled": true, ...}} and expectedStatus
http.StatusOK, and set expectedSettings.MetricsPushEnabled to br(true) so the
assertion on the response verifies the handler preserves the
metrics_push_enabled field; locate the table by the fields requestBody and
expectedSettings to add this new case next to the other PutAccount cases.
In `@management/server/peer_test.go`:
- Around line 1065-1069: The test currently checks NetbirdConfig presence and
several nil fields but doesn't assert Metrics; add an explicit assertion that
update.Update.NetbirdConfig.Metrics is not nil and that
update.Update.NetbirdConfig.Metrics.Enabled (or the boolean field name used)
equals the expected value to lock in the new contract; update the test in
peer_test.go around the existing assertions for update.Update.NetbirdConfig to
include checks for Metrics and Metrics.Enabled to catch regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfc3ef48-5242-4457-bf77-82b5a74dafad
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
client/internal/connect.goclient/internal/engine.goclient/internal/metrics/env.goclient/internal/metrics/metrics.gomanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/account.gomanagement/server/activity/codes.gomanagement/server/http/handlers/accounts/accounts_handler.gomanagement/server/http/handlers/accounts/accounts_handler_test.gomanagement/server/peer_test.gomanagement/server/types/settings.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/management.proto
bcmmbaga
approved these changes
Apr 16, 2026
lixmal
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Allow enabling/disabling client metrics push from the dashboard via account settings instead of requiring env vars on every client.
netbirdio/dashboard#613
netbirdio/docs#701
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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Release Notes