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: incorrect compactor matcher in loki-deletion dashboard mixin #12567

Merged
merged 9 commits into from
Apr 17, 2024

Conversation

QuentinBisson
Copy link
Contributor

@QuentinBisson QuentinBisson commented Apr 11, 2024

What this PR does / why we need it:

This PR fixes the loki-deletion dashboard compactor matcher for the SSD mixins

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
Copy link
Contributor Author

@MichelHollands could you take a look?

@QuentinBisson QuentinBisson changed the title Fix loki-deletion dashboard fix: incorrect compactor matcher in loki-deletion dashboard mixin Apr 11, 2024
@cstyan
Copy link
Contributor

cstyan commented Apr 16, 2024

@QuentinBisson it's not clear to me why this is broken in the first place? job=namespace/loki-read should work for ssd

@QuentinBisson
Copy link
Contributor Author

@cstyan I think you're right about the job but the container=compactor will definitely not work in ssd mode https://github.com/grafana/loki/pull/12567/files#diff-2e671957ef0b084f5abd5911a3ca6b92bc59b07dc8412477b13b2abac9fe1342L69

I took the matcher from

local compactor_pod_matcher = if $._config.ssd.enabled then 'container="loki", pod=~"%s-read.*"' % $._config.ssd.pod_prefix_matcher else 'container="compactor"',
but
local compactor_job_matcher = if $._config.ssd.enabled then '%s-read' % $._config.ssd.pod_prefix_matcher else 'compactor',
could also work

@cstyan
Copy link
Contributor

cstyan commented Apr 16, 2024

@cstyan I think you're right about the job but the container=compactor will definitely not work in ssd mode https://github.com/grafana/loki/pull/12567/files#diff-2e671957ef0b084f5abd5911a3ca6b92bc59b07dc8412477b13b2abac9fe1342L69

I took the matcher from

local compactor_pod_matcher = if $._config.ssd.enabled then 'container="loki", pod=~"%s-read.*"' % $._config.ssd.pod_prefix_matcher else 'container="compactor"',

but

local compactor_job_matcher = if $._config.ssd.enabled then '%s-read' % $._config.ssd.pod_prefix_matcher else 'compactor',

could also work

oh fun, I didn't notice at first that we had different label selectors in panels of the same dashboard, lets use the same matcher as the retention dashboard which you have now

@@ -61,15 +61,15 @@ local utils = import 'mixin-utils/utils.libsonnet';
)
.addPanel(
$.newQueryPanel('Lines Deleted / Sec') +
g.queryPanel('sum(rate(loki_compactor_deleted_lines{' + $._config.per_cluster_label + '=~"$cluster",job=~"$namespace/%s"}[$__rate_interval])) by (user)' % compactor_matcher, '{{user}}'),
g.queryPanel('sum(rate(loki_compactor_deleted_lines{' + $._config.per_cluster_label + '=~"$cluster",' + compactor_matcher + '}[$__rate_interval])) by (user)', '{{user}}'),
Copy link
Contributor

Choose a reason for hiding this comment

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

this now needs a namespace label selector since we're not selecting on job=~"$namespace/%s" anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cstyan this should be okay now :)

Signed-off-by: QuentinBisson <quentin@giantswarm.io>
@cstyan
Copy link
Contributor

cstyan commented Apr 17, 2024

@QuentinBisson thanks :)

@cstyan cstyan merged commit 006f88c into grafana:main Apr 17, 2024
12 checks passed
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