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

fix: cluster label replace in loki-operational.libsonnet #12789

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

QuentinBisson
Copy link
Contributor

This PR fixes the loki-operational.libsonnet mixin in case the value of per_cluster_label starts with cluster_.

For historical reasons, we named the cluster label cluster_id so generation actually replaces all cluster_id with cluster_id_id in this dashboard because the replace is incorrect

What this PR does / why we need it:

This PR fixes the loki operational dashboard when the cluster label starts with cluster_

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@QuentinBisson QuentinBisson requested a review from a team as a code owner April 25, 2024 13:05
@QuentinBisson
Copy link
Contributor Author

@cstyan I would really appreciate the review :)

@QuentinBisson QuentinBisson changed the title fix loki-operational.libsonnet fix: loki-operational.libsonnet Apr 25, 2024
@cstyan
Copy link
Contributor

cstyan commented Apr 26, 2024

@QuentinBisson sorry, I'm failing to visualize in my head what the new value of the label replace would end up inserting as the label, can you help me out here with a more concrete example of what happens currently vs what we should be doing?

@QuentinBisson
Copy link
Contributor Author

Sure, so the issue is that if we set the per_cluster_label now to cluster_id then all occurrences of cluster=$cluster are perfectly replaced to cluster_id=$cluster but when the englobing replace them again then this résults in cluster_id_id=$cluster. Limiting thé englobing to cluster_job only renames thé recording rules to cluster_id_job which is what this replace was for

@cstyan
Copy link
Contributor

cstyan commented Apr 26, 2024

Sure, so the issue is that if we set the per_cluster_label now to cluster_id then all occurrences of cluster=$cluster are perfectly replaced to cluster_id=$cluster but when the englobing replace them again then this résults in cluster_id_id=$cluster.

Okay this makes sense, thanks 👍

Limiting thé englobing to cluster_job only renames thé recording rules to cluster_id_job which is what this replace was for

can this not cause issues in other places though where we have just cluster? I supposed the earlier replace directives handle those cases?

@QuentinBisson
Copy link
Contributor Author

The previous one should be enough but i'll run it an test. From what I understood, there are 4 use cases:

  • cluster=$cluster
  • cluster=~$cluster
  • cluster in templating or by clauses which i'm or sure about
  • cluster_job recording rules.
    Let me generate this on monday to assert all cluster labels are fixed. This dashboard importing raw json is not the easiest

This PR fixes the  loki-operational.libsonnet mixin in case the value of per_cluster_label starts with cluster_.

For historical reasons, we named the cluster label cluster_id so generation actually replaces all cluster_id with cluster_id_id in this dashboard because the replace is incorrect
@QuentinBisson
Copy link
Contributor Author

QuentinBisson commented Apr 29, 2024

@cstyan I checked and there is no other use cases for cluster apart from those 3 replace. The one used in the templating section is managed via the addCluster libsonnet function. We could keep the replace of cluster_ only and move it as the first replace but that would not really change much in the end

Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for digging in and providing an explanation for your changes 👍

@cstyan cstyan merged commit 51a841f into grafana:main Apr 29, 2024
59 checks passed
@cstyan cstyan changed the title fix: loki-operational.libsonnet fix: cluster label replace in loki-operational.libsonnet Apr 29, 2024
shantanualsi pushed a commit that referenced this pull request May 6, 2024
@QuentinBisson QuentinBisson deleted the patch-1 branch September 19, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants