mgr: Fix ceph_daemon label in ceph_rgw_* metrics #43707

BenoitKnecht · 2021-10-28T17:14:48Z

The DaemonStateCollection used to always contain the daemon name in its
DaemonKey, but since #40220 (or more specifically
afc3375), the RadosGW registers with its
instance ID instead (rados.get_instance_id()).

As a result, the ceph_rgw_* metrics returned by ceph-mgr through the
prometheus module have their ceph_daemon label include that ID instead of
the daemon name, e.g.

ceph_rgw_req{ceph_daemon="rgw.127202"}

instead of

ceph_rgw_req{ceph_daemon="rgw.my-hostname.rgw0"}

This PR replace the ceph_daemon label with an instance_id label on all
ceph_rgw_* metrics. The old ceph_daemon label can then be added back to any
ceph_rgw_* metric using the following PromQL query, for instance:

ceph_rgw_req * on (instance_id) group_left(ceph_daemon) ceph_rgw_metadata

Fixes: https://tracker.ceph.com/issues/51120
Signed-off-by: Benoît Knecht bknecht@protonmail.ch

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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

BenoitKnecht · 2021-10-28T17:41:48Z

See https://tracker.ceph.com/issues/51120#note-7 for why this is still a draft.

pereman2

Shouldn't this PR adress all existing queries that will have to change (monitoring/grafana/dashboards/)?

BenoitKnecht · 2021-12-03T09:17:57Z

@pereman2 That's a good point, I can definitely add that.

BenoitKnecht · 2022-01-05T22:08:15Z

I've updated the dashboards and added tests to cover the modified panels, so this PR should be ready for another review.

github-actions · 2022-01-11T11:26:15Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

The `DaemonStateCollection` used to always contain the daemon name in its `DaemonKey`, but since ceph#40220 (or more specifically afc3375), the RadosGW registers with its instance ID instead (`rados.get_instance_id()`). As a result, the `ceph_rgw_*` metrics returned by `ceph-mgr` through the `prometheus` module have their `ceph_daemon` label include that ID instead of the daemon name, e.g. ``` ceph_rgw_req{ceph_daemon="rgw.127202"} ``` instead of ``` ceph_rgw_req{ceph_daemon="rgw.my-hostname.rgw0"} ``` This commit adds the daemon name from `state->metadata["id"]` if available, as `service.name` in the JSON document returned by `dump_server()`. Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

Now that the RadosGW returns its instance ID instead of its daemon name, replace the `ceph_daemon` label with an `instance_id` label on the `rgw` metrics. Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

In order to get the `ceph_daemon` label for `rgw` metrics corresponding to the value before ceph#40220, we need to add the `instance_id` label to the `ceph_rgw_metadata` metric. This way, the old `ceph_daemon` label can be added to any `ceph_rgw_*` metric using the following PromQL query, for instance: ``` ceph_rgw_req * on (instance_id) group_left(ceph_daemon) ceph_rgw_metadata ``` Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

With the `ceph_daemon` label now replaced by `instance_id` on all `ceph_rgw_*` metrics, we need to update Grafana dashboards get that label back from `ceph_rgw_metadata` using this type of construct: ``` ceph_rgw_req * on (instance_id) group_left(ceph_daemon) ceph_rgw_metadata ``` Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

Some of the expressions modified in c402903 were not covered by any tests, especially those in the `radosgw-detail.json` dashboard. This commit fills in those gaps. Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

BenoitKnecht · 2022-01-11T15:34:08Z

make check (arm64) seems to be failing for some unrelated reason.

pereman2

hey, sorry for the delay. I've been testing this and I was looking at ceph_rgw_metadata and ceph_rgw_req, but ceph_daemon label wasn't filled in any of them.

BenoitKnecht · 2022-01-18T12:47:41Z

Hi @pereman2, no worries, and thanks for the review!

It's very strange, I just tried again on a vstart cluster, and I get the expected labels:

$ RGW=1 ../src/vstart.sh --debug --new -x --localhost --bluestore
$ ./bin/ceph mgr module enable prometheus
$ curl -s localhost:9283/metrics | grep ^ceph_rgw_
ceph_rgw_metadata{ceph_daemon="rgw.8000",hostname="5e7ace7cdc93",ceph_version="ceph version Development (no_version) quincy (dev)",instance_id="4425"} 1.0
ceph_rgw_cache_hit{instance_id="4425"} 0.0
ceph_rgw_cache_miss{instance_id="4425"} 10.0
ceph_rgw_failed_req{instance_id="4425"} 0.0
ceph_rgw_gc_retire_object{instance_id="4425"} 0.0
ceph_rgw_get{instance_id="4425"} 0.0
ceph_rgw_get_b{instance_id="4425"} 0.0
ceph_rgw_get_initial_lat_sum{instance_id="4425"} 0.0
ceph_rgw_get_initial_lat_count{instance_id="4425"} 0.0
ceph_rgw_keystone_token_cache_hit{instance_id="4425"} 0.0
ceph_rgw_keystone_token_cache_miss{instance_id="4425"} 0.0
ceph_rgw_lc_abort_mpu{instance_id="4425"} 0.0
ceph_rgw_lc_expire_current{instance_id="4425"} 0.0
ceph_rgw_lc_expire_dm{instance_id="4425"} 0.0
ceph_rgw_lc_expire_noncurrent{instance_id="4425"} 0.0
ceph_rgw_lc_transition_current{instance_id="4425"} 0.0
ceph_rgw_lc_transition_noncurrent{instance_id="4425"} 0.0
ceph_rgw_pubsub_event_lost{instance_id="4425"} 0.0
ceph_rgw_pubsub_event_triggered{instance_id="4425"} 0.0
ceph_rgw_pubsub_events{instance_id="4425"} 0.0
ceph_rgw_pubsub_missing_conf{instance_id="4425"} 0.0
ceph_rgw_pubsub_push_failed{instance_id="4425"} 0.0
ceph_rgw_pubsub_push_ok{instance_id="4425"} 0.0
ceph_rgw_pubsub_push_pending{instance_id="4425"} 0.0
ceph_rgw_pubsub_store_fail{instance_id="4425"} 0.0
ceph_rgw_pubsub_store_ok{instance_id="4425"} 0.0
ceph_rgw_put{instance_id="4425"} 0.0
ceph_rgw_put_b{instance_id="4425"} 0.0
ceph_rgw_put_initial_lat_sum{instance_id="4425"} 0.0
ceph_rgw_put_initial_lat_count{instance_id="4425"} 0.0
ceph_rgw_qactive{instance_id="4425"} 0.0
ceph_rgw_qlen{instance_id="4425"} 0.0
ceph_rgw_req{instance_id="4425"} 0.0

Are you sure you were running a version of ceph-mgr compiled from this branch?

pereman2

Recompiled and worked :P, LGTM. Thanks @BenoitKnecht

BenoitKnecht · 2022-02-01T16:36:13Z

Has anyone else had a chance to review this?

pereman2 · 2022-02-01T17:10:40Z

@aaSharma14 @epuertat ping

aaSharma14

LGTM.!

aaSharma14 · 2022-02-02T09:19:06Z

jenkins test make check arm64

epuertat

Great work @BenoitKnecht ! This is overlapping with #44059, but given this is fixing a bug, I'd rather prioritize this over that one. @MrFreezeex are you ok with having this one merged first? I hope this doesn't mean much rebase for you...

epuertat · 2022-02-02T12:12:31Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

@@ -494,7 +494,7 @@ local addStyle(alias, colorMode, colors, dateFormat, decimals, mappingType, patt
        'Latencies are shown stacked, without a yaxis to provide a visual indication of GET latency imbalance across RGW hosts',
        's',
        'short',
-        'label_replace(rate(ceph_rgw_get_initial_lat_sum[30s]),"rgw_host","$1","ceph_daemon","rgw.(.*)") / \nlabel_replace(rate(ceph_rgw_get_initial_lat_count[30s]),"rgw_host","$1","ceph_daemon","rgw.(.*)")',
+        'label_replace(\n    rate(ceph_rgw_get_initial_lat_sum[30s]) /\n    rate(ceph_rgw_get_initial_lat_count[30s]) *\n    on (instance_id) group_left (ceph_daemon) ceph_rgw_metadata,\n"rgw_host", "$1", "ceph_daemon", "rgw.(.*)")',


Let's better use jsonnet verbatims (|||) instead of explicit line breaks:

Suggested change

'label_replace(\n rate(ceph_rgw_get_initial_lat_sum[30s]) /\n rate(ceph_rgw_get_initial_lat_count[30s]) *\n on (instance_id) group_left (ceph_daemon) ceph_rgw_metadata,\n"rgw_host", "$1", "ceph_daemon", "rgw.(.*)")',

|||

label_replace(

rate(ceph_rgw_get_initial_lat_sum[30s]) /

rate(ceph_rgw_get_initial_lat_count[30s]) *

on (instance_id) group_left (ceph_daemon) ceph_rgw_metadata,

"rgw_host", "$1", "ceph_daemon", "rgw.(.*)")

|||,

epuertat · 2022-02-02T12:13:46Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

@@ -520,7 +520,7 @@ local addStyle(alias, colorMode, colors, dateFormat, decimals, mappingType, patt
        'Total bytes transferred in/out through get/put operations, by radosgw instance',
        'bytes',
        'short',
-        'sum by(rgw_host) (\n  (label_replace(rate(ceph_rgw_get_b[30s]), "rgw_host","$1","ceph_daemon","rgw.(.*)")) + \n  (label_replace(rate(ceph_rgw_put_b[30s]), "rgw_host","$1","ceph_daemon","rgw.(.*)"))\n)',
+        'label_replace(sum by (instance_id) (\n    rate(ceph_rgw_get_b[30s]) + \n    rate(ceph_rgw_put_b[30s])\n) * on (instance_id) group_left (ceph_daemon) ceph_rgw_metadata, "rgw_host", "$1", "ceph_daemon", "rgw.(.*)")',


Same here, with verbatim magic (since we are refactoring this, let's take the chance to clean this up a bit).

epuertat · 2022-02-02T12:14:06Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

@@ -529,7 +529,7 @@ local addStyle(alias, colorMode, colors, dateFormat, decimals, mappingType, patt
        'Latencies are shown stacked, without a yaxis to provide a visual indication of PUT latency imbalance across RGW hosts',
        's',
        'short',
-        'label_replace(rate(ceph_rgw_put_initial_lat_sum[30s]),"rgw_host","$1","ceph_daemon","rgw.(.*)") / \nlabel_replace(rate(ceph_rgw_put_initial_lat_count[30s]),"rgw_host","$1","ceph_daemon","rgw.(.*)")',
+        'label_replace(\n    rate(ceph_rgw_put_initial_lat_sum[30s]) /\n    rate(ceph_rgw_put_initial_lat_count[30s]) *\n    on (instance_id) group_left (ceph_daemon) ceph_rgw_metadata,\n"rgw_host", "$1", "ceph_daemon", "rgw.(.*)")',


And here ;-)

epuertat · 2022-02-02T12:16:47Z

monitoring/grafana/dashboards/tests/features/radosgw-detail.feature

+  And variable `rgw_servers` is `rgw.foo`
+  Then Grafana panel `$rgw_servers GET/PUT Latencies` with legend `GET {{ceph_daemon}}` shows:
+    | metrics | values |
+    | {ceph_daemon="rgw.foo", instance_id="58892247"} | 2.5000000000000004 |


oh my IEEE 754 🙈

epuertat · 2022-02-02T12:17:24Z

monitoring/grafana/dashboards/tests/features/radosgw-detail.feature

+Scenario: "Test $rgw_servers GET/PUT Latencies - GET"
+  Given the following series:
+    | metrics | values |
+    | ceph_rgw_get_initial_lat_sum{instance="127.0.0.1", instance_id="58892247", job="ceph"} | 10 50 100 |
+    | ceph_rgw_get_initial_lat_count{instance="127.0.0.1", instance_id="58892247", job="ceph"} | 20 60 80 |
+    | ceph_rgw_metadata{ceph_daemon="rgw.foo", hostname="localhost", instance="127.0.0.1", instance_id="58892247", job="ceph"} | 1 1 1 |
+  When interval is `30s`
+  And variable `rgw_servers` is `rgw.foo`
+  Then Grafana panel `$rgw_servers GET/PUT Latencies` with legend `GET {{ceph_daemon}}` shows:
+    | metrics | values |
+    | {ceph_daemon="rgw.foo", instance_id="58892247"} | 2.5000000000000004 |


Awesome work!

MrFreezeex · 2022-02-02T17:16:14Z

@MrFreezeex are you ok with having this one merged first? I hope this doesn't mean much rebase for you...

Yes sure you can merge this before. It's always a pain to rebase but I did it already a couple of times so I can do it once more, no worries.

epuertat · 2022-02-02T17:39:38Z

Thank you @MrFreezeex ! Will merge this then.

BenoitKnecht · 2022-02-03T10:38:15Z

Thanks @epuertat for the review and subsequent merge. I didn't know the ||| syntax in jsonnet, I'll keep it in mind for the future.

And thanks @MrFreezeex for accepting the burden of having to rebase your own PR. :)

Could this be flagged for backport to Pacific? It would be great to have those metric labels be consistent again.

epuertat · 2022-02-03T12:50:16Z

@BenoitKnecht the pacific backport tracker is already there: https://tracker.ceph.com/issues/54118 (you just need to run the scr/script/ceph-backport.sh 54118). I'll take care of backporting this to quincy in batch PR.

BenoitKnecht · 2022-02-03T15:18:34Z

Ah that's great, thanks! I though some specific label was needed on this PR for a bot to do the backport, but the script worked just fine.

github-actions bot added core mgr labels Oct 28, 2021

BenoitKnecht changed the title ~~mgr/ActivePyModules: Use metadata id in dump_server()~~ Draft: mgr/ActivePyModules: Use metadata id in dump_server() Oct 28, 2021

BenoitKnecht marked this pull request as draft October 28, 2021 17:30

github-actions bot added monitoring pybind labels Nov 1, 2021

BenoitKnecht force-pushed the ceph-mgr-service-id branch from 507b8e0 to 2ab7463 Compare November 1, 2021 10:32

alfonsomthd requested review from epuertat and tchaikov November 9, 2021 12:20

p-se self-requested a review November 11, 2021 10:32

BenoitKnecht force-pushed the ceph-mgr-service-id branch 2 times, most recently from d3b7c95 to 6a2087c Compare November 12, 2021 14:22

BenoitKnecht changed the title ~~Draft: mgr/ActivePyModules: Use metadata id in dump_server()~~ mgr: Fix ceph_daemon label in ceph_rgw_* metrics Nov 12, 2021

BenoitKnecht marked this pull request as ready for review November 12, 2021 14:31

pereman2 reviewed Dec 2, 2021

View reviewed changes

BenoitKnecht force-pushed the ceph-mgr-service-id branch from 6a2087c to ebae43d Compare January 3, 2022 15:22

BenoitKnecht requested a review from a team as a code owner January 3, 2022 15:22

github-actions bot added the dashboard label Jan 3, 2022

BenoitKnecht force-pushed the ceph-mgr-service-id branch from ebae43d to 9a51a5f Compare January 4, 2022 23:15

BenoitKnecht force-pushed the ceph-mgr-service-id branch from 9a51a5f to b9b3098 Compare January 5, 2022 18:15

alfonsomthd requested review from aaSharma14 and pcuzner January 7, 2022 09:35

github-actions bot added the needs-rebase label Jan 11, 2022

BenoitKnecht added 2 commits January 11, 2022 12:48

pybind/mgr/mgr_module.py: Set instance_id label for rgw

9f6573b

Now that the RadosGW returns its instance ID instead of its daemon name, replace the `ceph_daemon` label with an `instance_id` label on the `rgw` metrics. Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

BenoitKnecht added 3 commits January 11, 2022 12:48

monitoring/grafana: Add tests for radosgw panels

2daaa05

Some of the expressions modified in c402903 were not covered by any tests, especially those in the `radosgw-detail.json` dashboard. This commit fills in those gaps. Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>

BenoitKnecht force-pushed the ceph-mgr-service-id branch from b9b3098 to 2daaa05 Compare January 11, 2022 12:21

github-actions bot removed the needs-rebase label Jan 11, 2022

pereman2 reviewed Jan 18, 2022

View reviewed changes

pereman2 approved these changes Jan 20, 2022

View reviewed changes

pereman2 requested a review from avanthakkar January 20, 2022 08:17

aaSharma14 approved these changes Feb 2, 2022

View reviewed changes

pereman2 added the needs-quincy-backport backport required for quincy label Feb 2, 2022

epuertat approved these changes Feb 2, 2022

View reviewed changes

epuertat merged commit c47ace9 into ceph:master Feb 2, 2022

BenoitKnecht mentioned this pull request Feb 3, 2022

pacific: mgr: Fix ceph_daemon label in ceph_rgw_* metrics #44885

Merged

epuertat mentioned this pull request Feb 4, 2022

quincy: dashboard: 2nd backport batch #44899

Merged

epuertat removed the needs-quincy-backport backport required for quincy label Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr: Fix ceph_daemon label in ceph_rgw_* metrics #43707

mgr: Fix ceph_daemon label in ceph_rgw_* metrics #43707

BenoitKnecht commented Oct 28, 2021 •

edited

Loading

BenoitKnecht commented Oct 28, 2021

pereman2 left a comment •

edited

Loading

BenoitKnecht commented Dec 3, 2021

BenoitKnecht commented Jan 5, 2022

github-actions bot commented Jan 11, 2022

BenoitKnecht commented Jan 11, 2022

pereman2 left a comment

BenoitKnecht commented Jan 18, 2022

pereman2 left a comment •

edited

Loading

BenoitKnecht commented Feb 1, 2022

pereman2 commented Feb 1, 2022

aaSharma14 left a comment

aaSharma14 commented Feb 2, 2022

epuertat left a comment

epuertat Feb 2, 2022

epuertat Feb 2, 2022

epuertat Feb 2, 2022

epuertat Feb 2, 2022

epuertat Feb 2, 2022

MrFreezeex commented Feb 2, 2022

epuertat commented Feb 2, 2022

BenoitKnecht commented Feb 3, 2022

epuertat commented Feb 3, 2022

BenoitKnecht commented Feb 3, 2022

mgr: Fix ceph_daemon label in ceph_rgw_* metrics #43707

mgr: Fix ceph_daemon label in ceph_rgw_* metrics #43707

Conversation

BenoitKnecht commented Oct 28, 2021 • edited Loading

Checklist

BenoitKnecht commented Oct 28, 2021

pereman2 left a comment • edited Loading

Choose a reason for hiding this comment

BenoitKnecht commented Dec 3, 2021

BenoitKnecht commented Jan 5, 2022

github-actions bot commented Jan 11, 2022

BenoitKnecht commented Jan 11, 2022

pereman2 left a comment

Choose a reason for hiding this comment

BenoitKnecht commented Jan 18, 2022

pereman2 left a comment • edited Loading

Choose a reason for hiding this comment

BenoitKnecht commented Feb 1, 2022

pereman2 commented Feb 1, 2022

aaSharma14 left a comment

Choose a reason for hiding this comment

aaSharma14 commented Feb 2, 2022

epuertat left a comment

Choose a reason for hiding this comment

epuertat Feb 2, 2022

Choose a reason for hiding this comment

epuertat Feb 2, 2022

Choose a reason for hiding this comment

epuertat Feb 2, 2022

Choose a reason for hiding this comment

epuertat Feb 2, 2022

Choose a reason for hiding this comment

epuertat Feb 2, 2022

Choose a reason for hiding this comment

MrFreezeex commented Feb 2, 2022

epuertat commented Feb 2, 2022

BenoitKnecht commented Feb 3, 2022

epuertat commented Feb 3, 2022

BenoitKnecht commented Feb 3, 2022

BenoitKnecht commented Oct 28, 2021 •

edited

Loading

pereman2 left a comment •

edited

Loading

pereman2 left a comment •

edited

Loading