feat: export prometheus metrics#49
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds comprehensive Prometheus metrics instrumentation: a metrics package with Prometheus metrics and helpers, a /metrics HTTP server, an HTTP client instrumentation layer, client API changes to return affected-entry counts, processor integration emitting and updating metrics, config validation/tests for metrics port, dependency bumps, and documentation. ChangesPrometheus Metrics System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/clients/pihole/pihole.go (1)
97-150:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCounters incremented before verifying API success in Pi-hole client. All four DNS/CNAME management methods increment their counters during the loop that builds the payload, before executing the PATCH request and verifying its success. If the PATCH fails, the methods return the incremented count alongside an error, causing metrics to count entries as added/deleted even though the operation failed. Additionally, the recursive retry on 401 may re-execute the entire method and re-increment the counter for the same entries.
pkg/clients/pihole/pihole.go#L97-L150:AddDnsRecordsincrementsnumOfAddedDnsRecordsat line 108 before the PATCH at line 135.pkg/clients/pihole/pihole.go#L152-L205:DeleteDnsRecordsincrementsnumOfDeletedDnsRecordsat line 163 before the PATCH at line 190.pkg/clients/pihole/pihole.go#L246-L299:AddCNameRecordsincrementsnumOfAddedCNameRecordsat line 257 before the PATCH at line 284.pkg/clients/pihole/pihole.go#L301-L354:DeleteCNameRecordsincrementsnumOfDeletedCNameRecordsat line 312 before the PATCH at line 339.Track which entries would be added/deleted in a temporary slice during the loop, then set the counter to the slice length only after confirming the PATCH succeeded (statusCode < 400).
🤖 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 `@pkg/clients/pihole/pihole.go` around lines 97 - 150, For each Pi‑Hole DNS/CNAME management function, stop incrementing the metrics during the payload-build loop and instead collect the affected entries in a temporary slice, perform the PATCH, and only set the counter to len(slice) after verifying statusCode < 400; specifically: in pkg/clients/pihole/pihole.go (97-150) update AddDnsRecords to gather new domains into a temp slice (e.g., toAdd) and only assign numOfAddedDnsRecords = len(toAdd) after a successful PATCH and not before retrying on 401 (ensure refreshAuth retry does not re-count by returning the result of the retried call unchanged); in pkg/clients/pihole/pihole.go (152-205) update DeleteDnsRecords similarly to collect toDelete and set numOfDeletedDnsRecords after successful PATCH; in pkg/clients/pihole/pihole.go (246-299) update AddCNameRecords to collect toAddCNames and set numOfAddedCNameRecords = len(toAddCNames) only after success; and in pkg/clients/pihole/pihole.go (301-354) update DeleteCNameRecords to collect toDeleteCNames and set numOfDeletedCNameRecords after success; keep error handling and 401 retry logic but ensure counters are only set post-success so retries don’t double-count.
🧹 Nitpick comments (1)
pkg/metrics/server.go (1)
33-38: ⚡ Quick winConsider using context-aware listener creation.
The static analysis tool correctly identifies that
net.Listendoesn't accept a context. For proper cancellation during startup, prefer(*net.ListenConfig).Listen:lc := net.ListenConfig{} listener, err := lc.Listen(ctx, "tcp", server.Addr)This allows the listen operation to be cancelled if the context is done during startup.
🤖 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 `@pkg/metrics/server.go` around lines 33 - 38, The code currently calls net.Listen("tcp", server.Addr) which cannot be cancelled; replace it with a context-aware call using a net.ListenConfig, e.g. create lc := net.ListenConfig{} and call listener, err := lc.Listen(ctx, "tcp", server.Addr) (ensure a context.Context named ctx is available in the surrounding function or add one as a parameter), then keep the existing error handling (log.Error and sending to errCh) unchanged; also ensure listener is closed on shutdown as before. Reference the net.Listen usage and the variables listener, err, server.Addr and errCh when making the change.
🤖 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 `@pkg/clients/adguardhome/adguardhome.go`:
- Around line 73-83: In both AddDnsRewrites
(pkg/clients/adguardhome/adguardhome.go lines 73-83) and DeleteDnsRewrites
(pkg/clients/adguardhome/adguardhome.go lines 95-105) move the counter
increments (numOfAddedRewrites and numOfDeletedRewrites) so they occur only
after confirming the request succeeded: after the POST/DELETE call returns,
check for err, then check statusCode for >= 400 (handle 401 as currently done),
and only increment the counter when statusCode < 400; update the logic in each
site accordingly (no other files need changes).
---
Outside diff comments:
In `@pkg/clients/pihole/pihole.go`:
- Around line 97-150: For each Pi‑Hole DNS/CNAME management function, stop
incrementing the metrics during the payload-build loop and instead collect the
affected entries in a temporary slice, perform the PATCH, and only set the
counter to len(slice) after verifying statusCode < 400; specifically: in
pkg/clients/pihole/pihole.go (97-150) update AddDnsRecords to gather new domains
into a temp slice (e.g., toAdd) and only assign numOfAddedDnsRecords =
len(toAdd) after a successful PATCH and not before retrying on 401 (ensure
refreshAuth retry does not re-count by returning the result of the retried call
unchanged); in pkg/clients/pihole/pihole.go (152-205) update DeleteDnsRecords
similarly to collect toDelete and set numOfDeletedDnsRecords after successful
PATCH; in pkg/clients/pihole/pihole.go (246-299) update AddCNameRecords to
collect toAddCNames and set numOfAddedCNameRecords = len(toAddCNames) only after
success; and in pkg/clients/pihole/pihole.go (301-354) update DeleteCNameRecords
to collect toDeleteCNames and set numOfDeletedCNameRecords after success; keep
error handling and 401 retry logic but ensure counters are only set post-success
so retries don’t double-count.
---
Nitpick comments:
In `@pkg/metrics/server.go`:
- Around line 33-38: The code currently calls net.Listen("tcp", server.Addr)
which cannot be cancelled; replace it with a context-aware call using a
net.ListenConfig, e.g. create lc := net.ListenConfig{} and call listener, err :=
lc.Listen(ctx, "tcp", server.Addr) (ensure a context.Context named ctx is
available in the surrounding function or add one as a parameter), then keep the
existing error handling (log.Error and sending to errCh) unchanged; also ensure
listener is closed on shutdown as before. Reference the net.Listen usage and the
variables listener, err, server.Addr and errCh when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cf6ebdc-f012-4213-9ebb-695d94b221e2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
docs/configuration.mddocs/monitoring.mddocs/usage.mdgo.modmain.gomkdocs.ymlpkg/cli/cli.gopkg/clients/adguardhome/adguardhome.gopkg/clients/adguardhome/adguardhome_test.gopkg/clients/common/common.gopkg/clients/npm/npm.gopkg/clients/npm/npm_test.gopkg/clients/pihole/pihole.gopkg/clients/pihole/pihole_test.gopkg/config/config.gopkg/config/config_test.gopkg/metrics/metrics.gopkg/metrics/server.gopkg/processor/processor.go
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation