From 06555e04de1fa4063d05fe0520e38f7af325dcd5 Mon Sep 17 00:00:00 2001 From: Dimitri Herzog Date: Mon, 15 Jun 2026 21:32:57 +0200 Subject: [PATCH] fix(metrics): bound reason label cardinality for blocked responses (#2114) --- docs/blocky-grafana.json | 4 +-- docs/prometheus_grafana.md | 7 ++++ helpertest/helper.go | 9 +++++ model/models.go | 7 +++- resolver/blocking_resolver.go | 29 ++++++++++++--- resolver/blocking_resolver_test.go | 41 +++++++++++++++++++++ resolver/caching_resolver.go | 9 +++-- resolver/metrics_resolver.go | 57 ++++++++++++++++++------------ resolver/metrics_resolver_test.go | 47 ++++++++++++++++++++++++ 9 files changed, 177 insertions(+), 33 deletions(-) diff --git a/docs/blocky-grafana.json b/docs/blocky-grafana.json index aff1021b..845ee71d 100644 --- a/docs/blocky-grafana.json +++ b/docs/blocky-grafana.json @@ -1777,7 +1777,7 @@ "legendFormat": "{{reason}}" } ], - "description": "Detailed answer reason, including the upstream resolver or the denylist group that matched." + "description": "Answer reason, including the upstream resolver or the denylist group that matched. For blocked queries the `reason` label is group-level (to keep its cardinality bounded); the specific matched rule/domain is available in the query-log dashboard." }, { "id": 22, @@ -2599,7 +2599,7 @@ "legendFormat": "{{reason}}" } ], - "description": "Which denylist groups blocked the most queries in the dashboard time range." + "description": "Which denylist groups blocked the most queries in the dashboard time range. The `reason` metric label is intentionally group-level to keep its cardinality bounded; for the specific matched rule/domain, use the query-log dashboard." }, { "id": 30, diff --git a/docs/prometheus_grafana.md b/docs/prometheus_grafana.md index a3d82d27..1758097d 100644 --- a/docs/prometheus_grafana.md +++ b/docs/prometheus_grafana.md @@ -34,6 +34,13 @@ Following metrics will be exported: | blocky_rate_limit_cap_exhausted_total | Counter of queries dropped because the rate limiter bucket store was full | | blocky_rate_limit_active_buckets | Gauge of token buckets (≈ distinct clients) currently tracked by the rate limiter | +!!! note "`reason` label for blocked responses" + + 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). + This avoids unbounded metric cardinality when large deny lists are used. + ### Grafana dashboard Example [Grafana](https://grafana.com/) dashboard diff --git a/helpertest/helper.go b/helpertest/helper.go index 668ddd48..1562b760 100644 --- a/helpertest/helper.go +++ b/helpertest/helper.go @@ -159,6 +159,15 @@ func HaveReason(reason string) types.GomegaMatcher { ) } +func HaveReasonLabel(reasonLabel string) types.GomegaMatcher { + return gcustom.MakeMatcher(func(m *model.Response) (bool, error) { + return m.ReasonLabel == reasonLabel, nil + }).WithTemplate( + "Expected:\n{{.Actual}}\n{{.To}} have reason label:\n{{format .Data 1}}", + reasonLabel, + ) +} + func HaveResponseType(c model.ResponseType) types.GomegaMatcher { return gcustom.MakeMatcher(func(m *model.Response) (bool, error) { return m.RType == c, nil diff --git a/model/models.go b/model/models.go index a417adc4..4fcdb6ca 100644 --- a/model/models.go +++ b/model/models.go @@ -53,7 +53,12 @@ func (t ResponseType) ToExtendedErrorCode() uint16 { type Response struct { Res *dns.Msg Reason string - RType ResponseType + // ReasonLabel is a low-cardinality variant of Reason, used as a Prometheus + // metric label. When empty, metrics fall back to Reason. Blocked responses + // set this to the matched group names only (without the matched rule), to + // keep the `reason` label bounded even with large deny lists. + ReasonLabel string + RType ResponseType } // RequestProtocol represents the server protocol ENUM( diff --git a/resolver/blocking_resolver.go b/resolver/blocking_resolver.go index 8af094f1..c54ad736 100644 --- a/resolver/blocking_resolver.go +++ b/resolver/blocking_resolver.go @@ -426,9 +426,10 @@ func determineAllowlistOnlyGroups(cfg *config.Blocking) (result map[string]bool) // sets answer and/or return code for DNS response, if request should be blocked func (r *BlockingResolver) handleBlocked(logger *logrus.Entry, - request *model.Request, question dns.Question, reason string, + request *model.Request, question dns.Question, reason, reasonLabel string, ) (*model.Response, error) { modelResp := model.NewResponseWithReason(request, model.ResponseTypeBLOCKED, reason) + modelResp.ReasonLabel = reasonLabel r.blockHandler.handleBlock(question, modelResp.Res) logger.Debugf("blocking request '%s'", reason) @@ -479,7 +480,7 @@ func (r *BlockingResolver) handleDenylist(ctx context.Context, groupsToCheck []s } if allowlistOnlyAllowed { - resp, err := r.handleBlocked(logger, request, question, "BLOCKED (ALLOWLIST ONLY)") + resp, err := r.handleBlocked(logger, request, question, "BLOCKED (ALLOWLIST ONLY)", "") if err != nil { err = fmt.Errorf("failed to handle allowlist-only block for %s: %w", domain, err) } @@ -488,7 +489,8 @@ func (r *BlockingResolver) handleDenylist(ctx context.Context, groupsToCheck []s } if matches := r.matches(groupsToCheck, r.denylistMatcher, domain); len(matches) > 0 { - resp, err := r.handleBlocked(logger, request, question, formatBlockReason(matches, "")) + resp, err := r.handleBlocked(logger, request, question, + formatBlockReason(matches, ""), formatBlockReasonLabel(matches, "")) return true, resp, err } @@ -520,7 +522,8 @@ func (r *BlockingResolver) Resolve(ctx context.Context, request *model.Request) if matches := r.matches(groupsToCheck, r.allowlistMatcher, entryToCheck); len(matches) > 0 { logger.WithField("matches", matches).Debugf("%s is allowlisted", tName) } else if matches := r.matches(groupsToCheck, r.denylistMatcher, entryToCheck); len(matches) > 0 { - return r.handleBlocked(logger, request, request.Req.Question[0], formatBlockReason(matches, tName)) + return r.handleBlocked(logger, request, request.Req.Question[0], + formatBlockReason(matches, tName), formatBlockReasonLabel(matches, tName)) } } } @@ -656,6 +659,18 @@ func (r *BlockingResolver) matches(groupsToCheck []string, m lists.Matcher, // "BLOCKED[ TYPE] (group: rule[, group: rule...])". Matched groups are sorted // so the rendered reason is deterministic when more than one group matches. func formatBlockReason(matches map[string]string, typeName string) string { + return renderBlockReason(matches, typeName, true) +} + +// formatBlockReasonLabel renders a low-cardinality block reason of the form +// "BLOCKED[ TYPE] (group[, group...])" — group names only, without the matched +// rule. It is used as the `reason` Prometheus metric label so the label stays +// bounded by the number of configured groups, regardless of deny-list size. +func formatBlockReasonLabel(matches map[string]string, typeName string) string { + return renderBlockReason(matches, typeName, false) +} + +func renderBlockReason(matches map[string]string, typeName string, includeRules bool) string { reason := "BLOCKED" if typeName != "" { reason += " " + typeName @@ -667,7 +682,11 @@ func formatBlockReason(matches map[string]string, typeName string) string { entries := make([]string, 0, len(matches)) for _, group := range slices.Sorted(maps.Keys(matches)) { - entries = append(entries, fmt.Sprintf("%s: %s", group, matches[group])) + if includeRules { + entries = append(entries, fmt.Sprintf("%s: %s", group, matches[group])) + } else { + entries = append(entries, group) + } } return fmt.Sprintf("%s (%s)", reason, strings.Join(entries, ", ")) diff --git a/resolver/blocking_resolver_test.go b/resolver/blocking_resolver_test.go index 9b6152ad..1d561abc 100644 --- a/resolver/blocking_resolver_test.go +++ b/resolver/blocking_resolver_test.go @@ -285,6 +285,15 @@ var _ = Describe("BlockingResolver", Label("blockingResolver"), func() { HaveReturnCode(dns.RcodeSuccess), )) }) + It("should set a group-only ReasonLabel while keeping the detailed Reason", func() { + Expect(sut.Resolve(ctx, newRequestWithClient("domain1.com.", A, "1.2.1.2", "client1"))). + Should( + SatisfyAll( + HaveResponseType(ResponseTypeBLOCKED), + HaveReason("BLOCKED (gr1: domain1.com)"), + HaveReasonLabel("BLOCKED (gr1)"), + )) + }) It("should block the A query if domain is on the denylist (multipart 1)", func() { Expect(sut.Resolve(ctx, newRequestWithClient("domain1.com.", A, "1.2.1.2", "client2"))). Should( @@ -1542,3 +1551,35 @@ var _ = Describe("formatBlockReason", func() { } }) }) + +var _ = Describe("formatBlockReasonLabel", func() { + It("renders the matched group name only, without the rule", func() { + Expect(formatBlockReasonLabel(map[string]string{"gr1": "domain1.com"}, "")). + Should(Equal("BLOCKED (gr1)")) + }) + + It("includes the entry type when given (e.g. CNAME/IP)", func() { + Expect(formatBlockReasonLabel(map[string]string{"ads": "*.docler.com"}, "CNAME")). + Should(Equal("BLOCKED CNAME (ads)")) + }) + + It("keeps the label bounded: different rules in the same group yield the same label", func() { + // This is the whole point of the label: the rule (regex/domain) is + // unbounded, but the group set is not. Both must map to one metric series. + a := formatBlockReasonLabel(map[string]string{"ads": "/snapchat/"}, "") + b := formatBlockReasonLabel(map[string]string{"ads": "tracker.example.com"}, "") + + Expect(a).Should(Equal("BLOCKED (ads)")) + Expect(b).Should(Equal(a)) + }) + + It("renders multiple matched groups sorted by group for deterministic output", func() { + matches := map[string]string{"zeta": "z.com", "alpha": "a.com", "mid": "m.com"} + + // repeat to guard against Go's randomized map iteration order + for range 20 { + Expect(formatBlockReasonLabel(matches, "")). + Should(Equal("BLOCKED (alpha, mid, zeta)")) + } + }) +}) diff --git a/resolver/caching_resolver.go b/resolver/caching_resolver.go index 651c21df..f0140a12 100644 --- a/resolver/caching_resolver.go +++ b/resolver/caching_resolver.go @@ -24,7 +24,12 @@ import ( "github.com/sirupsen/logrus" ) -const defaultCachingCleanUpInterval = 5 * time.Second +const ( + defaultCachingCleanUpInterval = 5 * time.Second + + // cachedReason is the response reason used for cache hits. + cachedReason = "CACHED" +) // CacheDecorator optionally wraps the result cache (e.g., with Redis sync). type CacheDecorator func(cache.ExpiringCache[[]byte]) (cache.ExpiringCache[[]byte], error) @@ -217,7 +222,7 @@ func (r *CachingResolver) Resolve(ctx context.Context, request *model.Request) ( setTTLInCachedResponse(val, ttl) if val.Rcode == dns.RcodeSuccess { - return &model.Response{Res: val, RType: model.ResponseTypeCACHED, Reason: "CACHED"}, nil + return &model.Response{Res: val, RType: model.ResponseTypeCACHED, Reason: cachedReason}, nil } return &model.Response{Res: val, RType: model.ResponseTypeCACHED, Reason: "CACHED NEGATIVE"}, nil diff --git a/resolver/metrics_resolver.go b/resolver/metrics_resolver.go index ef9b0897..e0856bf1 100644 --- a/resolver/metrics_resolver.go +++ b/resolver/metrics_resolver.go @@ -41,30 +41,41 @@ type MetricsResolver struct { func (r *MetricsResolver) Resolve(ctx context.Context, request *model.Request) (*model.Response, error) { response, err := r.next.Resolve(ctx, request) - if r.cfg.Enable { - r.totalQueries.With(prometheus.Labels{ - labelClient: strings.Join(request.ClientNames, ","), - labelType: dns.TypeToString[request.Req.Question[0].Qtype], + if !r.cfg.Enable { + return response, err + } + + r.totalQueries.With(prometheus.Labels{ + labelClient: strings.Join(request.ClientNames, ","), + labelType: dns.TypeToString[request.Req.Question[0].Qtype], + }).Inc() + + reqDuration := time.Since(request.RequestTS) + responseType := "err" + + if response != nil { + responseType = response.RType.String() + } + + r.durationHistogram.WithLabelValues(responseType).Observe(reqDuration.Seconds()) + + if err != nil { + r.totalErrors.Inc() + } else { + // Prefer the low-cardinality ReasonLabel for the metric label; fall + // back to Reason when it is not set. Blocked responses embed the + // matched rule in Reason (e.g. a regex or domain), which is unbounded + // for large deny lists, so they set ReasonLabel to the group names only. + reasonLabel := response.ReasonLabel + if reasonLabel == "" { + reasonLabel = response.Reason + } + + r.totalResponse.With(prometheus.Labels{ + labelReason: reasonLabel, + labelResponseCode: dns.RcodeToString[response.Res.Rcode], + labelResponseType: response.RType.String(), }).Inc() - - reqDuration := time.Since(request.RequestTS) - responseType := "err" - - if response != nil { - responseType = response.RType.String() - } - - r.durationHistogram.WithLabelValues(responseType).Observe(reqDuration.Seconds()) - - if err != nil { - r.totalErrors.Inc() - } else { - r.totalResponse.With(prometheus.Labels{ - labelReason: response.Reason, - labelResponseCode: dns.RcodeToString[response.Res.Rcode], - labelResponseType: response.RType.String(), - }).Inc() - } } return response, err diff --git a/resolver/metrics_resolver_test.go b/resolver/metrics_resolver_test.go index 87821858..84486db1 100644 --- a/resolver/metrics_resolver_test.go +++ b/resolver/metrics_resolver_test.go @@ -77,6 +77,53 @@ var _ = Describe("MetricResolver", func() { m.AssertExpectations(GinkgoT()) }) }) + When("Response has a low-cardinality ReasonLabel", func() { + BeforeEach(func() { + m = &mockResolver{} + m.On("Resolve", mock.Anything).Return(&Response{ + Res: new(dns.Msg), + RType: ResponseTypeBLOCKED, + Reason: "BLOCKED (ads: /snapchat/)", + ReasonLabel: "BLOCKED (ads)", + }, nil) + sut.Next(m) + }) + It("uses ReasonLabel for the reason metric label, not the detailed Reason", func() { + _, err := sut.Resolve(ctx, newRequestWithClient("example.com.", A, "", "client")) + Expect(err).Should(Succeed()) + + cnt, err := sut.totalResponse.GetMetricWith(prometheus.Labels{ + "reason": "BLOCKED (ads)", + "response_code": "NOERROR", + "response_type": "BLOCKED", + }) + Expect(err).Should(Succeed()) + Expect(testutil.ToFloat64(cnt)).Should(BeNumerically("==", 1)) + }) + }) + When("Response has no ReasonLabel", func() { + BeforeEach(func() { + m = &mockResolver{} + m.On("Resolve", mock.Anything).Return(&Response{ + Res: new(dns.Msg), + RType: ResponseTypeRESOLVED, + Reason: "CACHED", + }, nil) + sut.Next(m) + }) + It("falls back to Reason for the reason metric label", func() { + _, err := sut.Resolve(ctx, newRequestWithClient("example.com.", A, "", "client")) + Expect(err).Should(Succeed()) + + cnt, err := sut.totalResponse.GetMetricWith(prometheus.Labels{ + "reason": "CACHED", + "response_code": "NOERROR", + "response_type": "RESOLVED", + }) + Expect(err).Should(Succeed()) + Expect(testutil.ToFloat64(cnt)).Should(BeNumerically("==", 1)) + }) + }) When("Error occurs while request processing", func() { BeforeEach(func() { m = &mockResolver{}