dashboard-linter icon indicating copy to clipboard operation
dashboard-linter copied to clipboard

add rule for legend

Open zeitlinger opened this issue 1 year ago • 3 comments

It's a good practice to prefix the legend with the instance ID, so that dashboards can show all instances, or only a selected one

zeitlinger avatar May 08 '23 13:05 zeitlinger

Thank you for this @zeitlinger.

However, I don't think that this is a hard and fast rule, but more of a guideline. There are absolutely cases where we would want to aggregate all returned series with an average, or sum. Particularly on a stat panel or pie.

There are other cases where we would prefer to aggregate on a different label, such as cluster or a SUO specific label.

If we were to include this as a linting rule, I would advise that we have it emit a warning rather than an error to prevent breaking changes.

As an example, running this on the entire grafana/cloud-onboarding repo, this increases the number of integrations/mixins with failures from 13 to 76.

The 13 which were failing before this change are because of similar changes which broke backward compatibility and/or introduced a rule as an error when it was really a guideline, or simply wasn't thought out thoroughly.

A rule like target-counter-agg, which checks to see that an aggregation function is used, or the legend includes any label would be more applicable, without requiring a specific aggregation, or a specific label.

rgeyer avatar May 09 '23 19:05 rgeyer

Yes, I agree with @rgeyer , as there are cases when metrics are aggregated by job or cluster, or something else, effectively dropping instance label. Therefore it would be real challenge to enforce such rule. As a general guideline, it would be nice to have this reminder for legend.

v-zhuravlev avatar May 10 '23 07:05 v-zhuravlev

Great feedback. Let's see if we can come up with a better rule:

If global aggregation: skip rule else if aggregation x (e.g. job, cluster) is used: legend should start with {{x}} - else: legend should start with {{instance}} -.

zeitlinger avatar May 10 '23 13:05 zeitlinger

Hello @zeitlinger.

I'm doing some much-overdue maintenance and housekeeping here. Do you still have cycles to look at this?

Or perhaps it's best we close it?

rgeyer avatar Sep 19 '24 21:09 rgeyer

let's close it - the pr is probably out of date

zeitlinger avatar Sep 23 '24 10:09 zeitlinger