perf(resolver): avoid per-query label map allocations in metrics resolver (#2233)

Author: 0xERR0R

docs/prometheus_grafana.md

@@ -17,7 +17,7 @@ Following metrics will be exported:
 | blocky_query_total                               | Counter of total queries, partitioned by client and DNS request type (A, AAAA, PTR, etc) |
 | blocky_request_duration_seconds                  | Histogram of request duration, partitioned by response type (Blocked, cached, etc)  |
 | blocky_response_total                            | Counter of responses, partitioned by response type (Blocked, cached, etc), DNS response code, and reason |
-| blocky_client_response_total                     | Counter of responses, partitioned by client and response type (Blocked, cached, etc) |
+| blocky_client_response_total                     | Counter of query outcomes, partitioned by client and response type (Blocked, cached, etc); failed requests are counted as `response_type="err"` |
 | blocky_blocking_enabled                          | Boolean 1 if blocking is enabled, 0 otherwise |
 | blocky_cache_entries                             | Gauge of entries in cache |
 | blocky_cache_hits_total                          | Counter of the number of cache hits |
@@ -40,7 +40,7 @@ Following metrics will be exported:
 
     To keep the `reason` label of `blocky_response_total` bounded, blocked responses use the matched
     group names only (e.g. `BLOCKED (ads)`), **not** the matched rule. The full reason including the
-    matched rule (e.g. `BLOCKED (ads: *.docler.com)`) is still available in the [query log](configuration.md#query-log).
+    matched rule (e.g. `BLOCKED (ads: *.docler.com)`) is still available in the [query log](configuration.md#query-logging).
     This avoids unbounded metric cardinality when large deny lists are used.
 
 !!! note "`client` label cardinality"
@@ -52,6 +52,38 @@ Following metrics will be exported:
     the set of `client` label values can grow effectively unbounded over time. Consider this before
     scraping/retaining these metrics on such networks.
 
+    Blocky has no option to drop the label yet, so the mitigation is on the Prometheus side: drop the
+    affected metrics at scrape time when you do not need the per-client breakdown.
+
+    ```yaml
+    metric_relabel_configs:
+      - source_labels: [__name__]
+        regex: "blocky_(query|client_response)_total"
+        action: drop
+    ```
+
+    Dropping only the `client` label (`labeldrop`) does **not** work: the remaining series of the
+    different clients collapse into one, and Prometheus rejects the scrape with a duplicate-sample
+    error.
+
+!!! note "`response_type` values of `blocky_client_response_total`"
+
+    The counter is incremented once per query that reaches the metrics resolver, so it sums to
+    `blocky_query_total` rather than to `blocky_response_total` — the latter counts only successful
+    responses. Requests that produced no response at all are recorded as `response_type="err"`, which
+    is not one of the regular response types.
+
+    `FILTERED` and `NOTFQDN` never appear: the `filtering` and `fqdnOnly` resolvers answer those
+    queries above the metrics resolver in the chain, so they are missing from `blocky_query_total`,
+    `blocky_response_total` and `blocky_request_duration_seconds` as well. The query log sits below
+    them in the chain too, so those queries are only visible in the [statistics](configuration.md#statistics).
+
+    Example — per-client rate of queries that were actually resolved rather than blocked:
+
+    ```promql
+    sum by (client) (rate(blocky_client_response_total{response_type!~"BLOCKED|REBIND|err"}[5m]))
+    ```
+
 ### Grafana dashboard
 
 Example [Grafana](https://grafana.com/) dashboard

e2e/metrics_test.go

@@ -245,6 +245,9 @@ var _ = Describe("Metrics functional tests", func() {
 				g.Expect(metrics).Should(SatisfyAll(
 					ContainElement(MatchRegexp(`blocky_query_total\{[^}]*type="A"[^}]*\} \d+`)),
 					ContainElement(MatchRegexp(`blocky_response_total\{[^}]*\} \d+`)),
+					// blocked.com is on the ads denylist, so the per-client counter must show it
+					ContainElement(MatchRegexp(`blocky_client_response_total\{client="[^"]+",response_type="BLOCKED"\} \d+`)),
+					ContainElement(MatchRegexp(`blocky_client_response_total\{client="[^"]+",response_type="RESOLVED"\} \d+`)),
 					ContainElement(MatchRegexp(`blocky_request_duration_seconds_bucket\{[^}]*\}`)),
 					ContainElement(MatchRegexp(`blocky_request_duration_seconds_sum\{[^}]*\} [\d.]+`)),
 					ContainElement(MatchRegexp(`blocky_request_duration_seconds_count\{[^}]*\} \d+`)),

resolver/metrics_resolver.go

@@ -23,6 +23,11 @@ const (
 	labelReason       = "reason"
 	labelResponseCode = "response_code"
 	labelResponseType = "response_type"
+
+	// responseTypeErr is the synthetic response_type used when the chain returned no
+	// response at all. It is not part of the model.ResponseType enum, so metrics using
+	// the response_type label can carry it in addition to the enum values.
+	responseTypeErr = "err"
 )
 
 // MetricsResolver resolver that records metrics about requests/response
@@ -48,24 +53,21 @@ func (r *MetricsResolver) Resolve(ctx context.Context, request *model.Request) (
 
 	clientLabel := strings.Join(request.ClientNames, ",")
 
-	r.totalQueries.With(prometheus.Labels{
-		labelClient: clientLabel,
-		labelType:   dns.TypeToString[request.Req.Question[0].Qtype],
-	}).Inc()
+	// WithLabelValues is used instead of With(prometheus.Labels{...}) throughout: the map
+	// literal costs an allocation per query on the hot path. The value order must match the
+	// label order of the corresponding metric constructor below.
+	r.totalQueries.WithLabelValues(clientLabel, dns.TypeToString[request.Req.Question[0].Qtype]).Inc()
 
 	reqDuration := time.Since(request.RequestTS)
-	responseType := "err"
+	responseType := responseTypeErr
 
 	if response != nil {
 		responseType = response.RType.String()
 	}
 
 	r.durationHistogram.WithLabelValues(responseType).Observe(reqDuration.Seconds())
 
-	r.totalClientResponse.With(prometheus.Labels{
-		labelClient:       clientLabel,
-		labelResponseType: responseType,
-	}).Inc()
+	r.totalClientResponse.WithLabelValues(clientLabel, responseType).Inc()
 
 	if err != nil {
 		r.totalErrors.Inc()
@@ -79,11 +81,11 @@ func (r *MetricsResolver) Resolve(ctx context.Context, request *model.Request) (
 			reasonLabel = response.Reason
 		}
 
-		r.totalResponse.With(prometheus.Labels{
-			labelReason:       reasonLabel,
-			labelResponseCode: dns.RcodeToString[response.Res.Rcode],
-			labelResponseType: response.RType.String(),
-		}).Inc()
+		r.totalResponse.WithLabelValues(
+			reasonLabel,
+			dns.RcodeToString[response.Res.Rcode],
+			response.RType.String(),
+		).Inc()
 	}
 
 	return response, err
@@ -158,7 +160,8 @@ func totalClientResponseMetric() *prometheus.CounterVec {
 	return prometheus.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "blocky_client_response_total",
-			Help: "Number of total responses per client and response type",
+			Help: "Number of total responses per client and response type, " +
+				"including failed requests as response_type=\"err\"",
 		}, []string{labelClient, labelResponseType},
 	)
 }

resolver/metrics_resolver_test.go

@@ -108,6 +108,17 @@ var _ = Describe("MetricResolver", func() {
 					Expect(err).Should(Succeed())
 					Expect(testutil.ToFloat64(cnt)).Should(BeNumerically("==", 1))
 				})
+				It("records the blocked response per client, without the unbounded reason", func() {
+					_, err := sut.Resolve(ctx, newRequestWithClient("example.com.", A, "", "client"))
+					Expect(err).Should(Succeed())
+
+					clientCnt, err := sut.totalClientResponse.GetMetricWith(prometheus.Labels{
+						labelClient:       "client",
+						labelResponseType: ResponseTypeBLOCKED.String(),
+					})
+					Expect(err).Should(Succeed())
+					Expect(testutil.ToFloat64(clientCnt)).Should(BeNumerically("==", 1))
+				})
 			})
 			When("Response has no ReasonLabel", func() {
 				BeforeEach(func() {
@@ -132,6 +143,19 @@ var _ = Describe("MetricResolver", func() {
 					Expect(testutil.ToFloat64(cnt)).Should(BeNumerically("==", 1))
 				})
 			})
+			When("A client resolves to several names", func() {
+				It("joins them into a single client label, as blocky_query_total does", func() {
+					_, err := sut.Resolve(ctx, newRequestWithClient("example.com.", A, "", "name1", "name2"))
+					Expect(err).Should(Succeed())
+
+					clientCnt, err := sut.totalClientResponse.GetMetricWith(prometheus.Labels{
+						labelClient:       "name1,name2",
+						labelResponseType: ResponseTypeRESOLVED.String(),
+					})
+					Expect(err).Should(Succeed())
+					Expect(testutil.ToFloat64(clientCnt)).Should(BeNumerically("==", 1))
+				})
+			})
 			When("Error occurs while request processing", func() {
 				BeforeEach(func() {
 					m = &mockResolver{}
@@ -153,6 +177,20 @@ var _ = Describe("MetricResolver", func() {
 					Expect(testutil.ToFloat64(clientCnt)).Should(BeNumerically("==", 1))
 				})
 			})
+			When("Metrics are disabled", func() {
+				BeforeEach(func() {
+					sut = NewMetricsResolver(config.Metrics{Enable: false})
+					sut.Next(m)
+				})
+				It("records nothing", func() {
+					_, err := sut.Resolve(ctx, newRequestWithClient("example.com.", A, "", "client"))
+					Expect(err).Should(Succeed())
+
+					Expect(testutil.CollectAndCount(sut.totalClientResponse)).Should(BeZero())
+					Expect(testutil.CollectAndCount(sut.totalQueries)).Should(BeZero())
+					Expect(testutil.CollectAndCount(sut.totalResponse)).Should(BeZero())
+				})
+			})
 		})
 	})
 })