refactor(core): Move event and telemetry handling into workers in queue mode #7138
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
…-with-current-state # Conflicts: # packages/cli/src/commands/worker.ts
…-with-current-state
…-with-current-state
…-with-current-state # Conflicts: # packages/cli/src/AbstractServer.ts
…-with-current-state
…-with-current-state # Conflicts: # packages/cli/src/eventbus/eventBus.controller.ts
…-with-current-state
…rker # Conflicts: # packages/cli/src/commands/worker.ts # packages/cli/src/eventbus/MessageEventBus/MessageEventBus.ts
|
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Make sure to check off this list before asking for review. |
n8n-assistant
bot
added
core
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #7138 +/- ##
=======================================
Coverage 32.40% 32.40%
=======================================
Files 3276 3278 +2
Lines 198005 198019 +14
Branches 21652 21646 -6
=======================================
+ Hits 64162 64167 +5
- Misses 132782 132791 +9
Partials 1061 1061
☔ View full report in Codecov by Sentry. |
krynble
reviewed
Sep 12, 2023
flipswitchingmonkey
changed the title
…rker # Conflicts: # packages/cli/src/commands/worker.ts # packages/cli/src/worker/workerCommandHandler.ts
krynble
previously approved these changes
Sep 13, 2023
| @@ -193,7 +199,7 @@ export class Worker extends BaseCommand { | |||
| ); | |||
| await additionalData.hooks.executeHookFunctions('workflowExecuteAfter', [failedExecution]); | |||
| } | |||
| return { success: true }; | |||
| return { success: true, error: error as ExecutionError }; | |||
There was a problem hiding this comment.
We can also remove this property since this is not in use at the moment
There was a problem hiding this comment.
that one was actually being used but i just noticed that merging master in removed it...
krynble
approved these changes
Sep 13, 2023
| @@ -23,6 +23,7 @@ export interface JobData { | |||
|
|
|||
| export interface JobResponse { | |||
| success: boolean; | |||
| error?: ExecutionError; | |||
There was a problem hiding this comment.
Nit: maybe remove also this one?
|
|
1 flaky test on run #2165 ↗︎
Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| NDV > resolves expression with default item when input node is not parent, while still pairing items |
Output
Screenshots
Video
|
|
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
|
|
2 similar comments
|
|
|
|
|
✅ All Cypress E2E specs passed |
flipswitchingmonkey
deleted the
pay-780-move-onworkflowpostexecute-into-worker
branch
|
Got released with |
ivov
added a commit
that referenced
this pull request
Nov 20, 2023
This PR adds `status` to run data so that `determineFinalExecutionStatus` resolves correctly on execution failure and removes the cleanup that is being duplicated in a worker hook. Followup to #7138 Should fix: - #7705 - https://linear.app/n8n/issue/PAY-964/no-execution-found-after-execution-fails - https://linear.app/n8n/issue/PAY-1010/execution-deletion-in-queue-mode-not-complying-with-settings
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.
Motivation
In Queue mode, finished executions would cause the main instance to always pull all execution data from the database, unflatten it and then use it to send out event log events and telemetry events, as well as required returns to Respond to Webhook nodes etc.
This could cause OOM errors when the data was large, since it had to be fully unpacked and transformed on the main instance’s side, using up a lot of memory (and time).
This PR attempts to limit this behaviour to only happen in those required cases where the data has to be forwarded to some waiting webhook, for example.
Changes
Execution data is only required in cases, where the active execution has a
postExecutePromiseattached to it. These usually forward the data to some other endpoint (e.g. a listening webhook connection).By adding a helper
getPostExecutePromiseCount(), we can decide that in cases where there is nothing listening at all, there is no reason to pull the data on the main instance.Previously, there would always be postExecutePromises because the telemetry events were called. Now, these have been moved into the workers, which have been given the various InternalHooks calls to their hook function arrays, so they themselves issue these telemetry and event calls.
This results in all event log messages to now be logged on the worker’s event log, as well as the worker’s eventbus being the one to send out the events to destinations. The main event log does…pretty much nothing.
We are not logging executions on the main event log any more, because this would require all events to be replicated 1:1 from the workers to the main instance(s) (this IS possible and implemented, see the worker’s
replicateToRedisEventLogFunction- but it is not enabled to reduce the amount of traffic over redis).Partial events in the main log could confuse the recovery process and would result in, ironically, the recovery corrupting the execution data by considering them crashed.
Refactor
I have also used the opportunity to reduce duplicate code and move some of the hook functionality into
packages/cli/src/executionLifecycleHooks/shared/sharedHookFunctions.tsin preparation for a future full refactor of the hooks