Skip to content
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

Mixin: Update sidecar alert #2002

Merged
merged 4 commits into from
Feb 4, 2020
Merged

Conversation

daixiang0
Copy link
Member

@daixiang0 daixiang0 commented Jan 15, 2020

Signed-off-by: Xiang Dai [email protected]

Fix #544

@kakkoyun
Copy link
Member

kakkoyun commented Jan 15, 2020

@daixiang0 Would you maybe consider implementing alerts and dashboards for sidecar in thanos-mixin? That one is missing.

@daixiang0
Copy link
Member Author

I would like to do this :)

@kakkoyun
Copy link
Member

Super, cool @daixiang0. Ping me if you need anything on slack :)

@daixiang0
Copy link
Member Author

daixiang0 commented Jan 16, 2020

@kakkoyun thanks, i would file another pr for that.

@daixiang0 daixiang0 requested a review from bwplotka January 16, 2020 03:33
@kakkoyun
Copy link
Member

If you gonna create that PR, you will probably automate the generation of the alert examples so maybe we should close this one and focus on the upcoming PR. @daixiang0 WDYT?

@daixiang0
Copy link
Member Author

It would need much time to finish while this useless alert may make users confused i think.

@daixiang0 daixiang0 changed the title Remove ThanosSidecarBucketOperationsFailed alert Mixin: Update sidecar alert Feb 1, 2020
@daixiang0
Copy link
Member Author

@kakkoyun please review.

examples/alerts/alerts.md Show resolved Hide resolved
examples/alerts/alerts.yaml Outdated Show resolved Hide resolved
mixin/thanos/alerts/sidecar.libsonnet Outdated Show resolved Hide resolved
mixin/thanos/alerts/sidecar.libsonnet Show resolved Hide resolved
Signed-off-by: Xiang Dai <[email protected]>
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Besides my inline comments, we aim to generate examples in https://github.com/thanos-io/thanos/blob/master/examples/alerts/alerts.md file, so check

[//]: # "TODO(kakkoyun): Generate sidecar rules using thanos-mixin."
<!-- [embedmd]:# (../tmp/thanos-sidecar.rules.yaml yaml) -->
out.
If you uncomment and make sure the path is correct it'd embed generated file for the documentation.
Better to be consistent and generate examples for sidecar like the others.

mixin/thanos/alerts/sidecar.libsonnet Show resolved Hide resolved
Signed-off-by: Xiang Dai <[email protected]>
@daixiang0 daixiang0 requested review from povilasv and removed request for bwplotka February 4, 2020 09:16
@daixiang0
Copy link
Member Author

@kakkoyun ping

impact: We will lose metrics data if not fixed in 24h
action: Check {{ $labels.kubernetes_pod_name }} pod logs in {{ $labels.kubernetes_namespace}} namespace
dashboard: SIDECAR_URL
- alert: ThanosSidecarGrpcErrorRate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daixiang0 Let's not lose this alert, we have similar more up-to-date ones in other components' alerts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we're good to merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impact and action part are only seen in this alert, is it necessary to add for all component alert?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd say it's not necessary. At this stage, these are just examples to start from.

@bwplotka
Copy link
Member

bwplotka commented Feb 4, 2020

Who is generating this? will make docs generate it? 🤔

@kakkoyun
Copy link
Member

kakkoyun commented Feb 4, 2020

@bwplotka it's make examples since it's where it gets updated.

thanos/Makefile

Lines 335 to 337 in 56a1fb6

examples: jsonnet-format mixin/thanos/README.md examples/alerts/alerts.md examples/alerts/alerts.yaml examples/alerts/rules.yaml examples/dashboards examples/tmp
$(EMBEDMD) -w examples/alerts/alerts.md
$(EMBEDMD) -w mixin/thanos/README.md

@bwplotka bwplotka merged commit 7c02430 into thanos-io:master Feb 4, 2020
@daixiang0 daixiang0 deleted the update-alert branch February 4, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ThanosSidecarPrometheusDown error no metrics thanos_objstore_bucket_operation_failures_total
4 participants