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 Worker/Queues to process notification based on status #268

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

SerpentBytes
Copy link
Contributor

@SerpentBytes SerpentBytes commented Feb 23, 2023

Description

This PR resolves #113, #115.

What's working?

  • Worker/Queue pair to run checks every 5 mins for DNS record and certs expiring in less than a month
  • Send Notification once.

Steps to Test / Re-Test

  • Delete redis data and restart the Redis container (clear any previous jobs)
  • npm run setup (to update to DB)
  • Run the project (make sure Redis is up and running)
  • npm run db:studio (to see the lastNotified field getting updated)
  • Navigate to the MailHog interface at http://localhost:8025 to see notifications after 5 mins (see Note)

Note
To test if the jobs are repeating without waiting for 5 mins, modify the value of repeat here to every 5 seconds.

@SerpentBytes SerpentBytes force-pushed the issue-108 branch 2 times, most recently from 825d869 to 49f6dcd Compare February 23, 2023 02:38
@SerpentBytes SerpentBytes self-assigned this Feb 23, 2023
@SerpentBytes SerpentBytes added this to the Milestone 0.4 milestone Feb 23, 2023
@humphd humphd requested a review from a user February 23, 2023 13:26
@SerpentBytes SerpentBytes marked this pull request as ready for review February 23, 2023 16:42
@SerpentBytes SerpentBytes added category: notifications A service to notify users about their certificates/domains area: functionality Back end microservices that contribute to our main functionality category: queue A service that connects and manages certificate creation/expiration labels Feb 23, 2023
@humphd humphd requested a review from Genne23v February 23, 2023 18:14
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I read through this, and I'm going to suggest that we don't need a dedicated worker/queue for this. When we build the flow for creating/updating/deleting DNS, we're going to need something very similar to what @dadolhay has done for certs (i.e., a series of jobs that flow in order). Part of those jobs will be responding to the state changes, and sending a notification can be part of that.

If we run this query as a repeating job, we'll end-up spamming people with notifications every day. I don't think this will be a good experience, and it would be better to do it in a controlled way, based on a user action (e.g., user clicks Update or Delete etc and we start the flow).

Having said that, you have the basis here (and knowledge) for another feature we need, namely, running a query every day to expire domains, delete expired certs, send notifications of upcoming expirations, etc.

Would you be willing to evolve this code to focus on that? Maybe start with sending a notification if a domain or cert will expire in 1 month?

@SerpentBytes SerpentBytes marked this pull request as draft February 23, 2023 20:20
@humphd
Copy link
Contributor

humphd commented Feb 23, 2023

Filed #270 for the DNS flow I'm describing above.

@ghost
Copy link

ghost commented Feb 23, 2023

I agree with what @humphd proposed, with the exception, that this should not only run once a day, it should run every hour or even every 5 minutes.

If you allow tasks to pile up throughout the day, it will have to send a lot of mails all at once, and it is a lot easier to hit a rate limit + 3rd party providers do not really like large spike-like loads. I'd suggest to run it often, and only handle the tasks that are due, that represents a lot more even load on the system.

@humphd
Copy link
Contributor

humphd commented Feb 23, 2023

I agree with what @humphd proposed, with the exception, that this should not only run once a day, it should run every hour or even every 5 minutes.

If you allow tasks to pile up throughout the day, it will have to send a lot of mails all at once, and it is a lot easier to hit a rate limit + 3rd party providers do not really like large spike-like loads. I'd suggest to run it often, and only handle the tasks that are due, that represents a lot more even load on the system.

Sounds good, and something we can do load testing on later, then measure and tune.

@humphd
Copy link
Contributor

humphd commented Feb 24, 2023

One thing to call out with repeat jobs: we're going to need to not spam users. For example, if you run this every 5 mins (or every day), we need some way to keep track of the fact that we've already notified a user and don't need to do it again. There are various ways to approach this, and we'll need one of them.

@SerpentBytes SerpentBytes force-pushed the issue-108 branch 3 times, most recently from e38d970 to 04ae8a4 Compare February 25, 2023 21:00
@SerpentBytes SerpentBytes marked this pull request as ready for review February 25, 2023 21:13
@SerpentBytes
Copy link
Contributor Author

I have modified the code to check for expiring records and certificates every 5 mins and persist if the user is notified. At the moment the type is boolean, but we can replace it with a number value so we can send two reminders.

@humphd
Copy link
Contributor

humphd commented Feb 26, 2023

Let's move this to 0.5. I'm not going to be able to review it fully in time for shipping today.

@Genne23v Genne23v removed this from the Milestone 0.4 milestone Feb 26, 2023
@humphd
Copy link
Contributor

humphd commented Mar 10, 2023

Looks like there's a conflict to sort out still.

@SerpentBytes
Copy link
Contributor Author

It Looks like there's a conflict to sort out still.

Yes. I am just waiting for a few more reviews.

sfrunza13
sfrunza13 previously approved these changes Mar 10, 2023
@SerpentBytes SerpentBytes linked an issue Mar 10, 2023 that may be closed by this pull request
humphd
humphd previously approved these changes Mar 10, 2023
@SerpentBytes SerpentBytes marked this pull request as ready for review March 10, 2023 22:22
 * move notifcations queues related code in notifications/
 * Add a button to send trigger adding jobs
 * install cron
 * workers/queues for sending notifications based on domain status
 * update import path
 * Update structure and link worker with queue
 * Add comments
 * switch to interface
 * Data interface for domain status checks
 * Add UI to interact with notifications job processing
 * Update code
 * Add default in switch statment
 * Remove queuescheduler
 * Fix ESLint CI warnings
 * Rename IDomainStatusData.ts to domain-status-data.interface.ts
 * Update path and fix typo in job name
 * Add NOTIFICATIONS_REPEAT_PATTERN
 * Make jobs repeatable and send notifications
 * Update comments
 * Remove cron-parser, update cron value
 * Rename file to: record-expiration-monitor-worker.server.ts
 * Update variable and function names
 * Rename file to: record-expiration-notification.server.ts
 * Rename file to: certifcate-expiration-monitor-worker.server.ts
 * Update pattern to every 5 mins
 * Update expiration values to send notifications
 * Add isNotified field for certificate and record
 * Add isNotified field
 * Update routes and function calls
 * Update code to check for expiration for records/certs
 * Remove test log messages
 * fix: eslint check failing
 * fix: typecheck failing
 * Update code based on feedback
 * updated lock file
 * Update variable name to timesNotified
 * Update code based on feedback
 * remove NOTIFICATION_REPEAT_PATTERN
 * Add Notification table
 * Remove async/await from query function
 * hardcode value for repeating jobs
 * Refactor code based on feedback
 * Remove NOTIFICATION_REPEAT_PATTERN
 * Remove padding and update code based on feedback
 * Remove record-cert-expiration-date.interface.ts
 * remove checks for null and handle promises correctly
 * fix: if statement; use init model; update: log messages; simplify adding jobs
 * fix: Change from logger.error to logger.warn
 * Add logger.error back
 * add dayjs
 * Use dayjs
 * Update code
 * Update code
 * fix: typecheck
 * add NOTIFICATION_FREQUENCY constant
 * fix: update the value to 7
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: functionality Back end microservices that contribute to our main functionality category: notifications A service to notify users about their certificates/domains category: queue A service that connects and manages certificate creation/expiration
Projects
None yet
4 participants