-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Feature] Time error groupping #99
base: master
Are you sure you want to change the base?
Conversation
joaquinco
commented
Sep 18, 2024
- Add a time throttling mechanism to prevent multiple errors to trigger notifications. It works by scheduling a notification after the last error until :throttle milliseconds have passed without an error or :time_limt milliseconds have passed from the first occurrence.
2d8eb8f
to
02422c0
Compare
Fix test that was not detecting this
02422c0
to
51cfb59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 💪
Just left some questions
|
||
```elixir | ||
defmodule YourApp.Router do | ||
use Phoenix.Router | ||
|
||
use BoomNotifier, | ||
notification_trigger: [:exponential], | ||
count: [:exponential], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though I like this name better I'd rather not to introduce api changes in this pull request
I think before releasing v1.0.0
we should revisit the whole api and address the changes in a separate PR
notifiers: [ | ||
# ... | ||
], | ||
groupping: :time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it makes sense to have :time
as other notification_trigger
strategy instead of adding a new option? And maybe we could use the same time limit key we use for the others