-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: log all critical bounces #288
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mantariksh
force-pushed
the
improve-bounce-logging
branch
from
September 8, 2020 02:15
a814b55
to
4f003dd
Compare
Think you left out something in the Solution write-up "Log all critical bounces, including those for which {?}" |
tshuli
reviewed
Sep 8, 2020
karrui
approved these changes
Sep 9, 2020
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.
lgtm, with exception of the logging parameters. meta.action
should always be the calling function
arshadali172
reviewed
Sep 9, 2020
arshadali172
reviewed
Sep 9, 2020
arshadali172
reviewed
Sep 9, 2020
arshadali172
reviewed
Sep 9, 2020
arshadali172
reviewed
Sep 9, 2020
arshadali172
reviewed
Sep 9, 2020
arshadali172
reviewed
Sep 9, 2020
tshuli
approved these changes
Sep 10, 2020
mantariksh
force-pushed
the
improve-bounce-logging
branch
from
September 10, 2020 08:22
4f003dd
to
e8a5fb2
Compare
mantariksh
changed the title
feat: improve bounce logging
feat: log all critical bounces
Sep 10, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The current bounce collection and alarm system work as follows:
hasBounced
in the Bounce collection for each email recipient.hasBounced
is true for all the email recipients ANDhasAlarmed
is false, log a message containingCRITICAL BOUNCE
and sethasAlarmed
to true. IfhasAlarmed
is already true, do nothing.CRITICAL BOUNCE
. Every 5min, if there is at least 1 message containingCRITICAL BOUNCE
in the last 5min, activate the alarm.In other words, we do not log every single critical bounce. This is a mechanism to limit the rate of alarms; if there are critical bounces for one form ID, but lots of people submitting, we will still get only ONE alarm every 3 hours. This is because every 3 hours, the document in the Bounce collection will expire, upon which
hasAlarmed
will be set to its default of false. Once one critical bounce comes in,hasAlarmed
will be set to true. Subsequent critical bounces will see thathasAlarmed
is true and not log anything, hence will not activate the alarm again.The issue with this system is that we are unable to track the number of critical bounces over time, since we only send one log message per critical bounce per form every 3 hours. This is problematic for two reasons:
Solution
The following refactors and configuration changes were also made:
bounce.server.model
was moved insidesrc/app/modules/bounce
for better grouping of related code.bounce
model were inlined insrc/app/modules/bounce/__tests__
. A couple of configuration changes were made to accommodate this:tsconfig.build.json
was updated so that__tests__
directories are not compiled.jest.config.js
was updated so that files in the__tests__
directories are not included when calculating test coverage.Tests
Pre-release
formsg-email-notifications-staging
.Post-release