feat: add missing cluster label to mixins - #12870
Conversation
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
a866c00 to
07ac356
Compare
cstyan
left a comment
cstyan
left a comment
There was a problem hiding this comment.
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. |
| 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.
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. |
cstyan
left a comment
cstyan
left a comment
There was a problem hiding this comment.
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>
3 participants
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.mdguide (required)featPRs 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.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR