Adjust and rename `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Remove `ThanosSidecarPrometheusDown` alert; Remove unused `thanos_sidecar_last_heartbeat_success_time_seconds` metrics #4508

arajkumar · 2021-08-02T11:47:07Z

Prior to this fix, ThanosSidecarUnhealthy would fire even when
Prometheus is busy with WAL replay. This would trigger a false positive alert.

This PR considers prometheus_tsdb_data_replay_duration_seconds metric from
Prometheus for ThanosSidecarUnhealthy alert. In order to correlate
Thanos and Prometheus metrics we need to specify common label(s) which
can be configured through thanosPrometheusCommonDimensions jsonnet
variable.

This PR also removes ThanosSidecarPrometheusDown as it would fire at the same as ThanosSidecarUnhealthy.

Rename ThanosSidecarUnhealthy to ThanosSidecarNoConnectionToStartedPrometheus.

Remove unused thanos_sidecar_last_heartbeat_success_time_seconds metric.

Fixes #3915.

Signed-off-by: Arunprasad Rajkumar [email protected]

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Verification

bwplotka

I am not 100% sure if this is right approach. Sounds like we would like to set some dependency and alert only if Prometheus is healthy but sidecar is not.

I think in this case that might work, but let's make sure to be explicit. I would even vote for alert name to be explit, to something SidecarNoConnectionToStartedPrometheus, WDYT?

examples/alerts/alerts.yaml

arajkumar · 2021-08-05T11:08:30Z

Thanks a lot @bwplotka for this quick review.

I am not 100% sure if this is right approach. Sounds like we would like to set some dependency and alert only if Prometheus is healthy but sidecar is not.

Do you mean to express alert dependencies through inhibition rules? If so, I thought of that and @simonpasquier suggested that it might over complicate.

I think in this case that might work, but let's make sure to be explicit. I would even vote for alert name to be explit, to something SidecarNoConnectionToStartedPrometheus, WDYT?

+1, How about ThanosSidecarNotConnectedToPrometheus?

bwplotka · 2021-08-05T11:13:15Z

Yes it might be hard, especially when one might not alert when Prometheus is reloading (which makes sense!)

I would prefer what I propose as from title you expect this failing when it's not connected, but it will NOT fail if not connected AND if Prometheus is reloading which might surprise op. (:

examples/alerts/alerts.yaml

bwplotka · 2021-08-10T11:41:03Z

CHANGELOG.md

@@ -18,6 +18,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re

 - [#4468](https://github.com/thanos-io/thanos/pull/4468) Rule: Fix temporary rule filename composition issue.
 - [#4476](https://github.com/thanos-io/thanos/pull/4476) UI: fix incorrect html escape sequence used for '>' symbol.
+- [#4508](https://github.com/thanos-io/thanos/pull/4508) Fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarUnhealthy.


This entry does not make sense (fix ThanosSidecarUnhealthy and remove ThanosSidecarUnhealthy) - what we try to say here?

Indeed it was a typo :'(

examples/alerts/alerts.yaml

bwplotka · 2021-08-10T11:43:58Z

What about alert name?

What about ThanosSidecarNoConnectionToStartedPrometheus?

arajkumar · 2021-08-10T13:06:20Z

ThanosSidecarNoConnectionToStartedPrometheus

@bwplotka looks good to me.

bwplotka

Thanks! Let's fix changelog, otherwise LGTM 💪🏽

bwplotka · 2021-08-17T15:10:47Z

CHANGELOG.md

@@ -18,6 +18,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re

 - [#4468](https://github.com/thanos-io/thanos/pull/4468) Rule: Fix temporary rule filename composition issue.
 - [#4476](https://github.com/thanos-io/thanos/pull/4476) UI: fix incorrect html escape sequence used for '>' symbol.
+- [#4508](https://github.com/thanos-io/thanos/pull/4508) Refactor sidecar alerts.


This is not useful changelog item, is it? (:

Good to have, how about "Refactor sidecar alerts(ThanosSidecarUnhealthy, ThanosSidecarPrometheusDown)"?

Since this PR does 3 things, found it bit hard to put a concise title :)

fix ThanosSidecarUnhealthy

removes ThanosSidecarPrometheusDown

rename ThanosSidecarUnhealthy to ThanosSidecarNoConnectionToStartedPrometheus

We need to be specific here, otherwise it's hard for user to learn quickly if they are impacted or not.

What about

Suggested change

- [#4508](https://github.com/thanos-io/thanos/pull/4508) Refactor sidecar alerts.

- [#4508](https://github.com/thanos-io/thanos/pull/4508) Adjusted and renamed `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Removed `ThanosSidecarPrometheusDown`

@bwplotka Applied your suggestion, PTAL.

examples/alerts/alerts.yaml

bwplotka

2 more suggestions. Also please run make docs again - tests are failing.

Thanks!

bwplotka · 2021-09-06T08:47:27Z

examples/alerts/alerts.yaml

    expr: |
-      time() - max by (job, instance) (thanos_sidecar_last_heartbeat_success_time_seconds{job=~".*thanos-sidecar.*"}) >= 240
+      time() - max by (pod, job, instance) (thanos_sidecar_last_heartbeat_success_time_seconds{job=~".*thanos-sidecar.*"}) >= 240


Sorry for challenging this last minute, but isn't expression with thanos_sidecar_prometheus_up simpler?

Suggested change

time() - max by (pod, job, instance) (thanos_sidecar_last_heartbeat_success_time_seconds{job=~".*thanos-sidecar.*"}) >= 240

thanos_sidecar_prometheus_up{job=~".*thanos-sidecar.*"}) == 0

Great question :) AFAIK, thanos_sidecar_last_heartbeat_success_time_seconds and thanos_sidecar_prometheus_up carries the same information encoded in different ways. Let me evaluate and switch to thanos_sidecar_prometheus_up if it is the case.

@bwplotka If we switch to thanos_sidecar_prometheus_up , We may not able to show the duration at which sidecar is not connected in the alert description. Hope that is fine.
e.g.

Thanos Sidecar thanos-sidecar-1 is unhealthy for more than 720 seconds.

pkg/rules/rules_test.go

arajkumar · 2021-09-08T06:44:34Z

@bwplotka I don't know, for some reason make check-docs never fails on machine but always fails on CI. Looks like something related to line ending on OSX vs Linux.

bwplotka

Thanks, LGTM, just we need to move the changelog item now and add one more info.

bwplotka · 2021-09-20T09:10:20Z

CHANGELOG.md

@@ -28,6 +28,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#4476](https://github.com/thanos-io/thanos/pull/4476) UI: fix incorrect html escape sequence used for '>' symbol.
 - [#4532](https://github.com/thanos-io/thanos/pull/4532) Mixin: Fixed "all jobs" selector in thanos mixin dashboards.
 - [#4607](https://github.com/thanos-io/thanos/pull/4607) Azure: Fix Azure MSI Rate Limit
+- [#4508](https://github.com/thanos-io/thanos/pull/4508) Adjust and rename `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Remove `ThanosSidecarPrometheusDown`.


This has to be moved now to the top of this change log (different release)

cmd/thanos/sidecar.go

Prior to this fix, ThanosSidecarUnhealthy would fire even when Prometheus is busy with WAL replay. This would trigger a false positive alert. This PR considers prometheus_tsdb_data_replay_duration_seconds metric from Prometheus for ThanosSidecarUnhealthy alert. In order to correlate Thanos and Prometheus metrics we need to specify common label(s) which can be confiured through thanosPrometheusCommonDimensions jsonnet variable. This PR also removes ThanosSidecarPrometheusDown as it would fire at the same as ThanosSidecarUnhealthy. Fixes #3915. Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Arunprasad Rajkumar <[email protected]>

…ometheus Signed-off-by: Arunprasad Rajkumar <[email protected]>

…decar_prometheus_up Signed-off-by: Arunprasad Rajkumar <[email protected]>

…_time_seconds metric Signed-off-by: Arunprasad Rajkumar <[email protected]>

bwplotka

👍🏽 Thanks!

This commit pulls latest changes form thanos mixins and sets `thanosPrometheusCommonDimensions` to `namespace, pod` for k8s use case. Refer thanos-io/thanos#4508 for more details. Signed-off-by: Arunprasad Rajkumar <[email protected]>

This commit pulls latest changes from thanos mixins and sets `thanosPrometheusCommonDimensions` to `namespace, pod` for k8s use case. Refer thanos-io/thanos#4508 for more details. Signed-off-by: Arunprasad Rajkumar <[email protected]>

bwplotka previously approved these changes Aug 5, 2021

View reviewed changes

examples/alerts/alerts.yaml Outdated Show resolved Hide resolved

examples/alerts/alerts.yaml Outdated Show resolved Hide resolved

arajkumar dismissed bwplotka’s stale review via ca69ce9 August 5, 2021 10:49

bwplotka reviewed Aug 6, 2021

View reviewed changes

examples/alerts/alerts.yaml Show resolved Hide resolved

arajkumar changed the title ~~fix: consider prometheus wal replay for ThanosSidecarUnhealthy alert~~ fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarUnhealthy Aug 9, 2021

arajkumar changed the title ~~fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarUnhealthy~~ Fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarUnhealthy Aug 9, 2021

arajkumar requested a review from bwplotka August 9, 2021 09:52

bwplotka reviewed Aug 10, 2021

View reviewed changes

arajkumar changed the title ~~Fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarUnhealthy~~ Fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarPrometheusDown Aug 10, 2021

arajkumar changed the title ~~Fix ThanosSidecarUnhealthy alert during prometheus WAL replay and remove ThanosSidecarPrometheusDown~~ Refactor sidecar alerts Aug 10, 2021

arajkumar requested a review from bwplotka August 11, 2021 07:02

bwplotka previously approved these changes Aug 17, 2021

View reviewed changes

arajkumar requested a review from bwplotka August 25, 2021 05:20

arajkumar dismissed bwplotka’s stale review via a6b5b00 August 26, 2021 11:07

arajkumar changed the title ~~Refactor sidecar alerts~~ Adjust and rename ThanosSidecarUnhealthy to ThanosSidecarNoConnectionToStartedPrometheus; Remove ThanosSidecarPrometheusDown Aug 26, 2021

bwplotka reviewed Sep 6, 2021

View reviewed changes

arajkumar requested a review from bwplotka September 6, 2021 11:46

bwplotka previously approved these changes Sep 20, 2021

View reviewed changes

arajkumar dismissed bwplotka’s stale review via ab4be6b September 20, 2021 10:33

arajkumar and others added 3 commits September 21, 2021 14:34

Rename ThanosSidecarUnhealthy to ThanosSidecarNoConnectionToStartedPr…

ba8ac83

…ometheus Signed-off-by: Arunprasad Rajkumar <[email protected]>

Simplify ThanosSidecarNoConnectionToStartedPrometheus using thanos_si…

9eb576c

…decar_prometheus_up Signed-off-by: Arunprasad Rajkumar <[email protected]>

Remove unused implementation of thanos_sidecar_last_heartbeat_success…

e9ab760

…_time_seconds metric Signed-off-by: Arunprasad Rajkumar <[email protected]>

arajkumar requested a review from bwplotka September 21, 2021 10:10

bwplotka approved these changes Sep 24, 2021

View reviewed changes

bwplotka merged commit d5351b0 into thanos-io:main Sep 24, 2021

arajkumar mentioned this pull request Sep 27, 2021

thanos: bump to latest and add thanosPrometheusCommonDimensions prometheus-operator/kube-prometheus#1399

Merged

5 tasks

arajkumar mentioned this pull request Sep 27, 2021

Bug 1942913: Make ThanosSidecarNoConnectionToStartedPrometheus resilient to WAL replays openshift/cluster-monitoring-operator#1399

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust and rename `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Remove `ThanosSidecarPrometheusDown` alert; Remove unused `thanos_sidecar_last_heartbeat_success_time_seconds` metrics #4508

Adjust and rename `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Remove `ThanosSidecarPrometheusDown` alert; Remove unused `thanos_sidecar_last_heartbeat_success_time_seconds` metrics #4508

arajkumar commented Aug 2, 2021 •

edited

Loading

bwplotka left a comment

arajkumar commented Aug 5, 2021

bwplotka commented Aug 5, 2021

bwplotka Aug 10, 2021

arajkumar Aug 10, 2021

bwplotka commented Aug 10, 2021

arajkumar commented Aug 10, 2021

bwplotka left a comment

bwplotka Aug 17, 2021

arajkumar Aug 18, 2021 •

edited

Loading

bwplotka Aug 26, 2021

arajkumar Aug 26, 2021

bwplotka left a comment

bwplotka Sep 6, 2021

arajkumar Sep 6, 2021

arajkumar Sep 6, 2021 •

edited

Loading

arajkumar commented Sep 8, 2021

bwplotka left a comment

bwplotka Sep 20, 2021

bwplotka left a comment

	- [#4508](https://github.com/thanos-io/thanos/pull/4508) Refactor sidecar alerts.
	- [#4508](https://github.com/thanos-io/thanos/pull/4508) Adjusted and renamed `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Removed `ThanosSidecarPrometheusDown`

	time() - max by (pod, job, instance) (thanos_sidecar_last_heartbeat_success_time_seconds{job=~".thanos-sidecar."}) >= 240
	thanos_sidecar_prometheus_up{job=~".thanos-sidecar."}) == 0

Adjust and rename ThanosSidecarUnhealthy to ThanosSidecarNoConnectionToStartedPrometheus; Remove ThanosSidecarPrometheusDown alert; Remove unused thanos_sidecar_last_heartbeat_success_time_seconds metrics #4508

Adjust and rename ThanosSidecarUnhealthy to ThanosSidecarNoConnectionToStartedPrometheus; Remove ThanosSidecarPrometheusDown alert; Remove unused thanos_sidecar_last_heartbeat_success_time_seconds metrics #4508

Conversation

arajkumar commented Aug 2, 2021 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

arajkumar commented Aug 5, 2021

bwplotka commented Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Aug 10, 2021

arajkumar commented Aug 10, 2021

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajkumar Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajkumar Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

arajkumar commented Sep 8, 2021

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Adjust and rename `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Remove `ThanosSidecarPrometheusDown` alert; Remove unused `thanos_sidecar_last_heartbeat_success_time_seconds` metrics #4508

Adjust and rename `ThanosSidecarUnhealthy` to `ThanosSidecarNoConnectionToStartedPrometheus`; Remove `ThanosSidecarPrometheusDown` alert; Remove unused `thanos_sidecar_last_heartbeat_success_time_seconds` metrics #4508

arajkumar commented Aug 2, 2021 •

edited

Loading

arajkumar Aug 18, 2021 •

edited

Loading

arajkumar Sep 6, 2021 •

edited

Loading