perf(resolver): avoid per-query label map allocations in metrics resolver - #2233
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2233 +/- ##
==========================================
+ Coverage 88.04% 88.05% +0.01%
==========================================
Files 126 126
Lines 9963 9958 -5
==========================================
- Hits 8772 8769 -3
+ Misses 925 924 -1
+ Partials 266 265 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are low-risk and well-covered by new unit/e2e tests, with only a minor Help-text wording nit called out in review comments.
Pull request overview
This PR optimizes the resolver metrics hot path by eliminating per-query label-map allocations and expands/clarifies the per-client outcome metric (blocky_client_response_total) with supporting unit/e2e coverage and documentation updates.
Changes:
- Switches counter increments in
MetricsResolver.ResolvetoWithLabelValues(...)to avoid per-queryprometheus.Labelsmap allocations. - Adds/records
blocky_client_response_total{client,response_type}, including a syntheticresponse_type="err"when no response is produced. - Updates Prometheus/Grafana docs and strengthens unit + e2e tests to cover blocked/resolved/error paths and label behavior.
File summaries
| File | Description |
|---|---|
| resolver/metrics_resolver.go | Avoids label-map allocations on the hot path; records new per-client outcome counter and defines response_type="err". |
| resolver/metrics_resolver_test.go | Adds unit specs for the new counter (resolved/blocked/error), multi-name client joining, and metrics-disabled behavior. |
| metrics/metrics_test.go | Extends registry completeness assertion to include blocky_client_response_total. |
| e2e/metrics_test.go | Validates the new counter appears in /metrics for both BLOCKED and RESOLVED outcomes. |
| docs/prometheus_grafana.md | Documents new metric semantics plus cardinality mitigation guidance and response_type value details. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| prometheus.CounterOpts{ | ||
| Name: "blocky_client_response_total", | ||
| Help: "Number of total responses per client and response type, " + | ||
| "including failed requests as response_type=\"err\"", | ||
| }, []string{labelClient, labelResponseType}, |
b229157 to
ef56b08
Compare
…lver The metrics resolver built a prometheus.Labels map for every counter it touched, on the path taken by every DNS query. Each map literal costs an allocation; WithLabelValues takes the values positionally and costs none. Measured with a benchmark over MetricsResolver.Resolve (RESOLVED response, one client name), before -> after: GOGC default 4593 ns/op 1008 B/op 6 allocs/op -> 489 ns/op 0 0 GOGC=off 1245 ns/op 1008 B/op 6 allocs/op -> 480 ns/op 0 0 The direct cost of the map literals is ~765 ns/query; the rest of the gap is GC pressure from ~1 KB of garbage per query. The real-world gain sits between the two, depending on the overall allocation load. Also names the synthetic "err" response type, and adds specs for the blocked, multi-name and metrics-disabled cases of blocky_client_response_total, plus an e2e assertion that the counter reaches the /metrics endpoint with a real client label.
blocky_client_response_total is documented as being partitioned by the bounded response type enum, but the exposed value set differs from it in both directions: - Requests that produce no response are counted as response_type="err", which is not an enum value. An alert written as response_type!~"BLOCKED" silently counts failures as successfully resolved traffic. - FILTERED and NOTFQDN can never appear: the filtering and fqdnOnly resolvers answer those queries above the metrics resolver in the chain, so they are absent from blocky_query_total, blocky_response_total and blocky_request_duration_seconds too. Also notes that the counter sums to blocky_query_total rather than to the similarly named blocky_response_total, which skips the error path, and gives the client cardinality note an actual mitigation: drop the two affected metrics at scrape time. Dropping only the client label collapses the per-client series and makes Prometheus reject the scrape. Claude-Session: https://claude.ai/code/session_01X2Dg33aFoCW4AvWQasX5rX
ef56b08 to
ec4fb9d
Compare
Follow-up to the review of #2222, implementing the points raised there.
Important
Stacked on #2222, which is not merged yet, so the diff against
maincurrentlyalso contains that PR's two commits. Only
05cd2a8andb229157belong to this PR.Rebase onto
mainonce #2222 lands.1.
WithLabelValuesinstead ofWith(prometheus.Labels{…})Every counter in
MetricsResolver.Resolvebuilt a label map, on the path taken by everyDNS query.
#2222added a third one.WithLabelValuespasses the values positionally andallocates nothing.
Benchmarked over
MetricsResolver.Resolve(RESOLVED response, one client name):The direct cost of the map literals is ~765 ns/query; the rest of the gap is GC pressure
from ~1 KB of garbage per query. Real-world gain sits between the two numbers, depending
on the overall allocation load. The benchmark was not committed — it needs a stub resolver
that does not belong in the package.
2. Document what
response_typeactually containsblocky_client_response_totalwas justified byresponse_typebeing the bounded 12-valueenum. The exposed set differs from it in both directions:
erris a 13th value.responseTypestarts as"err"and is only overwritten whenthe chain returned a response, so failures land in the counter. An alert written as
response_type!~"BLOCKED"— the shape proposed in blocky_query_total can't be split by blocklist reason — no way to alert on "real" vs blocked query rate #2198 — silently counts failures assuccessfully resolved traffic. Now named as a constant, stated in the metric's
Helptext and in the docs.
FILTEREDandNOTFQDNcan never appear.FilteringResolverandFQDNOnlyResolveranswer those queries themselves and sit above
MetricsResolverin the chain, so theFILTEREDclause in the proposed alert is dead. They are missing fromblocky_query_total,blocky_response_totalandblocky_request_duration_secondstoo — pre-existing, but worthwriting down. See the note at the end.
Also documented: the counter increments once per query, so it sums to
blocky_query_total,not to the similarly named
blocky_response_total, which skips the error path.3. Give the cardinality note a lever
#2222 correctly warns that
clientis unbounded, butconfig.Metricsonly hasenableand
path, so the note left readers with nothing to do. It now shows the Prometheus-sidemitigation — and warns that the obvious one is wrong: dropping only the
clientlabelcollapses the per-client series into one and makes Prometheus reject the scrape with a
duplicate-sample error. Dropping the two metrics is the working option.
4. Tests
blocky_client_response_totalfor a BLOCKED response — the metric's whole purpose,previously untested.
name1,name2, covering theclientLabelvalue nowshared between two counters.
/metricswith a real client label and bothresponse_type="BLOCKED"andresponse_type="RESOLVED". This is what pins the positionalWithLabelValuesorder end-to-end.Each new spec was mutation-checked (break the expectation → spec fails), so none pass vacuously.
Not included
blocky-grafana.jsonhas atopk(10, sum by (client) (…blocky_query_total…))panel; a "top blocked clients" companion would fit, but that is a dashboard change with its
own review surface.
clientlabel. You mentioned planning this in blocky_query_total can't be split by blocklist reason — no way to alert on "real" vs blocked query rate #2198 — it is thereal fix for the cardinality note and deserves its own PR.
MetricsResolverabovefiltering/fqdnOnlysoFILTERED/NOTFQDNbecomevisible. Documented here rather than changed, since it shifts existing metric semantics.
Happy to open an issue for it.
Verification
go build ./...go test -race ./resolver/... ./metrics/...— passgo test ./resolver/... ./metrics/... ./server/... ./stats/...— passmake lint(golangci-lint v2.12.2) — 0 issuesblocky-e2eimage — pass