Add capability to disable specific alert rules Loki #11241

Daniel-Vaz · 2023-11-16T08:49:18Z

What this PR does / why we need it:

Currently using the Loki Chart we can only either enable\disable ALL alert rules.
For specific environments and use-cases sometimes not all alert Rules are useful to have enabled.

With this PR change, we can cleanly and through the Chart values disable specific Alerts.

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
values.yaml comments added

Extra Note:

This is my first time doing a PR for this project. I forked the main branch and implemented these changes. Did not change the Chart.yaml version neither the Changelog, but if needed (or any other missing action) just request me and I will do it.

Thank you in advance for all your awesome work.

CLAassistant · 2023-11-16T08:49:23Z

All committers have signed the CLA.

dannykopping

Hey @Daniel-Vaz

I appreciate what you're trying to do but I think we need a more generic solution. Can we take a step back and talk about the problem you're trying to solve? Why do you want to disable these specific alerts?

Daniel-Vaz · 2023-11-16T09:18:09Z

Hi @dannykopping , thank you for the feedback !

Can we take a step back and talk about the problem you're trying to solve? Why do you want to disable these specific alerts?

So currently I'm a consumer of this Loki Chart, and since the clusters where I have Loki deployed I also have the Prom Operator, I want to enable the Chart suggested Scrapes\Rules\Alerts.

The Problem I'm facing, is that in some of my clusters indeed we have some "issues" in the overall architecture that result on Loki reporting quite frequently some LokiRequestLatency.

We could just silence them.
Or even we could just disable all the Chart default alerts, and copy them on our side.

But I believe that giving the flexibility to the Chart consumer to pick and chose which alerts they want to have enabled or not would be a nice feature.

For example, on the Kube Prometheus Stack Chart they support this disabling specific alerts capability.

Do you have any other suggestion on how this capability could be implemented ?

github-actions · 2023-11-16T18:48:02Z

Trivy scan found the following vulnerabilities:

HIGH, Target: docker.io/grafana/loki:main-3cb2a6b (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
HIGH, Target: docker.io/grafana/loki:main-3cb2a6b (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0
\nTo see more details on these vulnerabilities, and how/where to fix them, please run docker build -t grafana/loki:main-3cb2a6b -f cmd/loki/Dockerfile .
trivy i grafana/loki:main-3cb2a6b on your branch. If these were not introduced by your PR, please considering fixing them in via a subsequent PR. Thanks!

dannykopping

Alright, that sounds like a good enough reason to me - and good to see some prior art where this is done the same way. Thanks for that

LGTM! I'll just request a PR from a colleague who knows Helm before we merge.

shantanualsi · 2023-11-17T08:09:03Z

@Daniel-Vaz have you tried testing when all alerts are set to false and the rules block under loki_alerts is empty?

Daniel-Vaz · 2023-11-17T08:21:18Z

LGTM! I'll just request a PR from a colleague who knows Helm before we merge.

Thank you very much @dannykopping !

@Daniel-Vaz have you tried testing when all alerts are set to false and the rules block under loki_alerts is empty?

@shantanualsi Indeed in that situation the object will fail to be applied because it as no rules under the groups section.
I assumed that it's not likely that people will disable all alerts one by one. In those situations they will just set the monitoring.rules.alerting value to false.

Either way I think that if we find a way to address this corner-case it would indeed increase the reliability of the Chart.
I'm just a bit unsure how could it be implemented :/ any ideas ?

shantanualsi · 2023-11-17T10:09:16Z

In those situations they will just set the monitoring.rules.alerting value to false.

in our case, you can technically have enabled: true and LokiCanaryLatency: true and the chart will still break. I'm still thinking if there's a better way to address this. The kube-prometheus-stack you referenced in one of the comments above has a python script under hack folder that creates yaml files.. do you know if it addresses this particular case?

I'm also thinking that rules section now has enabled which is a boolean and disabled which is a map of <rules>: true. This can be confusing to other users..

Daniel-Vaz · 2023-11-17T12:09:11Z

in our case, you can technically have enabled: true and LokiCanaryLatency: true and the chart will still break

The way I implemented it is not the case I believe. Since for the LokiCanaryLatency the If condition encapsulates the whole loki_canaries_alerts rules group section.

{{- if not (.Values.monitoring.rules.disabled.LokiCanaryLatency | default false) }}
  - name: "loki_canaries_alerts"
    rules:
      - alert: "LokiCanaryLatency"
        annotations:
          message: |
            {{`{{`}} $labels.job {{`}}`}} is experiencing {{`{{`}} printf "%.2f" $value {{`}}`}}s 99th percentile latency.
        expr: |
          histogram_quantile(0.99, sum(rate(loki_canary_response_latency_seconds_bucket[5m])) by (le, namespace, job)) > 5
        for: "15m"
        labels:
          severity: "warning"
{{- if .Values.monitoring.rules.additionalRuleLabels }}
{{ toYaml .Values.monitoring.rules.additionalRuleLabels | indent 10 }}
{{- end }}
{{- end }}

I did this because that rules group section had only that single alert. If more are added in he future, then that needs to be updated indeed.

The kube-prometheus-stack you referenced in one of the comments above has a python script under hack folder that creates yaml files.. do you know if it addresses this particular case?

I believe that hack script is used in order to have a sync from a external place where alerts are defined. They dont write their own alerts definitions inside the Kube-Prometheus-Stack Chart, they instead just use those script to sync the rules from a external place and format them accordingly.

The usage for those scripts is written here.

I'm also thinking that rules section now has enabled which is a boolean and disabled which is a map of : true. This can be confusing to other users.

I kind of agree with you on this one.
But at the same time the name of those values is quite self explanatory no ?

If you want to enable alerting then monitoring.rules.alerting: true
If you need to disable a specific rule from the whole group, then : monitoring.rules.disabled: {}

We could also move the disabled to be nested under alerting.
Or even under alerting instead of just supporting <true/false> values, we can change the structure, and there specify the list of rules names, to be enabled\disabled, for example:

monitoring:
  rules:
    alerting:
      LokiTooManyCompactorsRunning: true
      LokiRequestLatency: false
      
# By default all values should be true

shantanualsi · 2023-11-20T06:13:54Z

Sorry for the delay.

Since for the LokiCanaryLatency the If condition encapsulates the whole loki_canaries_alerts rules group section.

Apologies, I wasn't clear enough here.. in case if all rules under loki_alerts are disabled and rules under loki_canaries_alerts are enabled.

Can you please add some documentation on the values.yaml for the edge case mentioning to set monitoring.rules.alerting to false explicitly if all rules are disabled? Thanks for your patience! :)

Daniel-Vaz · 2023-11-20T14:49:59Z

@shantanualsi Thank you :D !

Updated the values.yaml comments, whenever you want feel free to have a look.

JStickler · 2023-11-20T23:32:43Z

Also, please note that when you update the Helm charts, you have to bump the version of the Helm Chart. This is mentioned in the PR checklist.

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

shantanualsi · 2023-11-22T09:39:44Z

@Daniel-Vaz can you also please increment the helm version and add a changelog as suggested by @JStickler

Daniel-Vaz · 2023-11-22T10:36:04Z

@JStickler @shantanualsi

Done, Thank you for the warning and sorry for missing that part previously.

dannykopping · 2023-12-18T10:50:38Z

@Daniel-Vaz there seem to be some CI failures (the helm linter and the helm docs checks, specifically).

Please address these, and then we can get this merged.

Daniel-Vaz · 2024-01-10T09:33:58Z

I updated the version reference to be 5.41.6.
I ran helm-docs without issues.
But based on the drone ci workflow error I tried to run the command make -BC docs sources/setup/install/helm/reference.md, but I'm getting the following error:

$ make -BC docs sources/setup/install/helm/reference.md
make: Entering directory '/home/user/repos/loki/docs'
podman run --rm --volume "/home/user/repos/loki:/helm-docs" -u "$(id -u)" "docker.io/jnorwood/helm-docs:v1.11.0" \
        -c /helm-docs/production/helm/ \
        -t reference.md.gotmpl \
        -o reference.md
mv "../production/helm/loki/reference.md" "sources/setup/install/helm/reference.md"
time="2024-01-10T09:33:13Z" level=info msg="Found Chart directories [loki]"
time="2024-01-10T09:33:13Z" level=info msg="Generating README Documentation for chart /helm-docs/production/helm/loki"
time="2024-01-10T09:33:13Z" level=warning msg="Could not open chart README file /helm-docs/production/helm/loki/README.md, skipping chart"
mv: cannot stat '../production/helm/loki/reference.md': No such file or directory
make: *** [Makefile:15: sources/setup/install/helm/reference.md] Error 1
make: Leaving directory '/home/user/repos/loki/docs'

dannykopping · 2024-01-15T07:48:13Z

I updated the version reference to be 5.41.6. I ran helm-docs without issues. But based on the drone ci workflow error I tried to run the command make -BC docs sources/setup/install/helm/reference.md, but I'm getting the following error:

$ make -BC docs sources/setup/install/helm/reference.md
make: Entering directory '/home/user/repos/loki/docs'
podman run --rm --volume "/home/user/repos/loki:/helm-docs" -u "$(id -u)" "docker.io/jnorwood/helm-docs:v1.11.0" \
        -c /helm-docs/production/helm/ \
        -t reference.md.gotmpl \
        -o reference.md
mv "../production/helm/loki/reference.md" "sources/setup/install/helm/reference.md"
time="2024-01-10T09:33:13Z" level=info msg="Found Chart directories [loki]"
time="2024-01-10T09:33:13Z" level=info msg="Generating README Documentation for chart /helm-docs/production/helm/loki"
time="2024-01-10T09:33:13Z" level=warning msg="Could not open chart README file /helm-docs/production/helm/loki/README.md, skipping chart"
mv: cannot stat '../production/helm/loki/reference.md': No such file or directory
make: *** [Makefile:15: sources/setup/install/helm/reference.md] Error 1
make: Leaving directory '/home/user/repos/loki/docs'

Hhmm, i just ran it and it worked fine; did you run this from the root of the project?

You can always copy the changes in manually from this commit:
377c7f8

Daniel-Vaz · 2024-01-16T10:32:10Z

@dannykopping
I thinks its ready to be merged now.
Thanks

dannykopping · 2024-01-16T11:11:02Z

Oh man what a nightmare, this CI is failing on whitespace now 💀 🙃
Thank you for your patience here @Daniel-Vaz! I think we're very nearly there.

Daniel-Vaz · 2024-01-17T07:56:33Z

@dannykopping
No worries 😅 I'm, the one who is sorry for this mess.

I see the workflow failing indeed on what it seems to be a removed line\whitespace....
But on my side running helm-docs and helm-lint dont result in any change:

$ helm-docs
INFO[2024-01-17T07:53:09Z] Found Chart directories [production/helm/loki]
INFO[2024-01-17T07:53:09Z] Generating README Documentation for chart production/helm/loki

$ make helm-lint
make -BC production/helm/loki lint
make[1]: Entering directory '/home/user/repos/loki/production/helm/loki'
yamllint -c /home/user/repos/loki/production/helm/loki/src/.yamllint.yaml /home/user/repos/loki/production/helm/loki/src
make[1]: Leaving directory '/home/user/repos/loki/production/helm/loki'

$ git status
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean

dannykopping · 2024-01-17T08:00:27Z

@dannykopping No worries 😅 I'm, the one who is sorry for this mess.

I see the workflow failing indeed on what it seems to be a removed line\whitespace.... But on my side running helm-docs and helm-lint dont result in any change:

$ helm-docs
INFO[2024-01-17T07:53:09Z] Found Chart directories [production/helm/loki]
INFO[2024-01-17T07:53:09Z] Generating README Documentation for chart production/helm/loki

$ make helm-lint
make -BC production/helm/loki lint
make[1]: Entering directory '/home/user/repos/loki/production/helm/loki'
yamllint -c /home/user/repos/loki/production/helm/loki/src/.yamllint.yaml /home/user/repos/loki/production/helm/loki/src
make[1]: Leaving directory '/home/user/repos/loki/production/helm/loki'

$ git status
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean

I think just manually delete that line then, and let's get this merged.
No need to keep merging main in, by the way.

Daniel-Vaz · 2024-01-17T08:07:54Z

Fingers crossed 🤞

dannykopping · 2024-01-17T08:59:30Z

😌 thanks for your patience @Daniel-Vaz! We appreciate the contribution a lot!

**What this PR does / why we need it**: Currently using the Loki Chart we can only either enable\disable ALL alert rules. For specific environments and use-cases sometimes not all alert Rules are useful to have enabled. With this PR change, we can cleanly and through the Chart values disable specific Alerts.

Helm: Allow per alert rule disabling on Loki

b77ec74

Daniel-Vaz requested a review from a team as a code owner November 16, 2023 08:49

pull-request-size bot added the size/S label Nov 16, 2023

github-actions bot added the area/helm label Nov 16, 2023

dannykopping reviewed Nov 16, 2023

View reviewed changes

Daniel-Vaz requested a review from dannykopping November 16, 2023 10:46

Daniel-Vaz added 2 commits November 16, 2023 17:13

Merge branch 'main' into main

dff672c

Merge branch 'main' into main

cd4f6bf

dannykopping approved these changes Nov 17, 2023

View reviewed changes

Merge branch 'main' into main

088e9f7

Daniel-Vaz and others added 6 commits November 20, 2023 11:30

update loki chart values docs about alerting rules changes

e8a0293

Merge branch 'main' of https://github.com/Daniel-Vaz/loki

966bdf4

Merge branch 'main' into main

90f014f

fixed values comment typo

9f94948

Merge branch 'main' of https://github.com/Daniel-Vaz/loki

9ebc1ea

Merge branch 'main' into main

3843338

Daniel-Vaz and others added 2 commits November 22, 2023 10:32

bumped version plus updated readme and changelog

1126bff

Merge branch 'main' into main

e93c2e3

auto-merge was automatically disabled December 12, 2023 11:11
Head branch was pushed to by a user without write access

Merge branch 'main' into main

be947fa

Daniel-Vaz requested a review from dannykopping December 18, 2023 09:58

Daniel-Vaz and others added 5 commits January 10, 2024 09:01

Merge branch 'main' into main

86f5a66

ran helm-docs

e3a2b7d

ran helm-docs

d7e419e

Merge branch 'main' into main

6ad1306

update

df43ad6

fix conflicts and add missing reference snippet

ba24793

pull-request-size bot added size/M and removed size/S labels Jan 16, 2024

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jan 16, 2024

Merge branch 'main' into main

2622fc5

Merge branch 'main' into main

28d254c

Daniel-Vaz added 3 commits January 17, 2024 08:05

rm line

f5253c1

Merge branch 'main' of https://github.com/Daniel-Vaz/loki

fa7022c

rm line

645456f

new line end of file

efbd44d

dannykopping merged commit dc1cb52 into grafana:main Jan 17, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add capability to disable specific alert rules Loki #11241

Add capability to disable specific alert rules Loki #11241

Daniel-Vaz commented Nov 16, 2023

CLAassistant commented Nov 16, 2023 •

edited

Loading

dannykopping left a comment

Daniel-Vaz commented Nov 16, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

dannykopping left a comment

shantanualsi commented Nov 17, 2023

Daniel-Vaz commented Nov 17, 2023

shantanualsi commented Nov 17, 2023 •

edited

Loading

Daniel-Vaz commented Nov 17, 2023 •

edited

Loading

shantanualsi commented Nov 20, 2023 •

edited

Loading

Daniel-Vaz commented Nov 20, 2023

JStickler commented Nov 20, 2023

shantanualsi commented Nov 22, 2023

Daniel-Vaz commented Nov 22, 2023

dannykopping commented Dec 18, 2023

Daniel-Vaz commented Jan 10, 2024

dannykopping commented Jan 15, 2024

Daniel-Vaz commented Jan 16, 2024

dannykopping commented Jan 16, 2024

Daniel-Vaz commented Jan 17, 2024

dannykopping commented Jan 17, 2024

Daniel-Vaz commented Jan 17, 2024

dannykopping commented Jan 17, 2024

Add capability to disable specific alert rules Loki #11241

Add capability to disable specific alert rules Loki #11241

Conversation

Daniel-Vaz commented Nov 16, 2023

CLAassistant commented Nov 16, 2023 • edited Loading

dannykopping left a comment

Choose a reason for hiding this comment

Daniel-Vaz commented Nov 16, 2023 • edited Loading

github-actions bot commented Nov 16, 2023 • edited Loading

dannykopping left a comment

Choose a reason for hiding this comment

shantanualsi commented Nov 17, 2023

Daniel-Vaz commented Nov 17, 2023

shantanualsi commented Nov 17, 2023 • edited Loading

Daniel-Vaz commented Nov 17, 2023 • edited Loading

shantanualsi commented Nov 20, 2023 • edited Loading

Daniel-Vaz commented Nov 20, 2023

JStickler commented Nov 20, 2023

shantanualsi commented Nov 22, 2023

Daniel-Vaz commented Nov 22, 2023

dannykopping commented Dec 18, 2023

Daniel-Vaz commented Jan 10, 2024

dannykopping commented Jan 15, 2024

Daniel-Vaz commented Jan 16, 2024

dannykopping commented Jan 16, 2024

Daniel-Vaz commented Jan 17, 2024

dannykopping commented Jan 17, 2024

Daniel-Vaz commented Jan 17, 2024

dannykopping commented Jan 17, 2024

CLAassistant commented Nov 16, 2023 •

edited

Loading

Daniel-Vaz commented Nov 16, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

shantanualsi commented Nov 17, 2023 •

edited

Loading

Daniel-Vaz commented Nov 17, 2023 •

edited

Loading

shantanualsi commented Nov 20, 2023 •

edited

Loading