exporter: use only counter dump/schema commands for extacting counters #50718

avanthakkar · 2023-03-28T15:16:49Z

Fixes: https://tracker.ceph.com/issues/59191
Signed-off-by: Avan Thakkar athakkar@redhat.com

Ceph exporter no more required the output of perf dump/schema, as the counter dump command returns both labeled and unlabeled perf counters which exporter can fetch and export. Removed the exporter_get_labeled_counters config option as exporter will now export all the counters, labeled or unlabeled.
Also the fix includes the support for renaming the metrics name of rgw multi-site and adding labels to it, similar to what is there in prometheus module.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

src/exporter/DaemonMetricCollector.cc

epuertat · 2023-03-28T16:06:27Z

src/exporter/DaemonMetricCollector.cc

+  else if (daemon_name.find("rbd-mirror") != std::string::npos) {
+    std::regex re(
+        "^rbd_mirror_image_([^/]+)/(?:(?:([^/]+)/"
+        ")?)(.*)\\.(replay(?:_bytes|_latency)?)$");
+    std::smatch match;
+    if (std::regex_search(daemon_name, match, re) == true) {
+      new_metric_name = "ceph_rbd_mirror_image_" + match.str(4);
+      labels["pool"] = quote(match.str(1));
+      labels["namespace"] = quote(match.str(2));
+      labels["image"] = quote(match.str(3));


Are we also dropping this from the mgr/prometheus? Otherwise, they'll be duplicate entries, won't it?

Additionally, rather than introducing this transformation code in the ceph-exporter, I'd expect this to be provided by the daemons themselves. If not, we're trading a language (Python) which is super-friendly for data processing with bare C++ (tbh, this code is not complicate at all, but since we are introducing labeled perf-counters, why not go all the way?).

Are we also dropping this from the mgr/prometheus? Otherwise, they'll be duplicate entries, won't it?

They're already removed.

Additionally, rather than introducing this transformation code in the ceph-exporter, I'd expect this to be provided by the daemons themselves. If not, we're trading a language (Python) which is super-friendly for data processing with bare C++ (tbh, this code is not complicate at all, but since we are introducing labeled perf-counters, why not go all the way?).

epuertat · 2023-03-28T16:06:52Z

src/exporter/DaemonMetricCollector.cc

+// Add fixed name metrics from existing ones that have details in their names
+// that should be in labels (not in name). For backward compatibility,
+// a new fixed name metric is created (instead of replacing)and details are put
+// in new labels. Intended for RGW sync perf. counters but extendable as required.
+// See: https://tracker.ceph.com/issues/45311


Thanks for the comment! It's really helpful.

epuertat · 2023-03-28T16:07:31Z

src/exporter/DaemonMetricCollector.cc

+  std::regex re("^data_sync_from_(.*)\\.");
+    std::smatch match;
+    if (std::regex_search(metric_name, match, re) == true) {
+      new_metric_name = std::regex_replace(metric_name, re, "from_([^.]*)', 'from_zone");
+      labels["source_zone"] = quote(match.str(1));
+      return {labels, new_metric_name};
+    }


Same thing here: why not providing the labeled perf-counter right from the rgw daemon?

Well basically this is just addressing this #49248 (comment). To have support for renaming the rgw multi-site metrics and adding labels. And well other daemons can make use of it as well for e.g. any rbd metrics which requires renaming metrics name and adding them as label. So for now it's just rgw in future there maybe other daemons which may need this functionality.

Fixes: https://tracker.ceph.com/issues/59191 Signed-off-by: Avan Thakkar <athakkar@redhat.com> Ceph exporter no more required the output of perf dump/schema, as the ``counter dump`` command returns both labeled and unlabeled perf counters which exporter can fetch and export. Removed the ``exporter_get_labeled_counters`` confiug option as exporter will now export all the counters, labeled or unlabeled. Also the fix includes the support for renaming the metrics name of rgw multi-site and adding labels to it, similar to what is there in prometheus module.

idryomov

The PR is closed but I'm commenting anyway since I had some comments pending -- if you are tagging people for review, give them at least a couple of days to review before merging.

idryomov · 2023-03-29T19:42:16Z

src/exporter/DaemonMetricCollector.cc

+  else if (daemon_name.find("rbd-mirror") != std::string::npos) {
+    std::regex re(
+        "^rbd_mirror_image_([^/]+)/(?:(?:([^/]+)/"
+        ")?)(.*)\\.(replay(?:_bytes|_latency)?)$");


I think this is dead code -- rbd-mirror daemon doesn't report any non-labeled per-image counters anymore (starting in reef).

idryomov · 2023-03-29T19:53:53Z

src/exporter/DaemonMetricCollector.cc

+  if (daemon_name.find("radosgw") != std::string::npos) {
+    std::size_t pos = daemon_name.find_last_of('.');
+    std::string tmp = daemon_name.substr(pos+1);
+    labels["instance_id"] = quote(tmp);


I know nothing about why both "rgw" and "radosgw" branches are needed but this seems wrong to me. Given radosgw.<instance_id>.asok string, wouldn't the above three lines always produce asok string for the value of the label?

idryomov · 2023-03-29T19:56:46Z

src/exporter/DaemonMetricCollector.cc

+      json_object labeled_perf_group_object = labeled_perf.value().as_object();
+      auto counters = labeled_perf_group_object["counters"].as_object();
+      auto counters_labels = labeled_perf_group_object["labels"].as_object();
+      auto labeled_perf_group_counters = counter_dump[labeled_perf_group].as_object()["counters"].as_object();


All these variable names that start with "labeled" are confusing because counter dump includes both labeled and unlabeled counters. I would remove this prefix throughout.

idryomov · 2023-03-29T19:58:25Z

src/exporter/DaemonMetricCollector.cc

+    json_object counter_schema = boost::json::parse(counter_schema_response).as_object();
+
+    for (auto &labeled_perf : counter_schema) {
+      std::string labeled_perf_group = {labeled_perf.key().begin(), labeled_perf.key().end()};


This and a few other lines here are over 80 columns.

idryomov · 2023-03-29T20:00:40Z

src/exporter/DaemonMetricCollector.cc

+
+      for(auto &label: counters_labels) {
+        std::string label_key = {label.key().begin(), label.key().end()};
+        labels[label_key] = quote(label.value().as_string().c_str());


Is c_str needed given that quote takes std::string?

idryomov · 2023-03-29T20:24:39Z

src/exporter/DaemonMetricCollector.cc

+  new_metric_name = metric_name;
+
+  std::regex re("^data_sync_from_(.*)\\.");
+    std::smatch match;


indentation

szinn · 2023-04-11T13:41:47Z

@avanthakkar

This PR might have introduced a bug in 17.2.6

Just upgraded from 17.2.5 to 17.2.6 and prometheus is complaining about an ‘invalid metric type “counter”’ from one of the exporters. Grabbed the metrics file and ran it through promtool check metrics and it pointed to

# HELP ceph_mds_mem_cap+ Capabilities added               
# TYPE ceph_mds_mem_cap+ counter            
ceph_mds_mem_cap+{ceph_daemon="ceph-mds.ceph-filesystem-a"} 3   
ceph_mds_mem_cap+{ceph_daemon="ceph-mds.ceph-filesystem-b"} 0

with the message
error while linting: text format parsing error in line 762: invalid metric name in comment
It would appear that a metric name cannot have a ‘+’ in it

idryomov · 2023-04-11T14:21:59Z

@szinn This PR is only merged in main, it hasn't been backported anywhere.

szinn · 2023-04-11T14:32:52Z

@idryomov Right - the issue appears in 17.2.6 now. exporter is generating a metric with a '+' in it which is invalid and causes prometheus to mark the exporter pod with TargetDown.

This may not be the PR that caused the bug, but the issue is with 17.2.6

idryomov · 2023-04-11T14:45:20Z

Understood. I don't think anything changed around mds_mem_cap+ counter -- it has been there with a plus in its name for years. So I'm guessing that either it wasn't surfaced in 17.2.5 but now is (e.g. due to a difference in prio settings) or something changed with regards to how perf counter names are massaged. @jmolmo @epuertat PTAL!

szinn · 2023-04-11T15:15:53Z

Looking at my prometheus data, I see ceph_mds_mem_cap, ceph_mds_mem_cap_plus and ceph_mds_mem_cap_minus so perhaps it is just getting surfaced in .6 as you mentioned

idryomov · 2023-04-11T15:59:59Z

Are you seeing both mds_mem_cap+ and ceph_mds_mem_cap_plus in your dump on 17.2.6?

szinn · 2023-04-11T16:19:54Z

Here's what I see on 17.2.6

# HELP ceph_mds_mem_cap Capabilities
# TYPE ceph_mds_mem_cap gauge
ceph_mds_mem_cap{ceph_daemon="ceph-mds.ceph-filesystem-a"} 1
ceph_mds_mem_cap{ceph_daemon="ceph-mds.ceph-filesystem-b"} 0
# HELP ceph_mds_mem_cap+ Capabilities added
# TYPE ceph_mds_mem_cap+ counter
ceph_mds_mem_cap+{ceph_daemon="ceph-mds.ceph-filesystem-a"} 3
ceph_mds_mem_cap+{ceph_daemon="ceph-mds.ceph-filesystem-b"} 0
# HELP ceph_mds_mem_cap_ Capabilities removed
# TYPE ceph_mds_mem_cap_ counter
ceph_mds_mem_cap_{ceph_daemon="ceph-mds.ceph-filesystem-a"} 2
ceph_mds_mem_cap_{ceph_daemon="ceph-mds.ceph-filesystem-b"} 0

It would appear that _minus now shows as _ and _plus now shows as +

The _plus and _minus were perhaps from 17.2.5. I just did a search in the prometheus graph for the prefix.

epuertat · 2023-04-11T17:52:49Z

@szinn this specific PR is not backported to 17.2.6, but the new ceph-exporter is there (I'd assume that disabled by default).

I see that the sanitizing code in the mgr/prometheus is still in 17.2.6:

ceph/src/pybind/mgr/prometheus/module.py

Lines 329 to 338 in d7ff0d1

    
           def promethize(path: str) -> str: 
        
               ''' replace illegal metric name characters ''' 
        
               result = re.sub(r'[./\s]|::', '_', path).replace('+', '_plus') 
        
               # Hyphens usually turn into underscores, unless they are 
        
               # trailing 
        
               if result.endswith("-"): 
        
                   result = result[0:-1] + "_minus" 
        
               else: 
        
                   result = result.replace("-", "_")

But there's no such code in the ceph-exporter daemon. The only explanation would be that these metrics are coming from the ceph-exporter instead of from the mgr/prometheus exporter.

@avanthakkar, @pereman2: could you plz confirm what's the status of ceph-exporter in Quincy/17.2.6?

szinn · 2023-04-11T17:56:19Z

@avanthakkar @pereman2 @epuertat Thanks for looking into this - even though it doesn't appear to impact functionality, it does mean that metrics aren't being pulled from one exporter which would stop any kind of alerting of issues off of that exporter. That would be a serious issue

idryomov · 2023-04-11T19:55:37Z

this specific PR is not backported to 17.2.6, but the new ceph-exporter is there (I'd assume that disabled by default).

@epuertat What do you mean by the new ceph-exporter? ceph-exporter was there in 17.2.5 and not much changed under src/exporter between 17.2.5 and 17.2.6:

33e58e2 (originally from exporter: don't skip loop if pid path is empty #48166)
0a83b88 (originally from exporter: avoid stoi for empty pid_str #48193)

The only explanation would be that these metrics are coming from the ceph-exporter instead of from the mgr/prometheus exporter.

Agreed... ceph_mds_mem_cap_ data point confirms this because the actual counter name is cap-. ceph-exporter would prepend ceph_ and substitute _ for - here:

ceph/src/exporter/DaemonMetricCollector.cc

Lines 148 to 149 in d7ff0d1

    
           std::string name = "ceph_" + perf_group + "_" + perf_name; 
        
           std::replace_if(name.begin(), name.end(), is_hyphen, '_');

jmolmo · 2023-04-12T12:23:24Z

Probably the right thing to do is to produce the perf counter with the right name in each daemon.

Obviously, we can fix the problem in Ceph exporter, (and continue making different assumptions about how to replace each invalid character in the perf counter name), but if we were fixed the problem in the mds daemon the first time the problem arose, now we will not find this problem again.

In the case of mds, (what i find taking a look to the offensive counter) we have four counters with the same problem:

ceph/src/mds/MDSRank.cc

Line 3514 in 094206c

mdm_plb.add_u64_counter(l_mdm_inoa, "ino+", "Inodes opened");

In fact the use of the signs "+" or "-" in the counters has different meaning depending of the counter.
"+" = "opened" or "added"
"-" = "closed" or "removed"

@vshankar Do you think that to change the name of these counters is possible (any possible regression), or should we continue doing the "syntactic" replacement in the new ceph exporter?

idryomov · 2023-04-12T12:36:31Z

@jmolmo Putting aside the choice between carrying over massaging code from mgr/prometheus module to the exporter daemon and changing the perf counters themselves (which are many years old!), can you link the PR that actually caused the regression in 17.2.6? A few comments above @epuertat said that

the new ceph-exporter is there (I'd assume that disabled by default).

What is new about ceph-exporter or how metrics are surfaced in 17.2.6? And, whatever it is, it doesn't appear to be disabled by default?

vshankar · 2023-04-12T14:47:10Z

@vshankar Do you think that to change the name of these counters is possible (any possible regression), or should we continue doing the "syntactic" replacement in the new ceph exporter?

I need to check. Will get back asap.

epuertat · 2023-04-12T16:09:29Z

@jmolmo Prometheus is only one way to expose perf-counters, but Ceph supports others: InfluxDB/Telegraf/Zabbix. Each one could have different requirements for metrics names, so I don't think that each Ceph daemon individually should deal with that. IMHO it corresponds to the adaptation layer (ceph-exporter) to sanitize each perf-counter name to make it suitable for each monitoring framework.

Different question is doing complicate metric renamings, like the one here. I think that those transformations should be provided by the daemons themselves.

epuertat · 2023-04-12T16:53:31Z

@szinn we have discussed this topic in today's CLT meeting:

Cause

The specific PRs that introduced this issue were:

Cephadm: quincy: ceph-exporter: cephadm changes #49771 which landed to 17.2.6.
Rook: core: introduce ceph-exporter rook/rook#11317

Those PR allowed Cephadm/Rook to deploy the new (17.2.5) ceph-exporter, which was meant (among other things) to resolve scalability/performance issues with the existing centralized mgr/prometheus exporter. However, this new service should have never been enabled by default in Quincy, as it was meant to only be (experimentally) used by those deployments with scalability issues.

Impact

Cephadm: As per our preliminary analysis it should only affect new deployments, not upgrades on existing ones, since services are only automatically deployed during cephadm bootstrap stage.
Rook: in this case it affects both new and existing deployments.

The impact would be:

Ceph metrics could be duplicated: from the existing centralized mgr/prometheus exporter (legacy approach), and from the new ceph-exporter, which is deployed as a side-car container on each node and reports Ceph metrics for all colocated services in that node.
In a few cases (e.g.: ceph_mds_mem_cap+), where those metrics contain characters not supported by Prometheus (e.g.: +), Prometheus would complain about malformed metric names (however those metrics will still be properly reported via mgr/prometheus as ceph_mds_mem_cap_plus).
In a few cases (e.g.: ceph_mds_mem_cap_minus), with metrics ending in -, they would have different names in Prometheus (e.g.: ceph_mds_mem_cap_minus when coming from mgr/prometheus and ceph_mds_mem_cap_ when coming from ceph-exporter).

Workaround

Cephadm:
- For upgrades on existing deployments no impact is expected, hence no workaround would be required.
- For fresh installs with cephadm bootstrap (Rook-based deployments are unaffected), the --skip-monitoring-stack flag could be enabled not to install this new ceph-exporter service. This will also skip the installation of the rest of monitoring services (prometheus, grafana, node-exporter, alertmanager), which should be manually deployed after the bootstrap stage.
- For upgrades already facing this issue: ceph orch rm ceph-exporter should be enough to prune this service.
Rook:
- ~~Remove the ceph-exporter service: kubectl delete -f exporter-service-monitor.yaml~~ (reported not to work by @szinn; waiting for @travisn for a workaround).

Next Actions

As announced in the CLT minutes today, a follow-up hotfix will be released as soon as possible reverting/fixing these changes: quincy: ceph-exporter: cephadm changes #49771 and core: introduce ceph-exporter rook/rook#11317
A fix to properly sanitize Prometheus metrics names in ceph-exporter will be delivered to main and reef.

Retrospective

Review the existing integration tests for monitoring.
Tighten up the backport screening process (suggestion to consider adopting best-practices as Conventional Commits).

CC: @adk3798 @avanthakkar @idryomov @jmolmo @nizamial09 @yuriw

jmolmo · 2023-04-12T17:03:21Z

Thanks @epuertat for the detailed clarifications.

szinn · 2023-04-12T17:24:52Z

Thanks for the writeup as well - This did show up when I upgraded to 17.2.6 from 17.2.5 via rook-ceph. I didl try the ceph orch command and it indicated there was no orchestrator configured.

epuertat · 2023-04-12T17:35:16Z

Thanks for the writeup as well - This did show up when I upgraded to 17.2.6 from 17.2.5 via rook-ceph. I will try the ceph orch command. If I understand you correctly, the ceph prometheus metrics will still be available via the previous mechanism.

Thanks for the heads up, @szinn. I completely ignored that Rook was already deploying this service. I'll update the notes above. Thanks again!

The ceph orch command is not likely to work on Rook deployments. I guess you'll have to run a kubectl delete -f <...>. @travisn: could you please help here? Thanks!

szinn · 2023-04-12T17:49:13Z

Yes, as of 17.2.6, rook is deploying Ceph-exporter and the service monitor instance to go with it. Unfortunately deleting the service monitor and Ceph-exporter won't work since the rook-operator will just put them back again. Will likely have to wait for either a fix from @travisn or wait until Ceph-exporter is corrected to sanitize all metric names before I can upgrade.

The expected ceph version for exporter is changed temporarily because some regression was detected in Ceph version 17.2.6 which is summarised here ceph/ceph#50718 (comment). Thus, disabling ceph-exporter for now until all the regression are fixed. Signed-off-by: avanthakkar <avanjohn@gmail.com>

avanthakkar requested review from idryomov, pereman2 and epuertat March 28, 2023 15:16

github-actions bot added the common label Mar 28, 2023

This was referenced Mar 28, 2023

mgr/prometheus: introduce exclude_perf_counters module option #49248

Merged

exporter: add support for exposing labeled perf counters #50369

Merged

avanthakkar added the wip-athakkar-testing label Mar 28, 2023

avanthakkar marked this pull request as ready for review March 28, 2023 15:30

epuertat reviewed Mar 28, 2023

View reviewed changes

avanthakkar force-pushed the use-only-counter-dump-schema branch from c81e3b9 to 51a8990 Compare March 28, 2023 18:12

pereman2 approved these changes Mar 29, 2023

View reviewed changes

avanthakkar merged commit ef6b9ae into ceph:main Mar 29, 2023
11 of 12 checks passed

avanthakkar deleted the use-only-counter-dump-schema branch March 29, 2023 08:52

This was referenced Mar 29, 2023

reef: exporter: user only counter dump/schema commands for extacting counters #50748

Merged

quincy: exporter: user only counter dump/schema commands for extacting counters #50749

Merged

idryomov reviewed Mar 29, 2023

View reviewed changes

avanthakkar changed the title ~~exporter: user only counter dump/schema commands for extacting counters~~ exporter: use only counter dump/schema commands for extacting counters Apr 12, 2023

epuertat added the regression label Apr 12, 2023

avanthakkar mentioned this pull request Apr 12, 2023

core: disable exporter for Ceph v17.2.6 rook/rook#12077

Merged

7 tasks

exporter: use only counter dump/schema commands for extacting counters #50718

exporter: use only counter dump/schema commands for extacting counters #50718

avanthakkar commented Mar 28, 2023 •

edited

epuertat Mar 28, 2023

avanthakkar Mar 28, 2023

epuertat Mar 28, 2023

epuertat Mar 28, 2023

avanthakkar Mar 28, 2023

idryomov left a comment

idryomov Mar 29, 2023

idryomov Mar 29, 2023

idryomov Mar 29, 2023

idryomov Mar 29, 2023

idryomov Mar 29, 2023

idryomov Mar 29, 2023

szinn commented Apr 11, 2023 •

edited

idryomov commented Apr 11, 2023

szinn commented Apr 11, 2023 •

edited

idryomov commented Apr 11, 2023 •

edited

szinn commented Apr 11, 2023

idryomov commented Apr 11, 2023

szinn commented Apr 11, 2023 •

edited by epuertat

epuertat commented Apr 11, 2023

szinn commented Apr 11, 2023

idryomov commented Apr 11, 2023 •

edited

jmolmo commented Apr 12, 2023

idryomov commented Apr 12, 2023 •

edited

vshankar commented Apr 12, 2023

epuertat commented Apr 12, 2023

epuertat commented Apr 12, 2023 •

edited

jmolmo commented Apr 12, 2023

szinn commented Apr 12, 2023 •

edited

epuertat commented Apr 12, 2023

szinn commented Apr 12, 2023

exporter: use only counter dump/schema commands for extacting counters #50718

exporter: use only counter dump/schema commands for extacting counters #50718

Conversation

avanthakkar commented Mar 28, 2023 • edited

Contribution Guidelines

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idryomov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szinn commented Apr 11, 2023 • edited

idryomov commented Apr 11, 2023

szinn commented Apr 11, 2023 • edited

idryomov commented Apr 11, 2023 • edited

szinn commented Apr 11, 2023

idryomov commented Apr 11, 2023

szinn commented Apr 11, 2023 • edited by epuertat

epuertat commented Apr 11, 2023

szinn commented Apr 11, 2023

idryomov commented Apr 11, 2023 • edited

jmolmo commented Apr 12, 2023

idryomov commented Apr 12, 2023 • edited

vshankar commented Apr 12, 2023

epuertat commented Apr 12, 2023

epuertat commented Apr 12, 2023 • edited

Cause

Impact

Workaround

Next Actions

Retrospective

jmolmo commented Apr 12, 2023

szinn commented Apr 12, 2023 • edited

epuertat commented Apr 12, 2023

szinn commented Apr 12, 2023

avanthakkar commented Mar 28, 2023 •

edited

szinn commented Apr 11, 2023 •

edited

szinn commented Apr 11, 2023 •

edited

idryomov commented Apr 11, 2023 •

edited

szinn commented Apr 11, 2023 •

edited by epuertat

idryomov commented Apr 11, 2023 •

edited

idryomov commented Apr 12, 2023 •

edited

epuertat commented Apr 12, 2023 •

edited

szinn commented Apr 12, 2023 •

edited