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

add_alertmanager_resolve_timeout #1187

Merged
merged 5 commits into from
Jul 14, 2023
Merged

add_alertmanager_resolve_timeout #1187

merged 5 commits into from
Jul 14, 2023

Conversation

eveningcafe
Copy link
Contributor

@eveningcafe eveningcafe commented Jun 1, 2023

Description

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

@eveningcafe
Copy link
Contributor Author

The thing is, alertmanager has a default resov_timeout is 5 minutes, meaning after 5 minutes if it doesn't r receive any repeat alert it will consider the alert stop firing. Sure, you can set "realert" below 5 min in your rules to keep alert fire, but it will affect another alert output. I think the best option is we can keep that timeout configurable in the rules, like this :\

alertmanager_alertname: "freq_alert" 
alertmanager_resolve_timeout: 5 # it will affect end_time - aka time to live - of each alert sent to alertmanager.

Users who config the rules can be aware of this config and expect the right behavior for alertmanager.

@eveningcafe
Copy link
Contributor Author

@jertel

@nsano-rururu
Copy link
Collaborator

nsano-rururu commented Jun 1, 2023

Please add alertmanager_resolve_timeout to the documentation. If there is a default value, please also provide the default value.
https://github.com/jertel/elastalert2/blob/master/docs/source/ruletypes.rst#alertmanager

Please use the validation check setting of alertmanager_resolve_timeout
https://github.com/jertel/elastalert2/blob/master/elastalert/schema.yaml#L321

alertmanager_resolve_timeout: {type: integer}

@nsano-rururu
Copy link
Collaborator

Please add test code for alertmanager_resolve_timeout
https://github.com/jertel/elastalert2/blob/master/tests/alerters/alertmanager_test.py

@nsano-rururu
Copy link
Collaborator

Please update CHANGELOG.md

@eveningcafe
Copy link
Contributor Author

ok I'll do it now. ~ chotto matte. But the ideal ok right?

@jertel
Copy link
Owner

jertel commented Jun 26, 2023

Hello @eveningcafe: Are you planning to continue this PR? If so I'll leave it open for a couple more weeks. Let us know.

Thanks!

@eveningcafe
Copy link
Contributor Author

oh yes, sorry. I'll do. please leave it open

@nsano-rururu
Copy link
Collaborator

@eveningcafe

Are there minimum and maximum values for alertmanager_resolve_timeout settings?

@eveningcafe
Copy link
Contributor Author

Hey guys, how does mock build respose of my request to 'http://alertmanager:9093' ? Here
image
Sorry, I'm new to mock and currently failing some unit tests. :(
image

Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

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

I refactored your implementation to make it a non-breaking change, and to utilize a consistent time configuration, similar to timeframe and buffer_time. Also added a unit test.

@jertel jertel merged commit 33d0d1f into jertel:master Jul 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants