monitoring/grafana: Replace missing legendFormat warning with error. #44332

pereman2 · 2021-12-16T14:14:31Z

In #43669 we introduced a warning to detect duplicate title, legendFormat keys. In this PR this warning message it's changed to an error so that we won't allow duplicate keys to happen in order to improve the reliability of grafana tests.

~~The first commit will be deleted after #44190 is merged.~~

Fixes: https://tracker.ceph.com/issues/53754
Signed-off-by: Pere Diaz Bou pdiazbou@redhat.com

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

monitoring/grafana/dashboards/tests/util.py

pereman2 · 2021-12-16T14:22:48Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

@@ -344,7 +344,7 @@ local addStyle(alias, colorMode, colors, dateFormat, decimals, mappingType, patt
    local RbdDetailsPanel(title, formatY1, expr1, expr2, x, y, w, h) =
      graphPanelSchema({}, title, '', 'null as zero', false, formatY1, formatY1, null, null, 0, 1, '$Datasource')
      .addTargets(
-        [addTargetSchema(expr1, 1, 'time_series', 'Write'),addTargetSchema(expr2, 1, 'time_series', 'Read')]) + {gridPos: {x: x, y: y, w: w, h: h}};
+        [addTargetSchema(expr1, 1, 'time_series', '{{pool}} Write'),addTargetSchema(expr2, 1, 'time_series', '{{pool}} Read')]) + {gridPos: {x: x, y: y, w: w, h: h}};


I added this because it conflicts with rbd-overview.json. If I understood it correctly, rbd-details provides metrics for each rbd pool so I thought that {{pool}} would be more specific and therefore make this work? @aaSharma14 you know more about this, what do you think.

@aaSharma14 ping

pereman2 · 2021-12-16T14:23:26Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

+        [addTargetSchema('(ceph_pool_compress_under_bytes / ceph_pool_compress_bytes_used > 0) and on(pool_id) (((ceph_pool_compress_under_bytes > 0) / ceph_pool_stored_raw) * 100 > 0.5)', 1, 'table', 'A'),
+        addTargetSchema('ceph_pool_max_avail * on(pool_id) group_left(name) ceph_pool_metadata', 1, 'table', 'B'),
+        addTargetSchema('((ceph_pool_compress_under_bytes > 0) / ceph_pool_stored_raw) * 100', 1, 'table', 'C'),
+        addTargetSchema('(ceph_pool_percent_used * on(pool_id) group_left(name) ceph_pool_metadata)', 1, 'table', 'D'),
+        addTargetSchema('(ceph_pool_compress_under_bytes - ceph_pool_compress_bytes_used > 0)', 1, 'table', 'E'),


Here since tables don't care about the legend format I added these legendFormat arbitrarily.

Maybe in a follow-up PR we should add a meaningful name for legendFormat in all tables: AFAIK this is displayed when you explore the table details.

pereman2 · 2021-12-16T14:23:57Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

@@ -634,8 +634,7 @@ local addStyle(alias, colorMode, colors, dateFormat, decimals, mappingType, patt
        {"Non-Encrypted": "#E5AC0E"}, '', 'OSD Objectstore Types'
      )
      .addTarget(addTargetSchema('count(ceph_bluefs_wal_total_bytes)', 1, 'time_series', 'bluestore'))
-      .addTarget(addTargetSchema('count(ceph_osd_metadata) - count(ceph_bluefs_wal_total_bytes)', 1, 'time_series', 'filestore'))
-      .addTarget(addTargetSchema('absent(ceph_bluefs_wal_total_bytes)*count(ceph_osd_metadata)', 1, 'time_series', 'filestore')) + {gridPos: {x: 4, y: 8, w: 4, h: 8}},
+      .addTarget(addTargetSchema('count(ceph_osd_metadata) - count(ceph_bluefs_wal_total_bytes)', 1, 'time_series', 'filestore')) + {gridPos: {x: 4, y: 8, w: 4, h: 8}},


There were 2 filestore counts? I removed one that wasn't correct.

Are you sure? I have no clue about what this was for... :S Maybe @pcuzner recalls...

@pcuzner ping

@epuertat

I deleted the wrong one :PPPPPP . In ceph-dev with 5 filestore osds you can see the output of both of the queries.

fixed in last commit

github-actions · 2021-12-21T17:34:56Z

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

epuertat · 2021-12-23T20:26:57Z

monitoring/grafana/dashboards/tests/__init__.py

@@ -171,7 +171,8 @@ def run_promtool(self):

        for variable, value in self.variables.items():
            expr = self.promql_expr_test.expr
-            new_expr = re.sub(r'\${0}'.format(variable), str(value), expr)
+            regex = fr'\${variable}(?=\W)'


Why the positive look-ahead? What exactly are you trying to match/not to match here? You could express that as a doctest (you'd need to extract part of this function and isolate the string processing code).

The positive lookahead is because I don't want to replace that part, but I need it to make a match.

@epuertat ping

monitoring/grafana/dashboards/tests/__init__.py

monitoring/grafana/dashboards/tests/util.py

epuertat · 2021-12-23T20:40:56Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

@@ -634,8 +634,7 @@ local addStyle(alias, colorMode, colors, dateFormat, decimals, mappingType, patt
        {"Non-Encrypted": "#E5AC0E"}, '', 'OSD Objectstore Types'
      )
      .addTarget(addTargetSchema('count(ceph_bluefs_wal_total_bytes)', 1, 'time_series', 'bluestore'))
-      .addTarget(addTargetSchema('count(ceph_osd_metadata) - count(ceph_bluefs_wal_total_bytes)', 1, 'time_series', 'filestore'))
-      .addTarget(addTargetSchema('absent(ceph_bluefs_wal_total_bytes)*count(ceph_osd_metadata)', 1, 'time_series', 'filestore')) + {gridPos: {x: 4, y: 8, w: 4, h: 8}},
+      .addTarget(addTargetSchema('count(ceph_osd_metadata) - count(ceph_bluefs_wal_total_bytes)', 1, 'time_series', 'filestore')) + {gridPos: {x: 4, y: 8, w: 4, h: 8}},


Are you sure? I have no clue about what this was for... :S Maybe @pcuzner recalls...

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

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

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

alfonsomthd · 2022-01-19T11:34:19Z

jenkins test dashboard cephadm

alfonsomthd · 2022-01-19T11:40:11Z

monitoring/grafana/dashboards/jsonnet/grafana_dashboards.jsonnet

+        [addTargetSchema('(ceph_pool_compress_under_bytes / ceph_pool_compress_bytes_used > 0) and on(pool_id) (((ceph_pool_compress_under_bytes > 0) / ceph_pool_stored_raw) * 100 > 0.5)', 1, 'table', 'A'),
+        addTargetSchema('ceph_pool_max_avail * on(pool_id) group_left(name) ceph_pool_metadata', 1, 'table', 'B'),
+        addTargetSchema('((ceph_pool_compress_under_bytes > 0) / ceph_pool_stored_raw) * 100', 1, 'table', 'C'),
+        addTargetSchema('(ceph_pool_percent_used * on(pool_id) group_left(name) ceph_pool_metadata)', 1, 'table', 'D'),
+        addTargetSchema('(ceph_pool_compress_under_bytes - ceph_pool_compress_bytes_used > 0)', 1, 'table', 'E'),


Maybe in a follow-up PR we should add a meaningful name for legendFormat in all tables: AFAIK this is displayed when you explore the table details.

epuertat

LGTM!

epuertat · 2022-01-19T12:18:19Z

cephadm failure but unrelated.

epuertat · 2022-01-19T12:18:47Z

@pereman2 shouldn't this be a quincy backport?

pereman2 added dashboard monitoring labels Dec 16, 2021

pereman2 requested a review from a team as a code owner December 16, 2021 14:14

pereman2 added this to In progress in Dashboard via automation Dec 16, 2021

pereman2 requested review from avanthakkar, aaSharma14 and alfonsomthd and removed request for a team December 16, 2021 14:14

alfonsomthd reviewed Dec 16, 2021

View reviewed changes

monitoring/grafana/dashboards/tests/util.py Outdated Show resolved Hide resolved

pereman2 commented Dec 16, 2021

View reviewed changes

github-actions bot added the needs-rebase label Dec 21, 2021

epuertat reviewed Dec 23, 2021

View reviewed changes

pereman2 force-pushed the grafana-warnings branch from 7e9a1cf to 2298084 Compare January 3, 2022 08:45

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

pereman2 changed the title ~~[AFTER #44190 ] monitoring/grafana: Replace missing legendFormat warning with error.~~ monitoring/grafana: Replace missing legendFormat warning with error. Jan 3, 2022

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

pereman2 force-pushed the grafana-warnings branch from ef3089d to 64b0a9f Compare January 17, 2022 15:06

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

pereman2 added 6 commits January 18, 2022 13:24

monitoring/grafana: Replace missing legendFormat warning with error

4c30223

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

monitoring/grafana: ValueError instead of RuntimeError

b381a83

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

monitoring/grafana: add generated json files

2b4f356

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

monitoring/grafana: remove explicit str casting

1e4d85d

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

monitoring/grafana: use Path class instead of split

a3cf5c5

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

monitoring/grafana: replace filestore osd count

57c2631

Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>

pereman2 force-pushed the grafana-warnings branch from 64b0a9f to 57c2631 Compare January 18, 2022 13:22

Dashboard automation moved this from In progress to Reviewer approved Jan 19, 2022

alfonsomthd approved these changes Jan 19, 2022

View reviewed changes

epuertat approved these changes Jan 19, 2022

View reviewed changes

epuertat merged commit 3e6afc3 into ceph:master Jan 19, 2022

Dashboard automation moved this from Reviewer approved to Done Jan 19, 2022

epuertat deleted the grafana-warnings branch January 19, 2022 12:18

epuertat added the needs-quincy-backport backport required for quincy label Jan 19, 2022

epuertat mentioned this pull request Jan 28, 2022

quincy: dashboard: first batch of dashboard backports #44824

Merged

14 tasks

epuertat removed the needs-quincy-backport backport required for quincy label Jan 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monitoring/grafana: Replace missing legendFormat warning with error. #44332

monitoring/grafana: Replace missing legendFormat warning with error. #44332

pereman2 commented Dec 16, 2021 •

edited

pereman2 Dec 16, 2021

pereman2 Jan 17, 2022

pereman2 Dec 16, 2021

alfonsomthd Jan 19, 2022 •

edited

pereman2 Dec 16, 2021

epuertat Dec 23, 2021

pereman2 Jan 17, 2022

pereman2 Jan 18, 2022

pereman2 Jan 19, 2022

github-actions bot commented Dec 21, 2021

epuertat Dec 23, 2021

pereman2 Jan 3, 2022

pereman2 Jan 17, 2022

epuertat Dec 23, 2021

github-actions bot commented Jan 11, 2022

alfonsomthd commented Jan 19, 2022

alfonsomthd Jan 19, 2022 •

edited

epuertat left a comment

epuertat commented Jan 19, 2022

epuertat commented Jan 19, 2022

monitoring/grafana: Replace missing legendFormat warning with error. #44332

monitoring/grafana: Replace missing legendFormat warning with error. #44332

Conversation

pereman2 commented Dec 16, 2021 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsomthd Jan 19, 2022 • edited

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

github-actions bot commented Dec 21, 2021

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

github-actions bot commented Jan 11, 2022

alfonsomthd commented Jan 19, 2022

alfonsomthd Jan 19, 2022 • edited

Choose a reason for hiding this comment

epuertat left a comment

Choose a reason for hiding this comment

epuertat commented Jan 19, 2022

epuertat commented Jan 19, 2022

pereman2 commented Dec 16, 2021 •

edited

alfonsomthd Jan 19, 2022 •

edited

alfonsomthd Jan 19, 2022 •

edited