exporter: promethize counter names #51069

pereman2 · 2023-04-13T16:57:59Z

This fix assures that metrics coming from daemons perf counters have the same name if they are exported by the Prometheus manager module or by the Ceph exporter.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2186557

src/exporter/DaemonMetricCollector.cc

idryomov · 2023-04-13T17:14:09Z

src/exporter/DaemonMetricCollector.cc

+
+  // just in case because there are some bluestore metrics which include a single ":"
+  if (name.find(":") != std::string::npos) {
+    boost::replace_all(name, ":", "_");


I don't see this in promethize in src/pybind/mgr/prometheus/module.py?

as the comment says, I saw a single colon around some bluestore metrics so I added that in just in case.

Then promethize in src/pybind/mgr/prometheus/module.py needs to updated to the same. There can't be "just in case" here, both methods should do exactly the same thing.

Yup, it's important to keep consistency with the existing metric names (even if the current approach was not the best possible). Otherwise, this will generate new time-series, and the existing Grafana Dashboards might have missing data.

Maybe those bluestore metrics are not exported due to their priority level?

jmolmo · 2023-04-13T17:17:08Z

Output with the new code. the "_", "+" has been properly replaced.

(standard input)752:ceph_mds_mem_cap_minus{ceph_daemon="mds.a"} 0
(standard input)753:# HELP ceph_mds_mem_cap_plus Capabilities added
(standard input)754:# TYPE ceph_mds_mem_cap_plus counter
(standard input)755:ceph_mds_mem_cap_plus{ceph_daemon="mds.a"} 0
(standard input)756:# HELP ceph_mds_mem_dir Directories
(standard input)757:# TYPE ceph_mds_mem_dir gauge
(standard input)758:ceph_mds_mem_dir{ceph_daemon="mds.a"} 12
(standard input)759:# HELP ceph_mds_mem_dir_minus Directories closed

idryomov · 2023-04-13T18:14:11Z

Output with the new code. the "_", "+" has been properly replaced.

Only because + happens to be the last character in this example.

pereman2 · 2023-04-14T09:17:01Z

I made the code replace all + with _plus just in case. Expecting a + as an infix will be a weird case so I don't think is something we should care about for now.

Another thing we could add is replace all invalid characters from the prometheus name regex they provide officially to underscores or something simliar but I don't feel like we need to reach that far for the current quick fix.

src/exporter/DaemonMetricCollector.cc

epuertat

LGTM! Thank you, @pereman2 !

BTW, I see no "Fixes:...". Shouldn't this be linked to a bug report?

src/test/exporter/promethize_data.h

src/exporter/DaemonMetricCollector.cc

jmolmo · 2023-04-17T08:32:37Z

LGTM! Thank you, @pereman2 !

BTW, I see no "Fixes:...". Shouldn't this be linked to a bug report?

@pereman2 was so fast fixing this , that the bug creation took more time than post the pr ... :-D ,..
https://bugzilla.redhat.com/show_bug.cgi?id=2186557

@pereman2, please, be sure to update the commit

pereman2 · 2023-04-18T06:08:40Z

@jmolmo @epuertat Added the fixes section.

jmolmo · 2023-04-18T08:48:53Z

The windows tests fail with an error in one of the exporter files, the weird thing is that the offending line has not been modified.... and the line was introduced 9 monthsd ago ...
/home/ubuntu/ceph/src/exporter/DaemonMetricCollector.cc:245:27: error: ‘_SC_CLK_TCK’ was not declared in this scope; did you mean ‘CLK_TCK’?

@pereman2 what do you think?

idryomov · 2023-04-18T17:29:55Z

The windows tests fail with an error in one of the exporter files, the weird thing is that the offending line has not been modified.... and the line was introduced 9 monthsd ago ...
/home/ubuntu/ceph/src/exporter/DaemonMetricCollector.cc:245:27: error: ‘_SC_CLK_TCK’ was not declared in this scope; did you mean ‘CLK_TCK’?

It's failing to build the test, not the daemon. I think the failure appeared because ceph-exporter isn't built for Windows but with this PR you are trying to build its test binary. You would need to disable that in src/test/CMakeLists.txt by moving add_subdirectory(exporter) line into the "Not available on Windows for the time being" section.

Cc @petrutlucian94 to keep me honest.

src/exporter/DaemonMetricCollector.cc

src/test/exporter/promethize_data.h

src/exporter/DaemonMetricCollector.cc

petrutlucian94 · 2023-04-19T09:25:19Z

The windows tests fail with an error in one of the exporter files, the weird thing is that the offending line has not been modified.... and the line was introduced 9 monthsd ago ...
/home/ubuntu/ceph/src/exporter/DaemonMetricCollector.cc:245:27: error: ‘_SC_CLK_TCK’ was not declared in this scope; did you mean ‘CLK_TCK’?

It's failing to build the test, not the daemon. I think the failure appeared because ceph-exporter isn't built for Windows but with this PR you are trying to build its test binary. You would need to disable that in src/test/CMakeLists.txt by moving add_subdirectory(exporter) line into the "Not available on Windows for the time being" section.

Cc @petrutlucian94 to keep me honest.

That's correct, please skip this test on Windows. Thanks!

src/exporter/DaemonMetricCollector.cc

src/exporter/util.h

src/test/exporter/CMakeLists.txt

jmolmo · 2023-04-21T09:02:15Z

jenkins test make check

jmolmo · 2023-04-21T09:02:42Z

jenkins test make check arm64

jmolmo · 2023-04-21T10:11:21Z

jenkins test make check arm64

src/test/exporter/test_exporter.cc

src/exporter/DaemonMetricCollector.cc

avanthakkar · 2023-04-24T12:19:49Z

jenkins test dashboard cephadm

avanthakkar · 2023-04-24T12:21:08Z

jenkins test dashboard cephadm

avanthakkar · 2023-04-24T12:47:30Z

jenkins test dashboard cephadm

jmolmo · 2023-04-24T13:02:00Z

jenkins test dashboard cephadm

avanthakkar · 2023-04-24T13:09:01Z

jenkins test dashboard cephadm

avanthakkar · 2023-04-24T13:47:18Z

jenkins test make check

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> Fixes: https://tracker.ceph.com/issues/59475

pereman2 · 2023-04-24T18:10:58Z

@idryomov I've fixed the last comments from you.

@avanthakkar can you give a brief on why this commit was added ? 4998f85 I've removed it completely as you didn't give any explanations on the commit plus it didn't follow naming conventions of commit messages.

avanthakkar · 2023-04-25T10:46:31Z

@idryomov I've fixed the last comments from you.

@avanthakkar can you give a brief on why this commit was added ? 4998f85 I've removed it completely as you didn't give any explanations on the commit plus it didn't follow naming conventions of commit messages.

I temporarily added it to see the cephadm grafana e2e tests are passing or not without enabling the feature for fetching perf counters from prometheus module just to verify if nothing is breaking in any grafana dashboards.

idryomov

Approving based on

For sure a more complete test coverage will be useful, and probably we need to set as priority. However, I suggest to be focused in this pr just in the problem to resolve.

As @jmolmo said, we should stick with the current purpose of this PR and @avanthakkar can work on on extending the testing suite.

@avanthakkar Can you link the respective tracker ticket here?

idryomov · 2023-04-25T11:45:01Z

jenkins test windows

avanthakkar · 2023-04-26T07:20:06Z

jenkins test dashboard

jmolmo · 2023-04-26T08:50:36Z

Ceph dashboard test failures not related with this change

pereman2 · 2023-04-26T14:24:11Z

@idryomov As per your request in the CLT, I've created this tracker https://tracker.ceph.com/issues/59561 (we can split it as necessary) to track the integration tests on the exporter.

cc: @jmolmo @avanthakkar

exporter: promethize counter names

pereman2 requested review from idryomov, jmolmo, epuertat and ljflores April 13, 2023 16:57

idryomov reviewed Apr 13, 2023

View reviewed changes

pereman2 force-pushed the exporter-prom branch from 5e3cd1c to 80fe8ec Compare April 14, 2023 09:14

epuertat requested changes Apr 14, 2023

View reviewed changes

src/exporter/DaemonMetricCollector.cc Outdated Show resolved Hide resolved

github-actions bot added build/ops tests labels Apr 14, 2023

epuertat approved these changes Apr 14, 2023

View reviewed changes

src/test/exporter/promethize_data.h Outdated Show resolved Hide resolved

src/exporter/DaemonMetricCollector.cc Show resolved Hide resolved

jmolmo approved these changes Apr 17, 2023

View reviewed changes

pereman2 force-pushed the exporter-prom branch from c1f028d to c43f071 Compare April 18, 2023 06:07

idryomov reviewed Apr 18, 2023

View reviewed changes

pereman2 force-pushed the exporter-prom branch from c43f071 to 526811c Compare April 19, 2023 10:42

idryomov reviewed Apr 19, 2023

View reviewed changes

src/exporter/DaemonMetricCollector.cc Outdated Show resolved Hide resolved

src/exporter/util.h Outdated Show resolved Hide resolved

src/test/exporter/CMakeLists.txt Outdated Show resolved Hide resolved

pereman2 force-pushed the exporter-prom branch from 526811c to b222f4b Compare April 21, 2023 07:26

github-actions bot added monitoring pybind labels Apr 21, 2023

avanthakkar added the wip-athakkar-testing label Apr 21, 2023

idryomov reviewed Apr 21, 2023

View reviewed changes

src/test/exporter/test_exporter.cc Show resolved Hide resolved

src/exporter/DaemonMetricCollector.cc Outdated Show resolved Hide resolved

avanthakkar requested a review from a team as a code owner April 24, 2023 12:47

avanthakkar requested review from nizamial09 and removed request for a team April 24, 2023 12:47

github-actions bot added the dashboard label Apr 24, 2023

avanthakkar changed the title ~~exporter: promethize counter names~~ exporter: promethize counter names Apr 24, 2023

exporter: promethize counter names

7087e76

Signed-off-by: Pere Diaz Bou <pere-altea@hotmail.com> Fixes: https://tracker.ceph.com/issues/59475

pereman2 force-pushed the exporter-prom branch from 4998f85 to 7087e76 Compare April 24, 2023 18:09

idryomov approved these changes Apr 25, 2023

View reviewed changes

jmolmo merged commit 11b3cc2 into ceph:main Apr 28, 2023
11 of 13 checks passed

jmolmo deleted the exporter-prom branch April 28, 2023 06:21

yuvalif pushed a commit to yuvalif/ceph that referenced this pull request Apr 30, 2023

Merge pull request ceph#51069 from rhcs-dashboard/exporter-prom

48545fe

exporter: promethize counter names

avanthakkar mentioned this pull request May 29, 2023

reef: exporter: promethize counter names #51813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exporter: promethize counter names #51069

exporter: promethize counter names #51069

pereman2 commented Apr 13, 2023 •

edited by jmolmo

idryomov Apr 13, 2023

pereman2 Apr 14, 2023

idryomov Apr 14, 2023

epuertat Apr 14, 2023

jmolmo commented Apr 13, 2023

idryomov commented Apr 13, 2023 •

edited

pereman2 commented Apr 14, 2023

epuertat left a comment

jmolmo commented Apr 17, 2023 •

edited

pereman2 commented Apr 18, 2023

jmolmo commented Apr 18, 2023

idryomov commented Apr 18, 2023

petrutlucian94 commented Apr 19, 2023

jmolmo commented Apr 21, 2023

jmolmo commented Apr 21, 2023

jmolmo commented Apr 21, 2023

avanthakkar commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

jmolmo commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

pereman2 commented Apr 24, 2023

avanthakkar commented Apr 25, 2023

idryomov left a comment

idryomov commented Apr 25, 2023

avanthakkar commented Apr 26, 2023

jmolmo commented Apr 26, 2023

pereman2 commented Apr 26, 2023

exporter: promethize counter names #51069

exporter: promethize counter names #51069

Conversation

pereman2 commented Apr 13, 2023 • edited by jmolmo

idryomov Apr 13, 2023

Choose a reason for hiding this comment

pereman2 Apr 14, 2023

Choose a reason for hiding this comment

idryomov Apr 14, 2023

Choose a reason for hiding this comment

epuertat Apr 14, 2023

Choose a reason for hiding this comment

jmolmo commented Apr 13, 2023

idryomov commented Apr 13, 2023 • edited

pereman2 commented Apr 14, 2023

epuertat left a comment

Choose a reason for hiding this comment

jmolmo commented Apr 17, 2023 • edited

pereman2 commented Apr 18, 2023

jmolmo commented Apr 18, 2023

idryomov commented Apr 18, 2023

petrutlucian94 commented Apr 19, 2023

jmolmo commented Apr 21, 2023

jmolmo commented Apr 21, 2023

jmolmo commented Apr 21, 2023

avanthakkar commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

jmolmo commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

avanthakkar commented Apr 24, 2023

pereman2 commented Apr 24, 2023

avanthakkar commented Apr 25, 2023

idryomov left a comment

Choose a reason for hiding this comment

idryomov commented Apr 25, 2023

avanthakkar commented Apr 26, 2023

jmolmo commented Apr 26, 2023

pereman2 commented Apr 26, 2023

pereman2 commented Apr 13, 2023 •

edited by jmolmo

idryomov commented Apr 13, 2023 •

edited

jmolmo commented Apr 17, 2023 •

edited