Helm: Fix Prometheus rule LokiTooManyCompactorsRunning #7049

calvinbui · 2022-09-05T06:54:53Z

Signed-off-by: Calvin Bui 3604363+calvinbui@users.noreply.github.com

What this PR does / why we need it:

The Helm chart deploys two services, loki and loki-headless.

The loki_boltdb_shipper_compactor_running metric will show all loki pods attached to both services.

The Prometheus rule in the chart, LokiTooManyCompactorsRunning, does an aggregation over an entire namespace, which will always double the amount of compactors it detects.

Which issue(s) this PR fixes:
Fixes N/A

Special notes for your reviewer:

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

Signed-off-by: Calvin Bui <3604363+calvinbui@users.noreply.github.com>

CLAassistant · 2022-09-05T06:55:00Z

All committers have signed the CLA.

Signed-off-by: Calvin Bui <3604363+calvinbui@users.noreply.github.com>

calvinbui · 2022-09-05T06:56:32Z

CLA signed and chart version bumped

grafanabot · 2022-09-05T07:00:55Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

kavirajk

LGTM. Thanks @calvinbui for the PR. Sounds reasonable to me. Leave it to @trevorwhitney for final approval.

Added one minor suggestion to the changelog entry.

production/helm/loki/CHANGELOG.md

Co-authored-by: Kaviraj Kanagaraj <kavirajkanagaraj@gmail.com>

calvinbui · 2022-09-06T00:32:44Z

On second thought, I'm not sure if this is the best approach as there'll be two alerts if it does go >1

trevorwhitney

On second thought, I'm not sure if this is the best approach as there'll be two alerts if it does go >1

Agreed. I think instead we should only scrape 1 of those services. My inclination would be to drop loki-headless, WDYT?

calvinbui · 2022-09-11T23:37:50Z

On second thought, I'm not sure if this is the best approach as there'll be two alerts if it does go >1

Agreed. I think instead we should only scrape 1 of those services. My inclination would be to drop loki-headless, WDYT?

what about the simple-scalable services? they use -read and -write suffixed services

varac · 2022-09-20T13:02:29Z

Hej, looking forward for this fix, what's the current state ?

trevorwhitney · 2022-09-20T22:37:41Z

what about the simple-scalable services? they use -read and -write suffixed services

yeah, good point, looks like maybe we want to always deploy the loki service (for both single binary and scalable topologies), and then we can just scrape this service and drop metrics from headless. WDYT?

zarikx · 2022-09-23T11:09:51Z

If I'm not mistaken, loki-headless just needs prometheus.io/service-monitor: "false" label (similar to service-read-headless and service-write-headless services) to prevent discovering single-binary StatefulSet twice.

…binary headless service

calvinbui · 2022-09-25T23:27:37Z

If I'm not mistaken, loki-headless just needs prometheus.io/service-monitor: "false" label (similar to service-read-headless and service-write-headless services) to prevent discovering single-binary StatefulSet twice.

agreed, the best solution.

PR updated

grafanabot · 2022-09-25T23:33:40Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

trevorwhitney

lgtm! thanks!

grafanabot · 2022-10-10T00:59:15Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
- querier/queryrange	-0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

trevorwhitney · 2022-10-18T22:27:07Z

@calvinbui looks like this just needs a helm-docs update and it's good to go.

grafanabot · 2022-10-18T22:44:23Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

trevorwhitney · 2022-10-21T16:05:35Z

@calvinbui looks like a markdown linting error?

grafanabot · 2022-10-24T05:07:38Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

calvinbui · 2022-10-24T05:16:43Z

@calvinbui looks like a markdown linting error?

I've updated my branch. I didn't do any changes to the markdown before, and this time around either.

trevorwhitney · 2022-10-25T22:17:08Z

@calvinbui looks like all we need is one more rebase, a lot moving on the helm chart these past few days 😄 Thanks!

grafanabot · 2022-10-31T00:12:40Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

-           ingester	-0.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

calvinbui · 2022-10-31T22:11:23Z

😦😩

) **What this PR does / why we need it**: Ignore the headless service from the Prometheus service-monitor as it was resulting in duplicate metrics. Replaces #7049

…afana#7638) **What this PR does / why we need it**: Ignore the headless service from the Prometheus service-monitor as it was resulting in duplicate metrics. Replaces grafana#7049

Helm: Fix Prometheus rule LokiTooManyCompactorsRunning

a718a5d

Signed-off-by: Calvin Bui <3604363+calvinbui@users.noreply.github.com>

calvinbui requested a review from a team as a code owner September 5, 2022 06:54

pull-request-size bot added the size/XS label Sep 5, 2022

calvinbui marked this pull request as draft September 5, 2022 06:55

Helm: bump chart version

71f2097

Signed-off-by: Calvin Bui <3604363+calvinbui@users.noreply.github.com>

pull-request-size bot added size/S and removed size/XS labels Sep 5, 2022

calvinbui marked this pull request as ready for review September 5, 2022 06:56

kavirajk reviewed Sep 5, 2022

View reviewed changes

production/helm/loki/CHANGELOG.md Outdated Show resolved Hide resolved

Reword changelog

c2a566b

Co-authored-by: Kaviraj Kanagaraj <kavirajkanagaraj@gmail.com>

kavirajk requested a review from trevorwhitney September 5, 2022 07:41

trevorwhitney added the area/helm label Sep 7, 2022

trevorwhitney requested changes Sep 7, 2022

View reviewed changes

iamleot mentioned this pull request Sep 7, 2022

Add Loki as single instance backed by filesystem iamleot/rpi-flux#19

Merged

calvinbui added 2 commits September 26, 2022 09:25

Merge branch 'main' into main

c459867

Revert

90d573e

pull-request-size bot added size/XS and removed size/S labels Sep 25, 2022

Add missing label prometheus.io/service-monitor: "false" to single-…

46a7337

…binary headless service

calvinbui requested review from trevorwhitney and removed request for trevorwhitney September 25, 2022 23:27

calvinbui requested review from trevorwhitney and kavirajk and removed request for trevorwhitney September 25, 2022 23:27

trevorwhitney approved these changes Sep 26, 2022

View reviewed changes

zarikx mentioned this pull request Oct 3, 2022

LokiTooManyCompactorsRunning alert incorrectly fires when using single binary mode with v3.2.0 of Helm chart #7315

Open

Merge branch 'main' into main

caa0bdc

calvinbui requested review from trevorwhitney and removed request for kavirajk October 10, 2022 00:53

calvinbui added 2 commits October 19, 2022 09:37

Merge branch 'grafana:main' into main

01e8460

helm-docs

f7d1d63

calvinbui added 2 commits October 24, 2022 15:57

Merge branch 'main' into main

f5e703b

bump chart and run helm-docs

a586807

Merge branch 'main' into main

b20b3a8

pull-request-size bot added size/S and removed size/XS labels Oct 31, 2022

calvinbui closed this Oct 31, 2022

trevorwhitney mentioned this pull request Nov 8, 2022

[helm] Ignore single binary headless service from service-monitor #7638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm: Fix Prometheus rule LokiTooManyCompactorsRunning #7049

Helm: Fix Prometheus rule LokiTooManyCompactorsRunning #7049

calvinbui commented Sep 5, 2022

CLAassistant commented Sep 5, 2022 •

edited

calvinbui commented Sep 5, 2022

grafanabot commented Sep 5, 2022

kavirajk left a comment

calvinbui commented Sep 6, 2022 •

edited

trevorwhitney left a comment

calvinbui commented Sep 11, 2022

varac commented Sep 20, 2022

trevorwhitney commented Sep 20, 2022

zarikx commented Sep 23, 2022

calvinbui commented Sep 25, 2022

grafanabot commented Sep 25, 2022

trevorwhitney left a comment

grafanabot commented Oct 10, 2022

trevorwhitney commented Oct 18, 2022

grafanabot commented Oct 18, 2022

trevorwhitney commented Oct 21, 2022

grafanabot commented Oct 24, 2022

calvinbui commented Oct 24, 2022

trevorwhitney commented Oct 25, 2022

grafanabot commented Oct 31, 2022

calvinbui commented Oct 31, 2022

Helm: Fix Prometheus rule LokiTooManyCompactorsRunning #7049

Helm: Fix Prometheus rule LokiTooManyCompactorsRunning #7049

Conversation

calvinbui commented Sep 5, 2022

CLAassistant commented Sep 5, 2022 • edited

calvinbui commented Sep 5, 2022

grafanabot commented Sep 5, 2022

kavirajk left a comment

Choose a reason for hiding this comment

calvinbui commented Sep 6, 2022 • edited

trevorwhitney left a comment

Choose a reason for hiding this comment

calvinbui commented Sep 11, 2022

varac commented Sep 20, 2022

trevorwhitney commented Sep 20, 2022

zarikx commented Sep 23, 2022

calvinbui commented Sep 25, 2022

grafanabot commented Sep 25, 2022

trevorwhitney left a comment

Choose a reason for hiding this comment

grafanabot commented Oct 10, 2022

trevorwhitney commented Oct 18, 2022

grafanabot commented Oct 18, 2022

trevorwhitney commented Oct 21, 2022

grafanabot commented Oct 24, 2022

calvinbui commented Oct 24, 2022

trevorwhitney commented Oct 25, 2022

grafanabot commented Oct 31, 2022

calvinbui commented Oct 31, 2022

CLAassistant commented Sep 5, 2022 •

edited

calvinbui commented Sep 6, 2022 •

edited