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.

Sign up for GitHub

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

Jump to bottom

mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert #3567

Merged
merged 1 commit into from
Dec 17, 2020
Merged

mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert #3567

merged 1 commit into from
Dec 17, 2020

Conversation

MalloZup
Copy link
Contributor

@MalloZup MalloZup commented Dec 10, 2020 •
edited
Loading

Description:

Metric was removed on PR 2002 accidentally. fix #3005

  • I added CHANGELOG entry for this change.

Changes

  • reintroduce alert thanos_objstore_bucket_operation_failures_total which was deleted by mistake

Verification

tested with make example-rules-lint

@MalloZup MalloZup changed the title mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert [WIP:] mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert Dec 10, 2020
@MalloZup MalloZup changed the title [WIP:] mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert [WIP]: mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert Dec 10, 2020
@yeya24
Copy link
Contributor

yeya24 commented Dec 10, 2020

Hi, please add this alert to the jsonnet file https://github.com/thanos-io/thanos/blob/master/mixin/alerts/sidecar.libsonnet.
The markdown files can be auto-generated I think.

@MalloZup MalloZup force-pushed the renable-alert branch 2 times, most recently from babac04 to 23a0e51 Compare December 10, 2020 21:41
@MalloZup MalloZup changed the title [WIP]: mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert mixin: Reintroduce thanos_objstore_bucket_operation_failures_total alert Dec 10, 2020
@MalloZup
Copy link
Contributor Author

MalloZup commented Dec 10, 2020 •
edited
Loading

@yeya24 thx. I added the jsonnet and generated the markdown via makefile targets. Let me know if something else is missing.

@MalloZup
Copy link
Contributor Author

I see unit-test are failing I will take look on this.

@MalloZup MalloZup force-pushed the renable-alert branch 3 times, most recently from 12e71a6 to eba61a6 Compare December 11, 2020 13:55
@yeya24
Copy link
Contributor

yeya24 commented Dec 11, 2020

I see unit-test are failing I will take look on this.

That's not related to this pr so all good.

@yeya24
Copy link
Contributor

yeya24 commented Dec 16, 2020

CC @kakkoyun or @metalmatze for a review ^^

kakkoyun
kakkoyun requested changes Dec 16, 2020
View reviewed changes
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. Thanks for your contribution. Only one comment.

mixin/alerts/sidecar.libsonnet Outdated Show resolved Hide resolved
@MalloZup
Copy link
Contributor Author

MalloZup commented Dec 16, 2020 •
edited
Loading

@yeya24 @kakkoyun thank you for your reviews. I have fixed your kind comments.

Let me know if other things are missing. 🙏

After this I wanted also to look to a next issue to contribute on 🐕 🐕‍🦺

yeya24
yeya24 requested changes Dec 16, 2020
View reviewed changes
examples/alerts/alerts.yaml Outdated Show resolved Hide resolved
@MalloZup
Reintroduce thanos_objstore_bucket_operation_failures_total alert
d00e8d2 
Metric was removed on PR 2002 by mistake

Signed-off-by: dmaiocchi <dmaiocchi@suse.com>
@MalloZup MalloZup requested a review from yeya24 December 17, 2020 11:10
kakkoyun
kakkoyun approved these changes Dec 17, 2020
View reviewed changes
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.

LGTM 🚀 Thanks for the contribution.

@kakkoyun
Copy link
Member

@yeya24 Feel free to merge it if it's also looking good to you.

@MalloZup
Copy link
Contributor Author

MalloZup commented Dec 17, 2020 •
edited
Loading

thx guys for patience and help for getting through this. 🌻
I have learned the mixin concept of prometheus/grafana, which I wasn't quite aware, so I did definitely discovered something new by this PR 🐩

And also jsonnet, was not familiar with it. I knew dhall and toml but not yet jsonnet. 😁 That is definitely something I enjoy when picking up new issue on unfamiliar environment .. Thx for reviews again 🎸

yeya24
yeya24 approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

@yeya24 yeya24 merged commit 5c12d46 into thanos-io:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeya24 yeya24 yeya24 approved these changes

@kakkoyun kakkoyun kakkoyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alert missing for bucket operation failures on the sidecar
3 participants
@MalloZup @yeya24 @kakkoyun