From: Otto Moerbeek Date: Tue, 13 Dec 2022 11:25:12 +0000 (+0100) Subject: rec: make response stats a tcounter object X-Git-Tag: dnsdist-1.8.0-rc1~159^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12323%2Fhead;p=thirdparty%2Fpdns.git rec: make response stats a tcounter object This allows for the packet cache hit path to record response stats without performance impact. The qtype and rcode counters are capped, as i ran into trouble with the thread stack sizes on macOS and OpenBSD. See the source comment for explanation. Closes #11534 --- diff --git a/pdns/recursordist/Makefile.am b/pdns/recursordist/Makefile.am index 0371e08c3f..dc30a296c0 100644 --- a/pdns/recursordist/Makefile.am +++ b/pdns/recursordist/Makefile.am @@ -169,6 +169,7 @@ pdns_recursor_SOURCES = \ rec-lua-conf.hh rec-lua-conf.cc \ rec-main.hh rec-main.cc \ rec-protozero.cc rec-protozero.hh \ + rec-responsestats.hh rec-responsestats.cc \ rec-snmp.hh rec-snmp.cc \ rec-taskqueue.cc rec-taskqueue.hh \ rec-tcounters.cc rec-tcounters.hh \ @@ -184,7 +185,6 @@ pdns_recursor_SOURCES = \ remote_logger.cc remote_logger.hh \ resolve-context.hh \ resolver.hh resolver.cc \ - responsestats.hh responsestats.cc \ root-addresses.hh \ root-dnssec.hh \ rpzloader.cc rpzloader.hh \ @@ -295,6 +295,7 @@ testrunner_SOURCES = \ query-local-address.hh query-local-address.cc \ rcpgenerator.cc \ rec-eventtrace.cc rec-eventtrace.hh \ + rec-responsestats.hh rec-responsestats.cc \ rec-taskqueue.cc rec-taskqueue.hh \ rec-tcounters.cc rec-tcounters.hh \ rec-zonetocache.cc rec-zonetocache.hh \ @@ -302,7 +303,6 @@ testrunner_SOURCES = \ recursor_cache.cc recursor_cache.hh \ reczones-helpers.cc reczones-helpers.hh \ resolver.hh resolver.cc \ - responsestats.cc \ root-dnssec.hh \ rpzloader.cc rpzloader.hh \ secpoll.cc \ diff --git a/pdns/recursordist/docs/upgrade.rst b/pdns/recursordist/docs/upgrade.rst index b6d65e75a0..0d81bafc22 100644 --- a/pdns/recursordist/docs/upgrade.rst +++ b/pdns/recursordist/docs/upgrade.rst @@ -4,9 +4,21 @@ Upgrade Guide Before upgrading, it is advised to read the :doc:`changelog/index`. When upgrading several versions, please read **all** notes applying to the upgrade. -4.7.0 to master +4.8.0 to master --------------- +Metrics +------- +The way metrics are collected has been changed to increase performance, especially when many thread are used. +This allows for solving a long standing issue that some statistics were not updated on packet cache hits. +This is now resolved, but has the consequence that some metrics (in particular response related ones) changed behaviour as they now also reflect packet cache hits, while they did not before. +This affects the results shown by ``rec_control get-qtypelist`` and the ``response-by-qtype``, ``response-sizes`` and ``response-by-rcode`` items returned by the ``/api/v1/servers/localhost/statistics`` API endpoint. +Additionally, most ``RCodes`` and ``QTypes`` that are marked ``Unassigned``, ``Reserved`` or ``Obsolete`` by IANA are not accounted, to reduce the memory consumed by these metrics. + + +4.7.0 to 4.8.0 +-------------- + Structured logging ^^^^^^^^^^^^^^^^^^ All logging (except query tracing) has been converted to structured logging. diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 5bc0969d4c..3937e10db5 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -28,7 +28,6 @@ #include "ednspadding.hh" #include "query-local-address.hh" #include "rec-taskqueue.hh" -#include "responsestats.hh" #include "shuffle.hh" #include "validate-recursor.hh" @@ -1553,7 +1552,7 @@ void startDoResolve(void* p) pw.commit(); } - g_rs.submitResponse(dc->d_mdp.d_qtype, packet.size(), pw.getHeader()->rcode, !dc->d_tcp); + t_Counters.at(rec::ResponseStats::responseStats).submitResponse(dc->d_mdp.d_qtype, packet.size(), pw.getHeader()->rcode); updateResponseStats(res, dc->d_source, packet.size(), &dc->d_mdp.d_qname, dc->d_mdp.d_qtype); #ifdef NOD_ENABLED bool nod = false; @@ -1927,8 +1926,10 @@ bool checkForCacheHit(bool qnameParsed, unsigned int tag, const string& data, ageDNSPacket(response, age); if (response.length() >= sizeof(struct dnsheader)) { const struct dnsheader* dh = reinterpret_cast(response.data()); - updateResponseStats(dh->rcode, source, response.length(), 0, 0); + updateResponseStats(dh->rcode, source, response.length(), nullptr, 0); + t_Counters.at(rec::ResponseStats::responseStats).submitResponse(qtype, response.length(), dh->rcode); } + // we assume 0 usec t_Counters.at(rec::DoubleWAvgCounter::avgLatencyUsec).addToRollingAvg(0.0, g_latencyStatSize); t_Counters.at(rec::DoubleWAvgCounter::avgLatencyOursUsec).addToRollingAvg(0.0, g_latencyStatSize); diff --git a/pdns/recursordist/rec-responsestats.cc b/pdns/recursordist/rec-responsestats.cc new file mode 100644 index 0000000000..ea283c3601 --- /dev/null +++ b/pdns/recursordist/rec-responsestats.cc @@ -0,0 +1,96 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "rec-responsestats.hh" + +#include + +#include "namespaces.hh" +#include "logger.hh" + +#include "dnsparser.hh" + +static auto sizeBounds() +{ + std::vector bounds; + + bounds.push_back(20); + bounds.push_back(40); + bounds.push_back(60); + bounds.push_back(80); + bounds.push_back(100); + bounds.push_back(150); + for (uint64_t n = 200; n < 65000; n += 200) { + bounds.push_back(n); + } + return bounds; +} + +RecResponseStats::RecResponseStats() : + d_sizecounters("SizeCounters", sizeBounds()) +{ + for (auto& entry : d_qtypecounters) { + entry = 0; + } + for (auto& entry : d_rcodecounters) { + entry = 0; + } +} + +RecResponseStats& RecResponseStats::operator+=(const RecResponseStats& rhs) +{ + for (size_t i = 0; i < d_qtypecounters.size(); i++) { + d_qtypecounters.at(i) += rhs.d_qtypecounters.at(i); + } + for (size_t i = 0; i < d_rcodecounters.size(); i++) { + d_rcodecounters.at(i) += rhs.d_rcodecounters.at(i); + } + d_sizecounters += rhs.d_sizecounters; + return *this; +} + +map RecResponseStats::getQTypeResponseCounts() const +{ + map ret; + for (size_t i = 0; i < d_qtypecounters.size(); ++i) { + auto count = d_qtypecounters.at(i); + if (count != 0) { + ret[i] = count; + } + } + return ret; +} + +map RecResponseStats::getSizeResponseCounts() const +{ + map ret; + for (const auto& sizecounter : d_sizecounters.getRawData()) { + if (sizecounter.d_count > 0) { + ret[sizecounter.d_boundary] = sizecounter.d_count; + } + } + return ret; +} + +map RecResponseStats::getRCodeResponseCounts() const +{ + map ret; + for (size_t i = 0; i < d_rcodecounters.size(); ++i) { + auto count = d_rcodecounters.at(i); + if (count != 0) { + ret[i] = count; + } + } + return ret; +} + +string RecResponseStats::getQTypeReport() const +{ + auto qtypenums = getQTypeResponseCounts(); + ostringstream ostr; + for (const auto& val : qtypenums) { + ostr << DNSRecordContent::NumberToType(val.first) << '\t' << std::to_string(val.second) << endl; + } + return ostr.str(); +} diff --git a/pdns/recursordist/rec-responsestats.hh b/pdns/recursordist/rec-responsestats.hh new file mode 100644 index 0000000000..00046bc52a --- /dev/null +++ b/pdns/recursordist/rec-responsestats.hh @@ -0,0 +1,75 @@ +/* + * This file is part of PowerDNS or dnsdist. + * Copyright -- PowerDNS.COM B.V. and its contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * In addition, for the avoidance of any doubt, permission is granted to + * link this program with OpenSSL and to (re)distribute the binaries + * produced as the result of such linking. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ +#pragma once + +#include + +#include "histogram.hh" +#include "dnspacket.hh" + +class RecResponseStats +{ +public: + RecResponseStats(); + + RecResponseStats& operator+=(const RecResponseStats&); + + // To limit the size of this object, we cap the rcodes and qtypes, + // in line with + // https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml + + // This is to reduce the memory used and the amount of work to be + // done by TCounters. As this class is part of the TCounter object, + // growing it too much would cause large objects on the stack. A + // full QType array would take 64k * sizeof(uint64_t) = 512k. + // Having such an object on a thread stack does not work well on + // e.g. macOS or OpenBSD, where the default thread stack size is + // limited. Additionally, C++ has no platform independent way to + // enlarge the thread stack size. + + // We could allocate parts of this on the heap, but this would still + // mean having to manipulate large amounts of data by the TCounter + // classes + + static const uint16_t maxRCode = 23; // BADCOOKIE + static const uint16_t maxQType = 260; // AMTRELAY + + void submitResponse(uint16_t qtype, uint16_t respsize, uint8_t rcode) + { + if (rcode <= maxRCode) { + d_rcodecounters.at(rcode)++; + } + if (qtype <= maxQType) { + d_qtypecounters.at(qtype)++; + } + d_sizecounters(respsize); + } + map getQTypeResponseCounts() const; + map getSizeResponseCounts() const; + map getRCodeResponseCounts() const; + string getQTypeReport() const; + +private: + std::array d_qtypecounters{}; + std::array d_rcodecounters{}; + pdns::Histogram d_sizecounters; +}; diff --git a/pdns/recursordist/rec-tcounters.cc b/pdns/recursordist/rec-tcounters.cc index b40815a8cc..23d2cb77a8 100644 --- a/pdns/recursordist/rec-tcounters.cc +++ b/pdns/recursordist/rec-tcounters.cc @@ -51,7 +51,7 @@ Counters& Counters::merge(const Counters& data) for (size_t i = 0; i < histograms.size(); i++) { histograms.at(i) += data.histograms.at(i); } - + responseStats += data.responseStats; return *this; } diff --git a/pdns/recursordist/rec-tcounters.hh b/pdns/recursordist/rec-tcounters.hh index d9f7040a46..6947228723 100644 --- a/pdns/recursordist/rec-tcounters.hh +++ b/pdns/recursordist/rec-tcounters.hh @@ -27,6 +27,7 @@ #include #include "histogram.hh" +#include "rec-responsestats.hh" namespace rec { @@ -110,6 +111,13 @@ enum class RCode : uint8_t numberOfCounters }; +// Recursor Response Stats +enum class ResponseStats : uint8_t +{ + responseStats, + numberOfCounters +}; + // A few other histograms enum class Histogram : uint8_t { @@ -172,15 +180,20 @@ struct Counters pdns::Histogram{"cumul-authanswers-", 1000, 13}, pdns::Histogram{"cumul-authanswers-", 1000, 13}}; + // Response stats + RecResponseStats responseStats{}; + Counters() { for (auto& elem : uint64Count) { elem = 0; } - // doubleWAvg has a default ct that initializes + // doubleWAvg has a default constructor that initializes for (auto& elem : auth.rcodeCounters) { elem = 0; } + // Histogram has a constructor that initializes + // RecResponseStats has a default constructor that initializes } // Merge a set of counters into an existing set of counters. For simple counters, that will be additions @@ -204,6 +217,12 @@ struct Counters return auth; } + RecResponseStats& at(ResponseStats index) + { + // We only have a single ResponseStats indexed RecResponseStats, so no need to select a specific one + return responseStats; + } + pdns::Histogram& at(Histogram index) { return histograms.at(static_cast(index)); diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index e58fc441a7..6dbc293531 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -25,7 +25,6 @@ #include #include #include "lock.hh" -#include "responsestats.hh" #include "rec-lua-conf.hh" #include "aggressive_nsec.hh" @@ -2319,7 +2318,7 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int s, const str return {0, setMinimumTTL(begin, end)}; } if (cmd == "get-qtypelist") { - return {0, g_rs.getQTypeReport()}; + return {0, g_Counters.sum(rec::ResponseStats::responseStats).getQTypeReport()}; } if (cmd == "add-nta") { return {0, doAddNTA(begin, end)}; diff --git a/pdns/recursordist/responsestats.cc b/pdns/recursordist/responsestats.cc deleted file mode 120000 index 24fbd43e53..0000000000 --- a/pdns/recursordist/responsestats.cc +++ /dev/null @@ -1 +0,0 @@ -../responsestats.cc \ No newline at end of file diff --git a/pdns/recursordist/responsestats.hh b/pdns/recursordist/responsestats.hh deleted file mode 120000 index c5449e573c..0000000000 --- a/pdns/recursordist/responsestats.hh +++ /dev/null @@ -1 +0,0 @@ -../responsestats.hh \ No newline at end of file diff --git a/pdns/tcounters.hh b/pdns/tcounters.hh index 4fcef47f0d..9689de4669 100644 --- a/pdns/tcounters.hh +++ b/pdns/tcounters.hh @@ -196,7 +196,7 @@ private: }; // Sum for a specific index -// In the future we might the move the specifics of computing an aggregated value to the +// In the future we might want to move the specifics of computing an aggregated value to the // app specific Counters class template template @@ -211,7 +211,7 @@ auto GlobalCounters::sum(Enum index) } // Average for a specific index -// In the future we might the move the specifics of computing an aggregated value to the +// In the future we might want to move the specifics of computing an aggregated value to the // app specific Counters class template template @@ -230,7 +230,7 @@ auto GlobalCounters::avg(Enum index) } // Max for a specific index -// In the future we might the move the specifics of computing an aggregated value to the +// In the future we might want to move the specifics of computing an aggregated value to the // app specific Counters class template template diff --git a/pdns/ws-api.cc b/pdns/ws-api.cc index 21c2267d86..f3b71104be 100644 --- a/pdns/ws-api.cc +++ b/pdns/ws-api.cc @@ -32,8 +32,10 @@ #include "version.hh" #include "arguments.hh" #include "dnsparser.hh" +#ifdef RECURSOR +#include "syncres.hh" +#else #include "responsestats.hh" -#ifndef RECURSOR #include "statbag.hh" #endif #include @@ -206,9 +208,16 @@ void apiServerStatistics(HttpRequest* req, HttpResponse* resp) { }); } +#ifdef RECURSOR + auto stats = g_Counters.sum(rec::ResponseStats::responseStats); + auto resp_qtype_stats = stats.getQTypeResponseCounts(); + auto resp_size_stats = stats.getSizeResponseCounts(); + auto resp_rcode_stats = stats.getRCodeResponseCounts(); +#else auto resp_qtype_stats = g_rs.getQTypeResponseCounts(); auto resp_size_stats = g_rs.getSizeResponseCounts(); auto resp_rcode_stats = g_rs.getRCodeResponseCounts(); +#endif { Json::array values; for(const auto& item : resp_qtype_stats) { diff --git a/regression-tests.api/runtests.py b/regression-tests.api/runtests.py index bbec7b0fab..d0472e77f4 100755 --- a/regression-tests.api/runtests.py +++ b/regression-tests.api/runtests.py @@ -109,6 +109,7 @@ allow-from-file=acl.list allow-notify-from-file=acl-notify.list api-config-dir=%(conf_dir)s include-dir=%(conf_dir)s +devonly-regression-test-mode """