Feature/gap 2152 v2 notifications service #59
Conversation
…> unsubscribe reference table
Co-authored-by: dominicwest <[email protected]>
| console.log(`Number of emails sent: ${notifications.length}`); | ||
| await this.savedSearchNotificationService.deleteSentSavedSearchNotifications(); | ||
| console.log( | ||
| `saved search notifications temp table has been cleared`, | ||
| ); | ||
| console.log( | ||
| `Task took ${ | ||
| performance.now() - startTime | ||
| } milliseconds to run \r\n`, | ||
| ); |
There was a problem hiding this comment.
Some of these logs will be really unclear when debugging in AWS
- Its logging inside of a loop so will be spammy/unclear
- Doesn't reference the scheduled job its running under
saved search notifications temp table has been cleareddoesn't feel significant enough to log unless of course theres an error
| const CRON_JOB_MAP = { | ||
| [ScheduledJobType.GRANT_UPDATED]: | ||
| this.v2GrantService.processGrantUpdatedNotifications, | ||
| [ScheduledJobType.GRANT_UPCOMING]: | ||
| this.v2GrantService.processGrantUpcomingNotifications, | ||
| [ScheduledJobType.NEW_GRANTS]: | ||
| this.v2GrantService.processNewGrantsNotifications, | ||
| [ScheduledJobType.SAVED_SEARCH_MATCHES]: | ||
| this.v2SavedSearchService.processSavedSearchMatches, | ||
| [ScheduledJobType.SAVED_SEARCH_MATCHES_NOTIFICATION]: | ||
| this.v2SavedSearchService | ||
| .processSavedSearchMatchesNotifications, | ||
| }; | ||
| const cronFn = CRON_JOB_MAP[type as keyof typeof CRON_JOB_MAP]; | ||
| const cronJob = getCronJob(cronFn, timer); | ||
| this.schedularRegistry.addCronJob(`${type}_${index}`, cronJob); | ||
| cronJob.start(); |
There was a problem hiding this comment.
At first this confused me, but now I understand it love this refactor 👌
| console.log( | ||
| `Number of saved saved searches to process: ${ | ||
| savedSearches ? savedSearches.length : 0 | ||
| }`, | ||
| ); | ||
| console.log( | ||
| `Number of saved search notifications created: ${numberOfSearchesWithMatches}`, | ||
| ); | ||
| console.log( | ||
| `Task took ${endTime - startTime} milliseconds to run \r\n`, | ||
| ); |
There was a problem hiding this comment.
Would ideally reference the scheduled job in each log, easy for them to get lost amongst other things happening concurrently in prod
| } | ||
| } | ||
|
|
||
| await this.contentfulService.updateEntries(grantIds, { |
There was a problem hiding this comment.
This function makes a lot of logs, would get rid of it logging the whole contentful entry and make it a bit clearer what its doing. Again, reference the scheduled job its running under in the logs
There was a problem hiding this comment.
Just a comment in general - for all logs related to these jobs, reference the job name its running under. And some additional logs around numbers affected and things like that would be ideal. There's a couple excessive logs too that output a whole contentful blob which is not ideal. Appreciate a lot of this was copy and paste, but lets improve things so its easier to debug if anything goes wrong
There was a problem hiding this comment.
Saved search scheduled jobs fail locally for me, I believe this is because of the cron jobs ive set them to run at and that this was a bug before. But ideally it shouldn't be so unstable. Falls over because it tries to insert a duplicate saved_search_notification before the other job clears it or something.
GAP-2152
related prs: cabinetoffice/gap-find-web#45, cabinetoffice/gap-user-service#135