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

[Feature] Notification delivery throttling timeout #92

Merged
merged 32 commits into from
Sep 18, 2024

Conversation

joaquinco
Copy link
Contributor

@joaquinco joaquinco commented Aug 13, 2024

Introduction

Allow users to specify a timeout for notification deliveries that are hold off by some notification_trigger counting strategy.

Changes

  • Refactor code that gets generated by using BoomNotifier module by just defining wrapper functions of the notify_error related logic (now in BoomNotifier.Api) with a specific configuration.
  • Refactor code so that notification triggering is decided in the NotificationSender (ex NotifierSenderServer) GenServer. So we can handle the timeout in the same process.
  • Change tests that used self() to send messages back and forth test examples to named process.
  • Save the error info hash key in the ErrorInfo struct
  • Unify Faker Mailers

@joaquinco joaquinco force-pushed the feature/group-by-timeout branch from 742e272 to b5ae6d7 Compare August 13, 2024 20:00
@joaquinco joaquinco changed the title [Feature] Notification delivery throttling [Feature] Notification delivery throttling timeout Aug 14, 2024
@joaquinco joaquinco force-pushed the feature/group-by-timeout branch 6 times, most recently from 80590c8 to 729c952 Compare August 20, 2024 18:33
@joaquinco joaquinco force-pushed the feature/group-by-timeout branch from 8bfea51 to 5eac749 Compare August 23, 2024 16:58
@joaquinco joaquinco force-pushed the feature/group-by-timeout branch from e5c7693 to b3fd0af Compare August 26, 2024 14:22
Copy link

@germanbottosur germanbottosur left a comment

Choose a reason for hiding this comment

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

I've been taking a look at the code in general and I think looks good overall 🙌

Apart from that, I think we should take a look at the String.to_existing_atom/1 thing and to the way we retrieve the error occurrences when resetting the accumulated errors. I still have to take a closer look at the NotificationSender but I suspect we may introduce a race condition if we don't retrieve the occurrences and reset the accumulated errors in an atomic step for the ErrorStorage agent (let's say that some other process is storing a new error occurrence while the sender is resetting it).

I'll be taking a deep look at the notification sender process later and I've left some suggestions to try to keep a consistent use of the pipe operator.

Great work 🙌

lib/boom_notifier/notification_sender.ex Show resolved Hide resolved
test/support/bamboo_fake_mailer.ex Outdated Show resolved Hide resolved
test/unit/error_info_test.exs Show resolved Hide resolved
lib/boom_notifier/error_info.ex Outdated Show resolved Hide resolved
test/unit/notification_sender_test.exs Show resolved Hide resolved
test/unit/notification_sender_test.exs Outdated Show resolved Hide resolved
test/unit/mailer_notifier_test.exs Outdated Show resolved Hide resolved
test/unit/notifier_test.exs Show resolved Hide resolved
test/support/test_utils.ex Outdated Show resolved Hide resolved
lib/boom_notifier/error_info.ex Outdated Show resolved Hide resolved
lib/boom_notifier/error_info.ex Outdated Show resolved Hide resolved
lib/boom_notifier/error_storage.ex Outdated Show resolved Hide resolved
@iaguirre88
Copy link
Member

@joaquinco Thanks for taking your time for implementing this 🚀
It's looking great 🤘

@iaguirre88
Copy link
Member

I'm gonna let you @germanbottosur do the approval since you've been working more closely on this 💖

Copy link

@germanbottosur germanbottosur left a comment

Choose a reason for hiding this comment

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

🙌

@joaquinco joaquinco merged commit 64f07d6 into wyeworks:master Sep 18, 2024
27 checks passed
@joaquinco joaquinco deleted the feature/group-by-timeout branch September 18, 2024 12:38
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.

3 participants