-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: add missing cluster label to mixins #12870
feat: add missing cluster label to mixins #12870
Conversation
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
a866c00
to
07ac356
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, some of these won't work as you'd like them to because the queries in each alerts expr
are not keeping the cluster label.
Oh you're right :( |
It's worth looking into, happy to review a PR if you come up with something that works 👍 |
Here is the output of pint without colors :( I think it would be best to add it to the loki-build-image but I'm not sure if you like to have contribution on that repo.
|
@@ -52,7 +52,7 @@ groups: | |||
message: | | |||
{{`{{`}} $labels.cluster {{`}}`}} {{`{{`}} $labels.namespace {{`}}`}} has had {{`{{`}} printf "%.0f" $value {{`}}`}} compactors running for more than 5m. Only one compactor should run at a time. | |||
expr: | | |||
sum(loki_boltdb_shipper_compactor_running) by (namespace, cluster) > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure they are all in the same order
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
@cstyan it should be fine now, pint is happy apart from the humanize thing |
Agreed, it would be best to have it nicely runnable in a reproduce-able way. For now, can you show me how the invocation of pint works and I can setup a CI job via github actions at least post-merging of your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; same as usual, I assume you've tried deploying the jsonnet version already to your own environment?
pint is happy apart from the humanize thing
not sure what you mean here?
I meant this section:
But I did not really play with humanize functions :) So I run pint from the cli for this one but I think you could use the pint ci directly: There are a lot of things that could be added (like enforcing that some annotations or labels are always set) but an initial version would be good. I will deploy it tomorrow or later today, I did not have time to full test my changes :( i'll ping you when i'm done and thank you for the review |
@vlad-diachenko any idea why the helm diff action is failing here? I believe that's a new one you wrote? |
From what I see, it's using the wrong remote as the log line talks about
|
yep, this seems like the issue, Vlad is looking into it on our end |
Awesome thanks :) |
Hey folks. I hope this PR fixes the issue #14173 |
Hey @QuentinBisson , @cstyan . I have fixed the checkout step that was failing at the beginning, but now it started to fail another step I will check it tomorrow morning and will fix it and merge the PR ;) |
Thanks for the work @vlad-diachenko :) |
Vlad mentioned to me that he couldn't get everything working correctly for PRs from forks, so he changed the workflow to not run on PRs from forks for now. |
Signed-off-by: QuentinBisson <quentin@giantswarm.io> Co-authored-by: Vladyslav Diachenko <82767850+vlad-diachenko@users.noreply.github.com> Co-authored-by: Callum Styan <callumstyan@gmail.com>
What this PR does / why we need it:
This PR enhance the loki alert mixins by adding the cluster label to all summary.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR