From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 9 Jan 2023 16:17:21 +0000 (+0100)
Subject: dnsdist: Only record one hit or miss per query in the cache metrics
X-Git-Tag: dnsdist-1.8.0-rc1~118^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ad9a0329bc07026e7d913964ee06c6b013bda063;p=thirdparty%2Fpdns.git

dnsdist: Only record one hit or miss per query in the cache metrics

The scope-zero feature and the DoH paths can actually do more than
one lookup per query, and until now this led to an increase of the
per-cache metric for every lookup, while the global `cache-hits`
and `cache-misses` metrics were only updated once per query.
This has led to several questions and misunderstandings, so we now
only update the per-cache metrics once per query as well.
---

diff --git a/pdns/dnsdist-cache.cc b/pdns/dnsdist-cache.cc
index cf2dab9e57..3fe10336e0 100644
--- a/pdns/dnsdist-cache.cc
+++ b/pdns/dnsdist-cache.cc
@@ -192,7 +192,7 @@ void DNSDistPacketCache::insert(uint32_t key, const boost::optional<Netmask>& su
   }
 }
 
-bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired, bool skipAging, bool truncatedOK)
+bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired, bool skipAging, bool truncatedOK, bool recordMiss)
 {
   const auto& dnsQName = dq.ids.qname.getStorage();
   uint32_t key = getKey(dnsQName, dq.ids.qname.wirelength(), dq.getData(), receivedOverUDP);
@@ -220,14 +220,18 @@ bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut
 
     std::unordered_map<uint32_t,CacheValue>::const_iterator it = map->find(key);
     if (it == map->end()) {
-      d_misses++;
+      if (recordMiss) {
+        d_misses++;
+      }
       return false;
     }
 
     const CacheValue& value = it->second;
     if (value.validity <= now) {
       if ((now - value.validity) >= static_cast<time_t>(allowExpired)) {
-        d_misses++;
+        if (recordMiss) {
+          d_misses++;
+        }
         return false;
       }
       else {
diff --git a/pdns/dnsdist-cache.hh b/pdns/dnsdist-cache.hh
index 9ee1f055e2..cce3708312 100644
--- a/pdns/dnsdist-cache.hh
+++ b/pdns/dnsdist-cache.hh
@@ -38,7 +38,7 @@ public:
   DNSDistPacketCache(size_t maxEntries, uint32_t maxTTL=86400, uint32_t minTTL=0, uint32_t tempFailureTTL=60, uint32_t maxNegativeTTL=3600, uint32_t staleTTL=60, bool dontAge=false, uint32_t shards=1, bool deferrableInsertLock=true, bool parseECS=false);
 
   void insert(uint32_t key, const boost::optional<Netmask>& subnet, uint16_t queryFlags, bool dnssecOK, const DNSName& qname, uint16_t qtype, uint16_t qclass, const PacketBuffer& response, bool receivedOverUDP, uint8_t rcode, boost::optional<uint32_t> tempFailureTTL);
-  bool get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired = 0, bool skipAging = false, bool truncatedOK = true);
+  bool get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut, boost::optional<Netmask>& subnet, bool dnssecOK, bool receivedOverUDP, uint32_t allowExpired = 0, bool skipAging = false, bool truncatedOK = true, bool recordMiss = true);
   size_t purgeExpired(size_t upTo, const time_t now);
   size_t expunge(size_t upTo=0);
   size_t expungeByName(const DNSName& name, uint16_t qtype=QType::ANY, bool suffixMatch=false);
diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 91158f442f..b02fadaa79 100644
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -1278,7 +1278,7 @@ ProcessQueryResult processQuery(DNSQuestion& dq, ClientState& cs, LocalHolders&
       // we need ECS parsing (parseECS) to be true so we can be sure that the initial incoming query did not have an existing
       // ECS option, which would make it unsuitable for the zero-scope feature.
       if (dq.ids.packetCache && !dq.ids.skipCache && (!selectedBackend || !selectedBackend->d_config.disableZeroScope) && dq.ids.packetCache->isECSParsingEnabled()) {
-        if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKeyNoECS, dq.ids.subnet, dq.ids.dnssecOK, !dq.overTCP(), allowExpired)) {
+        if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKeyNoECS, dq.ids.subnet, dq.ids.dnssecOK, !dq.overTCP(), allowExpired, false, true, false)) {
 
           vinfolog("Packet cache hit for query for %s|%s from %s (%s, %d bytes)", dq.ids.qname.toLogString(), QType(dq.ids.qtype).toString(), dq.ids.origRemote.toStringWithPort(), dq.ids.protocol.toString(), dq.getData().size());
 
@@ -1311,7 +1311,7 @@ ProcessQueryResult processQuery(DNSQuestion& dq, ClientState& cs, LocalHolders&
         forwardedOverUDP = false;
       }
 
-      if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKey, dq.ids.subnet, dq.ids.dnssecOK, forwardedOverUDP, allowExpired)) {
+      if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKey, dq.ids.subnet, dq.ids.dnssecOK, forwardedOverUDP, allowExpired, false, true, true)) {
 
         restoreFlags(dq.getHeader(), dq.ids.origFlags);