Repository navigation
Conversation
removePeerCharts (ntpd) and removeDeviceCharts (nvme) matched chart IDs by an unterminated prefix: "peer_<addr>" and "device_<name>". Removing peer 10.0.0.1 also marked every chart of 10.0.0.10-10.0.0.19 as obsolete, and removing nvme1 did the same to nvme10-nvme19. The surviving peer or device stays in peerAddr/devicePaths, so its charts were never re-created until the plugin restarted. Match the exact chart IDs built from the peer/device chart templates instead. Refs netdata#24230 Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-actions
Bot
added
area/collectors
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
ntpd peer chart removal src/go/plugin/go.d/collector/ntpd/charts.go, src/go/plugin/go.d/collector/ntpd/collector_test.go |
Peer chart removal matches IDs generated from peer chart templates. A regression test confirms that removing peer 1 does not obsolete peer 10 charts. |
NVMe device chart removal src/go/plugin/go.d/collector/nvme/charts.go, src/go/plugin/go.d/collector/nvme/collector_test.go |
Device chart removal matches IDs generated from device chart templates. A regression test confirms that removing nvme1n1 does not obsolete nvme10n1 charts. |
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix · Severity of issue fixed: Low
Possibly related PRs
- netdata/netdata#24402: Fixes the same open-prefix chart ownership bug in the
k8s_statecollector with exact chart identity matching. - netdata/netdata#24539: Fixes the same chart removal bug in the Docker collector by generating exact chart IDs from object templates.
Suggested labels: area/collectors, collectors/go.d, area/go
Merge Risk: 
62abd
-
The change targets stale-chart removal without an identified merge-blocking issue.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: limiting stale chart removal to the affected peers and devices in the ntpd and nvme collectors. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant NTPD as ntpd Collector
participant NVMe as nvme Collector
participant Charts as Charts Store
participant Client as NTP Client / NVMe CLI
Note over NTPD,Charts: NTPD Peer Chart Management
NTPD->>Client: getPeerIDs()
Client-->>NTPD: Peer list [1, 10]
NTPD->>NTPD: Build peer chart IDs from peerChartsTmpl
NTPD->>Charts: Add charts for peer_203_0_113_1
NTPD->>Charts: Add charts for peer_203_0_113_10
Note over NTPD,Charts: Peer 203.0.113.1 goes stale
NTPD->>Client: getPeerIDs()
Client-->>NTPD: Peer list [10]
NTPD->>NTPD: Build exact chart IDs for stale peer (peer_203_0_113_1)
NTPD->>Charts: MarkRemove() only peer_203_0_113_* charts
Charts-->>NTPD: Peer 203_0_113_10 charts remain active
Note over NVMe,Charts: NVMe Device Chart Management
NVMe->>Client: listDevices()
Client-->>NVMe: Devices [nvme1, nvme10]
NVMe->>NVMe: Build device chart IDs from deviceChartsTmpl
NVMe->>Charts: Add charts for device_nvme1
NVMe->>Charts: Add charts for device_nvme10
Note over NVMe,Charts: /dev/nvme1 goes stale
NVMe->>Client: listDevices()
Client-->>NVMe: Devices [nvme10]
NVMe->>NVMe: Build exact chart IDs for stale device (nvme1)
NVMe->>Charts: MarkRemove() only device_nvme1_* charts
Charts-->>NVMe: Device nvme10 charts remain active
|
|
Closing too, same reason (v2 conversion is the better fix). Thanks! |
|
And do not touch multiple collectors within a single PR - handle them one by one. |
Summary
Refs #24230 (same bug as the docker case there, which #24539 fixes)
ntpdandnvmeremove the charts of a peer/device that went away by an unterminated chart ID prefix:removePeerCharts:peer_<addr>, so removing peer10.0.0.1also marks every chart of10.0.0.10-10.0.0.19obsolete.removeDeviceCharts:device_<name>, so removingnvme1also hitsnvme10-nvme19.The peer/device that is still there stays in
peerAddr/devicePaths, so its charts are never added again until go.d.plugin restarts.Both now match the exact chart IDs built from
peerChartsTmpl/deviceChartsTmpl.Test Plan
TestCollector_Collect_RemovesOnlyChartsOfStalePeercollects peers203.0.113.1and203.0.113.10, then again with only.10, and compares the obsolete flag of every peer chart.TestCollector_Collect_RemovesOnlyChartsOfRemovedDevicedoes the same with/dev/nvme1n1and/dev/nvme10n1.On master both fail (all
203_0_113_10/nvme10charts are obsolete); with the fix they pass.go test -count=1 ./plugin/go.d/collector/ntpd/... ./plugin/go.d/collector/nvme/...andgo vetpass, except nvmeTestCollector_Init, which needs thenvmebinary (not installed here) and fails the same way on master.Additional Information
wireguardhas the same pattern (device_%s, sowg1vswg10). I kept this PR to ntpd and nvme.I used Claude Code to write this change.
For users: How does this change affect me?
10.0.0.10after10.0.0.1,nvme10afternvme1) no longer disappear.Summary by cubic
Fixes
ntpdandnvmechart removal so only the stale peer's or device's charts are marked obsolete.Previously chart IDs were matched by an unterminated prefix: removing peer
10.0.0.1also marked every chart of10.0.0.10–10.0.0.19obsolete, and removing/dev/nvme1did the same tonvme10–nvme19. The surviving peer/device stayed in the collector's lists, so its charts were never re-created until the plugin restarted. Both collectors now match the exact chart IDs built from their chart templates.wireguardhas the same prefix-matching pattern (device_%s) and may need the same fix./dev/nvme1n1and/dev/nvme10n1with the non-stale device still present.Written for commit 62abd92. Summary will update on new commits.
Summary by CodeRabbit