dnsdist: add support for labels on custom metrics #14956

esensar · 2024-12-12T08:08:55Z

Short description

This adds support for prometheus labels on custom metrics. Changes the metrics maps to hold a map of labels to metric values for each metric. Metrics without labels have an empty string label combination value.

Adds an optional options table as the last parameter of incMetric, decMetric, setMetric and getMetric, which can be used to set labels by using the label key or to set the previously used parameter (either step or value).

Closes: #13359

Checklist

I have:

read the CONTRIBUTING.md document
compiled this code
tested this code
included documentation (including possible behaviour changes)
documented the code
added or modified regression test(s)
added or modified unit test(s)

This adds support for prometheus labels on custom metrics. Changes the metrics maps to hold a map of labels to metric values for each metric. Metrics without labels have an empty string label combination value. Adds an optional options table as the last parameter of `incMetric`, `decMetric`, `setMetric` and `getMetric`, which can be used to set labels by using the `label` key or to set the previously used parameter (either `step` or `value`). Closes: PowerDNS#13359

coveralls · 2024-12-12T08:32:59Z

Pull Request Test Coverage Report for Build 12356677632

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

137 of 179 (76.54%) changed or added relevant lines in 7 files are covered.
7286 unchanged lines in 144 files lost coverage.
Overall coverage increased (+8.4%) to 63.958%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pdns/dnsdistdist/dnsdist-carbon.cc	1	3	33.33%
pdns/dnsdistdist/dnsdist-lua-ffi.cc	3	10	30.0%
pdns/dnsdistdist/dnsdist-lua-ffi.hh	13	21	61.9%
pdns/dnsdistdist/dnsdist-metrics.cc	65	75	86.67%
pdns/dnsdistdist/dnsdist-lua.cc	43	58	74.14%

Files with Coverage Reduction	New Missed Lines	%
pdns/auth-catalogzone.hh	1	66.67%
pdns/dnsdistdist/test-dnsdistrules_cc.cc	1	95.15%
pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc	1	85.12%
pdns/recursordist/nod.hh	1	92.59%
pdns/test-dnsrecords_cc.cc	2	95.97%
pdns/dnsdistdist/test-dnsdist-lua-ffi.cc	2	99.59%
pdns/dnsdistdist/dnsdist-lua-ffi.hh	2	72.22%
pdns/epollmplexer.cc	2	85.0%
pdns/dnssecinfra.cc	2	76.3%
pdns/snmp-agent.hh	2	75.0%

Totals
Change from base Build 12305519461:	8.4%
Covered Lines:	85317
Relevant Lines:	110706

💛 - Coveralls

pdns/dnsdistdist/dnsdist-lua.cc

rgacogne

I made a few comments, and I think we need to exclude metrics with labels from both Carbon export and dumpStats(), but I'm quite happy with the general concept, and the code itself is good as well.

pdns/dnsdistdist/dnsdist-metrics.cc

rgacogne · 2024-12-12T09:39:52Z

pdns/dnsdistdist/dnsdist-metrics.cc

+    auto combinationOfLabels = generateCombinationOfLabels(labels);
+    auto metricEntry = metric->second.find(combinationOfLabels);
+    if (metricEntry == metric->second.end()) {
+      metricEntry = metric->second.emplace(combinationOfLabels, MutableCounter()).first;
+      g_stats.entries.write_lock()->emplace_back(Stats::EntryTriple{std::string(name), combinationOfLabels, &metricEntry->second.d_value});
+    }
+    metricEntry->second.d_value -= step;
+    return metricEntry->second.d_value.load();


Just a nit but it looks like we could get rid of a bit of code duplication by turning this into a (templated to handle both counters and gauges?) function.

That makes sense, I will try to clean it up. My C++ skills are not good, so expect a messy first attempt 😄

No problem, and please feel free to ask for help if needed, or even to just leave the cleanup to me when you are done, that's completely fine!

Thank you! I will take a shot and let you know if I get stuck

I have added a template function for this. Let me know if it looks okay or if it needs some work.

…els are used

esensar · 2024-12-12T16:41:02Z

By the way, an unrelated question:
I have tried to fix formatting in files I have touched, using the provided script (./build-scripts/format-code...). Doing that has produced wrong results, which failed on CI. On further inspection I saw that the script expects clang-format-11, while I had clang-format-17 installed. After installing 11, it worked as expected. Is there a reason that clang-format-11 is still used?

omoerbeek · 2024-12-12T17:00:24Z

By the way, an unrelated question: I have tried to fix formatting in files I have touched, using the provided script (./build-scripts/format-code...). Doing that has produced wrong results, which failed on CI. On further inspection I saw that the script expects clang-format-11, while I had clang-format-17 installed. After installing 11, it worked as expected. Is there a reason that clang-format-11 is still used?

Version 11 was the one that was available in our CI when we started enforcing formatting. Later we noticed differences with newer clang-format versions, so we fixated the version. It mights be time to move to a newer version...

esensar · 2024-12-12T18:37:40Z

I think everything is covered now. I still need to update documentation though.

rgacogne

Looks good to me, I suggested a few changes to appease the static analyzer. A new regression test would be nice, and of course as you already mentioned this needs to be documented :)

pdns/dnsdistdist/dnsdist-lua-ffi.cc

pdns/dnsdistdist/dnsdist-lua.cc

Co-authored-by: Remi Gacogne <github@coredump.fr>

esensar · 2024-12-16T10:33:50Z

I have added the documentation. I was not sure what to set as versionAdded. Also, should I have added variants with options as separate functions (in case of incMetric, decMetric and declareMetric) and deprecate old variants, or is this way of documenting parameters with different variants okay?

rgacogne

A couple nits but the documentation looks OK and the tests are quite nice, thanks!

pdns/dnsdistdist/docs/reference/custommetrics.rst

Co-authored-by: Remi Gacogne <github@coredump.fr>

esensar · 2024-12-16T16:13:26Z

A couple nits but the documentation looks OK and the tests are quite nice, thanks!

Thank you! I think for 2.0.0 (if breaking changes are allowed between major versions), we could just leave opts for incMetric, decMetric and declareMetric, instead of allowing previous optional parameter for these, but I guess that can be done in a separate PR for easier backporting.

rgacogne · 2024-12-17T08:53:03Z

I think for 2.0.0 (if breaking changes are allowed between major versions), we could just leave opts for incMetric, decMetric and declareMetric, instead of allowing previous optional parameter for these, but I guess that can be done in a separate PR for easier backporting.

Breaking changes in major versions are of course OK when they are needed or particularly useful, but my feeling is that it's neither in this case. The solution you came with is backward-compatible and quite clean 👍.

esensar mentioned this pull request Dec 12, 2024

Prometheus metric labels on custom metrics #13359

Closed

rgacogne added enhancement dnsdist labels Dec 12, 2024

rgacogne self-requested a review December 12, 2024 08:28

rgacogne added this to the dnsdist-2.0.0 milestone Dec 12, 2024

github-advanced-security bot found potential problems Dec 12, 2024

View reviewed changes

pdns/dnsdistdist/dnsdist-lua.cc Fixed Show fixed Hide fixed

pdns/dnsdistdist/dnsdist-lua.cc Fixed Show fixed Hide fixed

rgacogne requested changes Dec 12, 2024

View reviewed changes

esensar added 6 commits December 12, 2024 16:01

dnsdist: add options to declareMetric to skip initialization if lab…

0dff6be

…els are used

chore: reformat metrics related files

138a0fb

dnsdist: remove unnecessary optional around variant opts

7d07538

dnsdist: fix formatting in dnsdist-lua.cc

423251d

dnsdist: use template function to simplify metrics mutation functions

657caa5

dnsdist: clean up clang-tidy warnings in lua and metrics

ec216cd

dnsdist: skip metrics with labels for carbon export and stats dump

64e9352

esensar requested a review from rgacogne December 12, 2024 18:37

dnsdist: remove todo comments from dnsdist-lua-ffi

9e9c3e5

rgacogne reviewed Dec 13, 2024

View reviewed changes

pdns/dnsdistdist/dnsdist-lua-ffi.cc Show resolved Hide resolved

pdns/dnsdistdist/dnsdist-lua-ffi.cc Show resolved Hide resolved

pdns/dnsdistdist/dnsdist-lua.cc Show resolved Hide resolved

pdns/dnsdistdist/dnsdist-lua.cc Show resolved Hide resolved

esensar and others added 3 commits December 13, 2024 16:02

dnsdist: ignore clangd-tidy warnings

60872f6

Co-authored-by: Remi Gacogne <github@coredump.fr>

dnsdist: update metrics related regression tests

ce95774

dnsdist: add docs for new arguments for custom metric functions

15bee38

esensar marked this pull request as ready for review December 16, 2024 10:32

esensar requested a review from rgacogne December 16, 2024 10:32

esensar added 2 commits December 16, 2024 11:42

dnsdist: use empty() instead of "" check

882718e

dnsdist: fix empty labels check in json stats endpoint

fc8822a

rgacogne requested changes Dec 16, 2024

View reviewed changes

pdns/dnsdistdist/docs/reference/custommetrics.rst Outdated Show resolved Hide resolved

pdns/dnsdistdist/docs/reference/custommetrics.rst Outdated Show resolved Hide resolved

pdns/dnsdistdist/docs/reference/custommetrics.rst Outdated Show resolved Hide resolved

esensar and others added 2 commits December 16, 2024 17:10

dnsdist: set version changed for new custom metrics options in docs

c625839

dnsdist: fix typos in custom metrics docs

b3f572f

Co-authored-by: Remi Gacogne <github@coredump.fr>

esensar requested a review from rgacogne December 16, 2024 16:54

rgacogne approved these changes Dec 17, 2024

View reviewed changes

rgacogne merged commit 34ede92 into PowerDNS:master Dec 17, 2024
77 checks passed

esensar deleted the feat/metric-labels branch December 17, 2024 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dnsdist: add support for labels on custom metrics #14956

dnsdist: add support for labels on custom metrics #14956

esensar commented Dec 12, 2024 •

edited

Loading

coveralls commented Dec 12, 2024 •

edited

Loading

rgacogne left a comment

rgacogne Dec 12, 2024

esensar Dec 12, 2024

rgacogne Dec 12, 2024

esensar Dec 12, 2024

esensar Dec 12, 2024

esensar commented Dec 12, 2024

omoerbeek commented Dec 12, 2024

esensar commented Dec 12, 2024

rgacogne left a comment

esensar commented Dec 16, 2024 •

edited

Loading

rgacogne left a comment

esensar commented Dec 16, 2024

rgacogne commented Dec 17, 2024

dnsdist: add support for labels on custom metrics #14956

dnsdist: add support for labels on custom metrics #14956

Conversation

esensar commented Dec 12, 2024 • edited Loading

Short description

Checklist

coveralls commented Dec 12, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12356677632

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

rgacogne left a comment

Choose a reason for hiding this comment

rgacogne Dec 12, 2024

Choose a reason for hiding this comment

esensar Dec 12, 2024

Choose a reason for hiding this comment

rgacogne Dec 12, 2024

Choose a reason for hiding this comment

esensar Dec 12, 2024

Choose a reason for hiding this comment

esensar Dec 12, 2024

Choose a reason for hiding this comment

esensar commented Dec 12, 2024

omoerbeek commented Dec 12, 2024

esensar commented Dec 12, 2024

rgacogne left a comment

Choose a reason for hiding this comment

esensar commented Dec 16, 2024 • edited Loading

rgacogne left a comment

Choose a reason for hiding this comment

esensar commented Dec 16, 2024

rgacogne commented Dec 17, 2024

esensar commented Dec 12, 2024 •

edited

Loading

coveralls commented Dec 12, 2024 •

edited

Loading

esensar commented Dec 16, 2024 •

edited

Loading