#3666 - Queue Monitoring - Default Queue Error Handling #4020
Conversation
andrewsignori-aot
changed the title
sources/packages/backend/apps/queue-consumers/src/processors/schedulers/base-queue.ts
Outdated
Show resolved
Hide resolved
| const result = await this.process(job, processSummary); | ||
| processSummary.info(`${job.queue.name}, job ID ${job.id}, executed.`); | ||
| const logsSum = processSummary.getLogLevelSum(); | ||
| if (logsSum.error) { |
There was a problem hiding this comment.
With logSum errors now certainly resulting in retry(being thrown) we should have a clear understanding with the team about when to use warning and error messages.
| /** | ||
| * Provides basic functionality for queue processing. | ||
| */ | ||
| export abstract class BaseQueue<T> { |
There was a problem hiding this comment.
It's good to have it in a abstract class. We can even have the logging for the job initialization and job ending in abstract methods if we that level of customization for some process. Good idea!
| async processQueue(job: Job<T>): Promise<string | string[]> { | ||
| const processSummary = new ProcessSummary(); | ||
| try { | ||
| this.logger.log(`Processing queue ${job.queue.name}, job ID ${job.id}.`); |
There was a problem hiding this comment.
Great initiative to centralize the common logic. ❤️
| workflowName, | ||
| job.data.assessmentId, | ||
| ); | ||
| return "Workflow call executed with success."; | ||
| } | ||
|
|
||
| @InjectLogger() |
There was a problem hiding this comment.
I believe the logger is not required here anymore.
There was a problem hiding this comment.
The logger should be added to every processor to ensure the right context is set (it is done during the property injection). Not having it causes the logs to be set to the base class context. We have many schedulers right now with this issue.
| "No impacts were detected on future applications.", | ||
| ); | ||
| } | ||
| } | ||
| await summary.info("Assessment cancelled with success."); | ||
| return summary.getSummary(); | ||
| return "Assessment cancelled with success."; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same with removing logger here.
There was a problem hiding this comment.
Please check #4020 (comment)
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
andrewsignori-aot
had a problem deploying
to
DEV
GitHub Actions
Failure
- Extended the `BaseScheduler` from the `BaseQueue` to enforce all the same basic features like `porcessSummary` and error handling, as agreed during PR #4020. - Started the refactor os some scheduler to be used as a baseline for the upcoming refactors. - cas-supplier-integration.scheduler.ts - cra-process-integration.scheduler.ts - cra-response-integration.scheduler.ts - Added the below code to every scheduler to be refactored. _Note:_ **this is also responsible for the Sonarcloud duplication issue.** **There are 23 files that have exactly the same change.** ```ts /** * To be removed once the method {@link process} is implemented. * This method "hides" the {@link Process} decorator from the base class. */ async processQueue(): Promise<string | string[]> { throw new Error("Method not implemented."); } /** * When implemented in a derived class, process the queue job. * To be implemented. */ protected async process(): Promise<string | string[]> { throw new Error("Method not implemented."); } ```
This PR is a proposal to unify the queue behavior. It contains changes for start and cancel assessments regular queues and the idea is to expand it to all schedulers.
The below image shows these points to be shared and forced for all the queues, using the CAS scheduler as an example.
1 - Return type;
2 - Provide a
processSummary;3 - try/catch and default start message.
4 - Error check on
processSummaryto force the jobs to fail.5 - Error handling on
catch.6 - Final log activities during
finally.Note: The above will also ensure the E2E tests follow a closer approach.
Sample log for a queue using the new
BaseQueue<T>class.Side effects
The idea is to have the BaseScheduler extend the BaseQueue which will force a change in every single scheduler (which would be the idea). To avoid the need to change every single scheduler and adapt every E2E, the methods required by BaseQueue can be implemented temporarily as below. The idea is to start the scheduler refactor still during this ticket.
Error Fix
Fixed an issue with the logger service where
nullmessage was logged right after the regular message.