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 realert_key feature #1004

Merged
merged 4 commits into from
Nov 11, 2022
Merged

Add realert_key feature #1004

merged 4 commits into from
Nov 11, 2022

Conversation

Goggin
Copy link
Contributor

@Goggin Goggin commented Nov 9, 2022

Description

Add realert_key option to rules that defaults to the rule name. Allows you to reuse the silence cache for multiple rules. We use this for account lockouts, if a user triggers multiple lockout rules, we don't want to call the lockout scripts multiple times.

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

Unsure if I need to write a specific test for this, since it is just a modification of what is set in the silence_cache_key, but I can write one if desired.

@@ -1675,7 +1675,7 @@ def silence(self, silence_cache_key=None):
# With --rule, self.rules will only contain that specific rule
if not silence_cache_key:
if self.args.silence_qk_value:
silence_cache_key = self.rules[0]['name'] + "." + self.args.silence_qk_value
silence_cache_key = self.rules[0]['realert_key'] + "." + self.args.silence_qk_value
else:
silence_cache_key = self.rules[0]['name'] + "._silence"
Copy link
Owner

@jertel jertel Nov 9, 2022

Choose a reason for hiding this comment

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

I'm wondering if this line 1680, and line 888, should also use the new rule['realert_key'] instead of rule['name']. If you made that change it would let users run elastalert from the command line, with the silence args and still silence an entire group of alerts instead of only one alert. For backward compatibility, line 888 could still check for rule['name'] + "._silence" so that pre-upgrade silenced rules remain silenced after the upgrade, but also check for `rule['realert_key'] + "._silence". Thoughts?

I can see how this contribution could be useful to some folks. Thanks for submitting the PR!

Copy link
Contributor Author

@Goggin Goggin Nov 10, 2022

Choose a reason for hiding this comment

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

I didn't do it for that because they may just want to manually silence a specific rule. Maybe there needs to be an option for both ?

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, that's fine with me to leave it as-is. I'll leave this open through tomorrow for any others to review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Thanks for your quick reply in anycase.

jertel
jertel previously approved these changes Nov 10, 2022
@jertel jertel merged commit 766a654 into jertel:master Nov 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 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.

2 participants