From 7c05901b1d22693a29cced84f9c3b24f5dc4a0ec Mon Sep 17 00:00:00 2001
From: Peter van Dijk <peter.van.dijk@powerdns.com>
Date: Mon, 30 Jan 2023 18:42:46 +0100
Subject: [PATCH] ixfrdist /metrics: quote label values; suffix counters with
 _total; remove full totals; add promtool test

---
 pdns/ixfrdist-stats.cc                  | 53 +++++++++++--------------
 regression-tests.ixfrdist/test_Stats.py | 32 ++++++++++++---
 tasks.py                                |  1 +
 3 files changed, 50 insertions(+), 36 deletions(-)

diff --git a/pdns/ixfrdist-stats.cc b/pdns/ixfrdist-stats.cc
index 3aa9720568..292565a819 100644
--- a/pdns/ixfrdist-stats.cc
+++ b/pdns/ixfrdist-stats.cc
@@ -42,67 +42,60 @@ std::string ixfrdistStats::getStats() {
       stats<<"# TYPE "<<prefix<<"soa_serial gauge"<<std::endl;
     }
     if(d.second.haveZone)
-      stats<<prefix<<"soa_serial{domain="<<d.first<<"} "<<d.second.currentSOA<<std::endl;
+      stats<<prefix<<"soa_serial{domain=\""<<d.first<<"\"} "<<d.second.currentSOA<<std::endl;
     else
-      stats<<prefix<<"soa_serial{domain="<<d.first<<"} NaN"<<std::endl;
+      stats<<prefix<<"soa_serial{domain=\""<<d.first<<"\"} NaN"<<std::endl;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"soa_checks Number of times a SOA check at the master was attempted"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"soa_checks counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"soa_checks_total Number of times a SOA check at the master was attempted"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"soa_checks_total counter"<<std::endl;
     }
-    stats<<prefix<<"soa_checks{domain="<<d.first<<"} "<<d.second.numSOAChecks<<std::endl;
+    stats<<prefix<<"soa_checks_total{domain=\""<<d.first<<"\"} "<<d.second.numSOAChecks<<std::endl;
     numSOAChecks += d.second.numSOAChecks;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"soa_checks_failed Number of times a SOA check at the master failed"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"soa_checks_failed counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"soa_checks_failed_total Number of times a SOA check at the master failed"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"soa_checks_failed_total counter"<<std::endl;
     }
-    stats<<prefix<<"soa_checks_failed{domain="<<d.first<<"} "<<d.second.numSOAChecksFailed<<std::endl;
+    stats<<prefix<<"soa_checks_failed_total{domain=\""<<d.first<<"\"} "<<d.second.numSOAChecksFailed<<std::endl;
     numSOAChecksFailed += d.second.numSOAChecksFailed;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"soa_inqueries Number of times a SOA query was received"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"soa_inqueries counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"soa_inqueries_total Number of times a SOA query was received"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"soa_inqueries_total counter"<<std::endl;
     }
-    stats<<prefix<<"soa_inqueries{domain="<<d.first<<"} "<<d.second.numSOAinQueries<<std::endl;
+    stats<<prefix<<"soa_inqueries_total{domain=\""<<d.first<<"\"} "<<d.second.numSOAinQueries<<std::endl;
     numSOAinQueries += d.second.numSOAinQueries;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"axfr_inqueries Number of times an AXFR query was received"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"axfr_inqueries counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"axfr_inqueries_total Number of times an AXFR query was received"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"axfr_inqueries_total counter"<<std::endl;
     }
-    stats<<prefix<<"axfr_inqueries{domain="<<d.first<<"} "<<d.second.numAXFRinQueries<<std::endl;
+    stats<<prefix<<"axfr_inqueries_total{domain=\""<<d.first<<"\"} "<<d.second.numAXFRinQueries<<std::endl;
     numAXFRinQueries += d.second.numAXFRinQueries;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"axfr_failures Number of times an AXFR query was not properly answered"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"axfr_failures counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"axfr_failures_total Number of times an AXFR query was not properly answered"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"axfr_failures_total counter"<<std::endl;
     }
-    stats<<prefix<<"axfr_failures{domain="<<d.first<<"} "<<d.second.numAXFRFailures<<std::endl;
+    stats<<prefix<<"axfr_failures_total{domain=\""<<d.first<<"\"} "<<d.second.numAXFRFailures<<std::endl;
     numAXFRFailures += d.second.numAXFRFailures;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"ixfr_inqueries Number of times an IXFR query was received"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"ixfr_inqueries counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"ixfr_inqueries_total Number of times an IXFR query was received"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"ixfr_inqueries_total counter"<<std::endl;
     }
-    stats<<prefix<<"ixfr_inqueries{domain="<<d.first<<"} "<<d.second.numIXFRinQueries<<std::endl;
+    stats<<prefix<<"ixfr_inqueries_total{domain=\""<<d.first<<"\"} "<<d.second.numIXFRinQueries<<std::endl;
     numIXFRinQueries += d.second.numIXFRinQueries;
 
     if (!helpAdded) {
-      stats<<"# HELP "<<prefix<<"ixfr_failures Number of times an IXFR query was not properly answered"<<std::endl;
-      stats<<"# TYPE "<<prefix<<"ixfr_failures counter"<<std::endl;
+      stats<<"# HELP "<<prefix<<"ixfr_failures_total Number of times an IXFR query was not properly answered"<<std::endl;
+      stats<<"# TYPE "<<prefix<<"ixfr_failures_total counter"<<std::endl;
     }
-    stats<<prefix<<"ixfr_failures{domain="<<d.first<<"} "<<d.second.numIXFRFailures<<std::endl;
+    stats<<prefix<<"ixfr_failures_total{domain=\""<<d.first<<"\"} "<<d.second.numIXFRFailures<<std::endl;
     numIXFRFailures += d.second.numIXFRFailures;
     helpAdded = true;
   }
 
-  stats<<prefix<<"soa_checks "<<numSOAChecks<<std::endl;
-  stats<<prefix<<"soa_checks_failed "<<numSOAChecksFailed<<std::endl;
-  stats<<prefix<<"soa_inqueries "<<numSOAinQueries<<std::endl;
-  stats<<prefix<<"axfr_inqueries "<<numAXFRinQueries<<std::endl;
-  stats<<prefix<<"ixfr_inqueries "<<numIXFRinQueries<<std::endl;
-  stats<<prefix<<"axfr_failures "<<numAXFRFailures<<std::endl;
-  stats<<prefix<<"ixfr_failures "<<numIXFRFailures<<std::endl;
   return stats.str();
 }
diff --git a/regression-tests.ixfrdist/test_Stats.py b/regression-tests.ixfrdist/test_Stats.py
index f170a6732e..8c76511add 100644
--- a/regression-tests.ixfrdist/test_Stats.py
+++ b/regression-tests.ixfrdist/test_Stats.py
@@ -1,6 +1,7 @@
 from ixfrdisttests import IXFRDistTest
 import time
 import requests
+import subprocess
 
 xfrServerPort = 4244
 
@@ -29,11 +30,11 @@ webserver-address: %s
     _config_domains = {'example': '127.0.0.1:' + str(xfrServerPort)}
 
     metric_prog_stats = ["ixfrdist_uptime_seconds", "ixfrdist_domains"]
-    metric_domain_stats = ["ixfrdist_soa_serial", "ixfrdist_soa_checks",
-                           "ixfrdist_soa_checks_failed",
-                           "ixfrdist_soa_inqueries",
-                           "ixfrdist_axfr_inqueries", "ixfrdist_axfr_failures",
-                           "ixfrdist_ixfr_inqueries", "ixfrdist_ixfr_failures"]
+    metric_domain_stats = ["ixfrdist_soa_serial", "ixfrdist_soa_checks_total",
+                           "ixfrdist_soa_checks_failed_total",
+                           "ixfrdist_soa_inqueries_total",
+                           "ixfrdist_axfr_inqueries_total", "ixfrdist_axfr_failures_total",
+                           "ixfrdist_ixfr_inqueries_total", "ixfrdist_ixfr_failures_total"]
 
     @classmethod
     def setUpClass(cls):
@@ -45,6 +46,24 @@ webserver-address: %s
     def tearDownClass(cls):
         cls.tearDownIXFRDist()
 
+    def checkPrometheusContentPromtool(self, content):
+        output = None
+        try:
+            testcmd = ['promtool', 'check', 'metrics']
+            process = subprocess.Popen(testcmd, stdout=subprocess.PIPE, stdin=subprocess.PIPE, stderr=subprocess.STDOUT, close_fds=True)
+            output = process.communicate(input=content)
+        except subprocess.CalledProcessError as exc:
+            raise AssertionError('%s failed (%d): %s' % (testcmd, process.returncode, process.output))
+
+        # commented out because promtool returns 3 because of the "_total" suffix warnings
+        #if process.returncode != 0:
+        #    raise AssertionError('%s failed (%d): %s' % (testcmd, process.returncode, output))
+
+        for line in output[0].splitlines():
+            if line.endswith(b"should have \"_total\" suffix"):
+                continue
+            raise AssertionError('%s returned an unexpected output. Faulty line is "%s", complete content is "%s"' % (testcmd, line, output))
+
     def test_program_stats_exist(self):
         res = requests.get('http://{}/metrics'.format(self.webserver_address))
         self.assertEqual(res.status_code, 200)
@@ -55,6 +74,7 @@ webserver-address: %s
                 continue
             self.assertIn(line.split(" ")[0],
                           self.metric_prog_stats + self.metric_domain_stats)
+        self.checkPrometheusContentPromtool(res.content)
 
     def test_registered(self):
         res = requests.get('http://{}/metrics'.format(self.webserver_address))
@@ -67,7 +87,7 @@ webserver-address: %s
                 continue
             if "{" not in line:
                 continue
-            self.assertIn('{domain=example}', line)
+            self.assertIn('{domain="example"}', line)
             self.assertIn(line.split("{")[0], self.metric_domain_stats)
 
     def test_metrics_have_help(self):
diff --git a/tasks.py b/tasks.py
index 3686718cc0..c3076df329 100644
--- a/tasks.py
+++ b/tasks.py
@@ -108,6 +108,7 @@ auth_test_deps = [   # FIXME: we should be generating some of these from shlibde
     'libsystemd0',
     'libyaml-cpp0.6',
     'libzmq3-dev',
+    'prometheus',
     'ruby-bundler',
     'ruby-dev',
     'socat',
-- 
2.47.2