Convert mimir-mixin from deprecated graph panel type to timeseries #7413
Conversation
narqo
force-pushed
the
7188-graph-panel
branch
from
16c6c28
to
952b4e9
Compare
| @@ -167,6 +175,21 @@ local utils = import 'mixin-utils/utils.libsonnet'; | |||
| recordingRulePrefix(selectors):: | |||
| std.join('_', [matcher.label for matcher in selectors]), | |||
|
|
|||
| // Overrite upstream's "stack" field to make it compatible with timeseriesPanel. | |||
There was a problem hiding this comment.
Can you expand this comment a bit. It took me a little while to figure out why this was required since we're using $.timeseriesPanel() in a few places below. I.e. explain that we're using some utility methods for panels from grafana-builder/grafana.libsonnet that can't be converted without coordinating updates to a bunch of different mixins in jsonnet-libs.
There was a problem hiding this comment.
🆙
I've rephrased the comment. Could you have another look, if it's less obscure now
There was a problem hiding this comment.
Looks good. In the future, please don't rebase / force-push after review. It makes it harder to figure out which changes are new since last time.
| $.queryPanel($.resourceUtilizationQuery('memory_working', $._config.instance_names.read, $._config.container_names.read), '{{%s}}' % $._config.per_instance_label) + | ||
| { yaxes: $.yaxes('bytes') }, | ||
| { fieldConfig+: { defaults+: { unit: 'bytes' } } }, |
There was a problem hiding this comment.
nit: Overrides of the unit seems to have come up a few times, worth making a helper method in dashboard-utils.libsonnet?
There was a problem hiding this comment.
I was thinking if we can skip that for this PR. Maybe later we could refactor the jsonnet-lib's timeseriesPanel() mixin, to allow passing the "unit" argument:
$.timeseriesPanel(title, unit="short")For me, this fits better in the panels' JSON model
| $.queryPanel( | ||
| 'sum(rate(cortex_distributor_samples_in_total{%(job)s, user="$user"}[$__rate_interval]))' | ||
| % { job: $.jobMatcher($._config.job_names.distributor) }, | ||
| 'rate', | ||
| ) + | ||
| { legend: { show: false } } + | ||
| { options+: { legend+: { showLegend: false } } } + |
There was a problem hiding this comment.
nit: Utility method to hide the legend?
Signed-off-by: Vladimir Varankin <vladimir.varankin@grafana.com>
narqo
force-pushed
the
7188-graph-panel
branch
from
952b4e9
to
8f0be58
Compare
Signed-off-by: Vladimir Varankin <vladimir.varankin@grafana.com>
Signed-off-by: Vladimir Varankin <vladimir.varankin@grafana.com>
narqo
force-pushed
the
7188-graph-panel
branch
from
8f0be58
to
0d2a7b4
Compare
make build-mixin build-helm-tests Signed-off-by: Vladimir Varankin <vladimir.varankin@grafana.com>
narqo
force-pushed
the
7188-graph-panel
branch
from
0d2a7b4
to
5e1073c
Compare
|
this deserves a changelog entry, it's a nice fix. @narqo, can you add one in the root-level CHANGELOG.md under the Mixin section? |
What this PR does
Note, it turned out it's simpler to convert all dashboards at once, than trying to do that dashboard-by-dashboard, because of how jsonnet mixins are structured.
To make review simpler, I suggest looking through the individual commits in
mimir-mixin/, where I divided the changes into parts: "update the utils" and "update the dashboards". The majority of other changes are autogenerated from make.Overall, after eyeballing all dashboards for a day, I'm fairly confident that everything there is fine 🤞
Below, attaching random before & after screenshots. Note, I tested the dashboards in Grafana 10.0, which doesn't automatically convert graph panels to timeseries (i.e. what one sees on the screenshots below is what one gets).
Mimir / Overview
Before
After
Mimir / Writes
Before
After
Mimir / Reads
Before
After
Which issue(s) this PR fixes or relates to
Fixes #7188
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.