Cover the engine, config.Size and metrics; fix the five bugs they found
internal/engine, internal/config and internal/metrics had no test file at all, and writing them turned up five real bugs. internal/engine (23 tests, new harness_test.go): a wired engine over a fake upstream on loopback, a local zone, a blocklist, a cache, a query log and an injectable clock. It pins the pipeline order end to end -- ACL deny before the local zone, local zone before blocklist, blocklist before cache, drop sends nothing, the concurrency and rate-limit refusals, serve-stale served with TTL 1 and refreshed once per key, truncation against a 512-byte client, and exactly one metrics record and one query-log entry per query. Five fixes: - config.ParseSize ignored the unit multiplier, so 512MiB parsed as 512 bytes and the whole memory envelope was inert for anyone who wrote a unit. - finalize (EDNS0 and the UDP size) ran only on the cache-hit path, so a large uncached answer left oversized for a 512-byte client. It now runs on the upstream, local-zone and blocklist paths too. - The upstream UDP client read buffer was hard-coded to 1232 while we advertise dns.max_udp_size, so with a larger buffer configured every answer above 1232 bytes was read truncated and became SERVFAIL. The pool takes the size now. - LRU.prefetchGate was a 64-slot global cap, not the single flight its comment promised: a burst of stale hits launched up to 64 duplicate refreshes of one name. It is a per-key latch now; distinct names still refresh in parallel. - metrics.RecordMiss and RecordStaleHit moved only the Prometheus collector, so the Cache page's misses and stale hits read 0 for the life of the process. AGENTS.md gains the trap that cost the most time: this version of miekg/dns reports a character-string over 255 bytes as zero length, so such a message silently fails to pack, and the write failure looks like a client timeout.
This commit is contained in:
developer committed
1 parent
48d37db15b
commit
4f49033f1b
12 files changed
+1837
-37
No files matched your search
@@ -200,11 +200,13 @@ func Record(transport, outcome string, d time.Duration) {
|
||||
}
|
||||
|
||||
// RecordMiss counts a cache miss. A miss is not an error: it just means the
|
||||
// cache could not answer, so only the Prometheus counter moves.
|
||||
func RecordMiss() { initCollectors(); cacheMisses.Inc() }
|
||||
// cache could not answer, so the error counter does not move. The in-process
|
||||
// counter does, like every other outcome: the dashboard reads it and must
|
||||
// never disagree with the Prometheus series.
|
||||
func RecordMiss() { initCollectors(); cacheMisses.Inc(); misses.Add(1) }
|
||||
|
||||
// RecordStaleHit counts a serve-stale hit.
|
||||
func RecordStaleHit() { initCollectors(); cacheStaleHits.Inc() }
|
||||
func RecordStaleHit() { initCollectors(); cacheStaleHits.Inc(); staleHits.Add(1) }
|
||||
|
||||
// RecordUpstreamError counts an upstream failure.
|
||||
func RecordUpstreamError() { initCollectors(); upstreamErrors.Inc() }
|
||||
|
||||
@@ -0,0 +1,250 @@
|
||||
package metrics
|
||||
|
||||
import (
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// The counters are package singletons, so every assertion here is a delta
|
||||
// around the call rather than an absolute value. Nothing in this package runs
|
||||
// in parallel, so a delta is exact.
|
||||
|
||||
func TestRecordMovesTheOutcomeCounter(t *testing.T) {
|
||||
cases := []struct {
|
||||
outcome string
|
||||
check func(Totals) uint64
|
||||
name string
|
||||
wantMove bool
|
||||
}{
|
||||
{OutcomeCache, func(x Totals) uint64 { return x.Hits }, "hits", true},
|
||||
{OutcomeBlocked, func(x Totals) uint64 { return x.Blocked }, "blocked", true},
|
||||
{OutcomeError, func(x Totals) uint64 { return x.Errors }, "errors", true},
|
||||
{OutcomeRefused, func(x Totals) uint64 { return x.Refused }, "refused", true},
|
||||
{OutcomeDropped, func(x Totals) uint64 { return x.Drops }, "dropped", true},
|
||||
// local and upstream are outcomes for the query log and the
|
||||
// duration histogram, but they have no counter of their own: a
|
||||
// resolution that succeeded is counted as a query and nothing
|
||||
// more.
|
||||
{OutcomeLocal, func(x Totals) uint64 { return x.Queries }, "queries only", true},
|
||||
{OutcomeUpstream, func(x Totals) uint64 { return x.Queries }, "queries only", true},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
before := TotalsNow()
|
||||
Record("udp", tc.outcome, 2*time.Millisecond)
|
||||
after := TotalsNow()
|
||||
if got := after.Queries - before.Queries; got != 1 {
|
||||
t.Errorf("%s: queries moved by %d, want 1", tc.name, got)
|
||||
}
|
||||
if tc.outcome != OutcomeLocal && tc.outcome != OutcomeUpstream {
|
||||
if got := tc.check(after) - tc.check(before); got != 1 {
|
||||
t.Errorf("%s: outcome counter moved by %d, want 1", tc.name, got)
|
||||
}
|
||||
}
|
||||
// No other counter may move: a blocked query is not also a miss, and
|
||||
// a refused one is not an error, or the operator cannot trust the
|
||||
// sum on the dashboard.
|
||||
if tc.outcome == OutcomeLocal || tc.outcome == OutcomeUpstream {
|
||||
continue
|
||||
}
|
||||
for _, other := range cases {
|
||||
if other.outcome == tc.outcome {
|
||||
continue
|
||||
}
|
||||
if other.outcome == OutcomeLocal || other.outcome == OutcomeUpstream {
|
||||
continue
|
||||
}
|
||||
// A drop is the one deliberate overlap: it is also an error.
|
||||
// See TestRecordDropIsAlsoAnError.
|
||||
if tc.outcome == OutcomeDropped && other.outcome == OutcomeError {
|
||||
continue
|
||||
}
|
||||
if got := other.check(after) - other.check(before); got != 0 {
|
||||
t.Errorf("%s: %s also moved by %d, want 0", tc.name, other.name, got)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A drop is a refusal to resolve, so an operator counts it as both. Pin that,
|
||||
// because the two series are what the "why are queries failing" dashboard
|
||||
// panel sums.
|
||||
func TestRecordDropIsAlsoAnError(t *testing.T) {
|
||||
before := TotalsNow()
|
||||
Record("udp", OutcomeDropped, time.Millisecond)
|
||||
after := TotalsNow()
|
||||
if after.Drops != before.Drops+1 {
|
||||
t.Errorf("drops moved by %d, want 1", after.Drops-before.Drops)
|
||||
}
|
||||
if after.Errors != before.Errors+1 {
|
||||
t.Errorf("errors moved by %d, want 1: a drop is an error for the operator", after.Errors-before.Errors)
|
||||
}
|
||||
}
|
||||
|
||||
// Record with no duration still counts the query: the duration is for the
|
||||
// histogram, the counter is for the total.
|
||||
func TestRecordWithoutDurationStillCounts(t *testing.T) {
|
||||
before := TotalsNow()
|
||||
Record("udp", OutcomeCache, 0)
|
||||
after := TotalsNow()
|
||||
if after.Queries != before.Queries+1 {
|
||||
t.Errorf("queries moved by %d, want 1", after.Queries-before.Queries)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRecordMissAndStaleAreNotOutcomes(t *testing.T) {
|
||||
// A cache miss is not an error and a stale hit is not a cache hit: each
|
||||
// has its own counter precisely so the two can be told apart.
|
||||
before := TotalsNow()
|
||||
RecordMiss()
|
||||
RecordStaleHit()
|
||||
RecordUpstreamError()
|
||||
after := TotalsNow()
|
||||
if after.Misses != before.Misses+1 {
|
||||
t.Errorf("misses moved by %d, want 1", after.Misses-before.Misses)
|
||||
}
|
||||
if after.StaleHits != before.StaleHits+1 {
|
||||
t.Errorf("stale hits moved by %d, want 1", after.StaleHits-before.StaleHits)
|
||||
}
|
||||
if after.Queries != before.Queries {
|
||||
t.Errorf("queries moved by %d: these three do not go through Record", after.Queries-before.Queries)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCountHelpersDoNotCountQueries(t *testing.T) {
|
||||
// These exist for the few places that report an outcome without a full
|
||||
// Response. They must not inflate the query total, or the queries counter
|
||||
// stops meaning "queries".
|
||||
before := TotalsNow()
|
||||
CountBlocked()
|
||||
CountError()
|
||||
CountDrop()
|
||||
CountRefused()
|
||||
after := TotalsNow()
|
||||
if after.Queries != before.Queries {
|
||||
t.Errorf("queries moved by %d, want 0", after.Queries-before.Queries)
|
||||
}
|
||||
if after.Blocked != before.Blocked+1 || after.Errors != before.Errors+1 ||
|
||||
after.Drops != before.Drops+1 || after.Refused != before.Refused+1 {
|
||||
t.Errorf("a helper missed its own counter: %+v", after)
|
||||
}
|
||||
}
|
||||
|
||||
// Totals is a value, not a view: a caller holding a snapshot must not see it
|
||||
// change under them.
|
||||
func TestTotalsIsASnapshot(t *testing.T) {
|
||||
before := TotalsNow()
|
||||
queries := before.Queries
|
||||
Record("udp", OutcomeCache, time.Millisecond)
|
||||
if before.Queries != queries {
|
||||
t.Errorf("the snapshot moved: %d -> %d", queries, before.Queries)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGaugesArePublished(t *testing.T) {
|
||||
SetConcurrent(7)
|
||||
SetUp(true)
|
||||
SetUpstreamHealth("10.0.0.1:53", "udp", true)
|
||||
SetUpstreamHealth("10.0.0.2:53", "udp", false)
|
||||
SetBlocklistStats(1234, 4096)
|
||||
SetACLGroups(3)
|
||||
SetLocalRecords(11)
|
||||
RecordBlocklistCompile(15 * time.Millisecond)
|
||||
RecordUpstreamLatency("10.0.0.1:53", 4*time.Millisecond)
|
||||
|
||||
body := scrape(t)
|
||||
for _, want := range []string{
|
||||
// The name and the value: a dashboard or a ServiceMonitor that
|
||||
// reads the wrong series fails silently otherwise.
|
||||
"fancydns_concurrent_queries 7",
|
||||
"fancydns_up 1",
|
||||
`fancydns_upstream_health{addr="10.0.0.1:53",proto="udp"} 1`,
|
||||
`fancydns_upstream_health{addr="10.0.0.2:53",proto="udp"} 0`,
|
||||
"fancydns_blocklist_rules 1234",
|
||||
"fancydns_blocklist_source_bytes 4096",
|
||||
"fancydns_acl_groups 3",
|
||||
"fancydns_local_records 11",
|
||||
} {
|
||||
if !strings.Contains(body, want) {
|
||||
t.Errorf("scrape is missing %q\nbody (%d bytes):\n%s", want, len(body), truncate(body, 1200))
|
||||
}
|
||||
}
|
||||
// The histogram's observation count is cumulative across runs of the
|
||||
// suite, so only its presence is pinned here, never its value.
|
||||
if !strings.Contains(body, "fancydns_blocklist_compile_seconds_count") {
|
||||
t.Errorf("scrape is missing the blocklist compile histogram\nbody (%d bytes):\n%s", len(body), truncate(body, 1200))
|
||||
}
|
||||
}
|
||||
|
||||
// The counter names are the contract with the Grafana dashboards and the
|
||||
// ServiceMonitor, so they are pinned here rather than assumed.
|
||||
func TestHandlerExposesTheCounterSeries(t *testing.T) {
|
||||
Record("udp", OutcomeBlocked, time.Millisecond)
|
||||
RecordMiss()
|
||||
body := scrape(t)
|
||||
for _, want := range []string{
|
||||
"fancydns_queries_total",
|
||||
"fancydns_blocked_total",
|
||||
"fancydns_cache_hits_total",
|
||||
"fancydns_cache_misses_total",
|
||||
"fancydns_cache_stale_hits_total",
|
||||
"fancydns_upstream_errors_total",
|
||||
"fancydns_errors_total",
|
||||
"fancydns_dropped_total",
|
||||
"fancydns_refused_total",
|
||||
"fancydns_query_duration_seconds_bucket",
|
||||
"fancydns_query_duration_seconds_count",
|
||||
} {
|
||||
if !strings.Contains(body, want) {
|
||||
t.Errorf("scrape is missing the series %q\nbody (%d bytes):\n%s", want, len(body), truncate(body, 1200))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestSetUpFalseClearsLiveness(t *testing.T) {
|
||||
SetUp(true)
|
||||
if !strings.Contains(scrape(t), "fancydns_up 1") {
|
||||
t.Fatal("up was not published as 1")
|
||||
}
|
||||
SetUp(false)
|
||||
if !strings.Contains(scrape(t), "fancydns_up 0") {
|
||||
t.Error("up was not published as 0 after SetUp(false)")
|
||||
}
|
||||
}
|
||||
|
||||
func scrape(t *testing.T) string {
|
||||
t.Helper()
|
||||
srv := httptest.NewServer(Handler())
|
||||
t.Cleanup(srv.Close)
|
||||
res, err := srv.Client().Get(srv.URL)
|
||||
if err != nil {
|
||||
t.Fatalf("scrape: %v", err)
|
||||
}
|
||||
defer res.Body.Close()
|
||||
if res.StatusCode != http.StatusOK {
|
||||
t.Fatalf("scrape status = %d", res.StatusCode)
|
||||
}
|
||||
b, err := io.ReadAll(res.Body)
|
||||
if err != nil {
|
||||
t.Fatalf("read scrape: %v", err)
|
||||
}
|
||||
return string(b)
|
||||
}
|
||||
|
||||
func truncate(s string, n int) string {
|
||||
if len(s) <= n {
|
||||
return s
|
||||
}
|
||||
return s[:n] + "..."
|
||||
}
|
||||
|
||||
// The namespace is part of the contract with the dashboards, the ServiceMonitor
|
||||
// and every saved query: changing it orphans all of them silently.
|
||||
func TestNamespaceIsStable(t *testing.T) {
|
||||
if Namespace != "fancydns" {
|
||||
t.Fatalf("Namespace = %q, want %q: every dashboard and alert is keyed on it", Namespace, "fancydns")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user