Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add rule for legend #126

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add rule for legend #126

wants to merge 1 commit into from

Conversation

zeitlinger
Copy link
Member

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 zeitlinger requested a review from rgeyer May 9, 2023 12:57
@zeitlinger zeitlinger self-assigned this May 9, 2023
@rgeyer
Copy link
Collaborator

rgeyer commented May 9, 2023

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.

@v-zhuravlev
Copy link

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.

@zeitlinger
Copy link
Member Author

zeitlinger commented May 10, 2023

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 zeitlinger removed their assignment Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants