refactor(core): Move all execution lifecycle telemetry events to lifecycle hooks (no-changelog) #12816
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
n8n-assistant
bot
added
core
| if (hooks.hasOwnProperty(key)) { | ||
| result[key]!.push(...hooks[key]!); | ||
| } |
There was a problem hiding this comment.
- Do we need the
hasOwnPropertycheck for a key coming fromin? - Can we avoid the assertions? Maybe like this?
if (!result[key] || !hooks[key]) continue;
There was a problem hiding this comment.
- no. type safety should take care of that.
- sure
| workflowExecuteBefore: [ | ||
| async function (this: WorkflowHooks): Promise<void> { | ||
| const { executionId, workflowData } = this; | ||
| eventService.emit('workflow-pre-execute', { executionId, data: workflowData }); |
There was a problem hiding this comment.
Odd that workflow-pre-execute doesn't need user ID but workflow-post-execute does.
There was a problem hiding this comment.
yeah, there are still a couple of inconsistencies. I'd like to change this in another PR though.
| }, | ||
| ], | ||
| workflowExecuteAfter: [ | ||
| async function (this: WorkflowHooks, runData: IRun): Promise<void> { |
There was a problem hiding this comment.
What's our policy on inferrable return types? Personally I find them noisy.
There was a problem hiding this comment.
quite a lot of explicit typing in this file will be removed after I refactor WorkflowHooks to be type-safe.
| }; | ||
| } | ||
|
|
||
| function hookFunctionsNodeTelemetry(): IWorkflowExecuteHooks { |
There was a problem hiding this comment.
telemetry is inaccurate because more things happen in reaction to these events, e.g. log streaming. hookFunctionsNodeEvents?
| async function (this: WorkflowHooks, nodeName: string): Promise<void> { | ||
| const { executionId, workflowData: workflow } = this; | ||
| eventService.emit('node-pre-execute', { executionId, workflow, nodeName }); | ||
| }, | ||
| ], | ||
| nodeExecuteAfter: [ | ||
| async function (this: WorkflowHooks, nodeName: string): Promise<void> { | ||
| const { executionId, workflowData: workflow } = this; | ||
| eventService.emit('node-post-execute', { executionId, workflow, nodeName }); | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Do these need to be async? Same elsewhere.
There was a problem hiding this comment.
the async is coming from the typing. I'll change this in the WorkflowHooks refactor.
| hookFunctionsWorkflowTelemetry(userId), | ||
| hookFunctionsNodeTelemetry(), |
There was a problem hiding this comment.
Now that these hooks are no longer in WorkflowRunner and in WorkflowExecuteAdditionalData, can it happen that events reach listeners in the wrong sequence? e.g. log streaming writing a node-execute-before event before writing a workflow-execute-before event. Is that a risk?
There was a problem hiding this comment.
that was what I was concerned about when I originally started this refactor in #12364 .
I've since tested this a bunch already, and haven't seen any issues so far.
also, the order of these was already inconsistent between various modes.
The only thing that I can tell is affected by these bring reordered is the final status determination. but to address that perhaps we should do that before any other workflowExecuteAfter hook in every mode. WDYT?
| } | ||
|
|
||
| const hookFunctions = mergeHookFunctions(...toMerge); |
There was a problem hiding this comment.
mergeHookFunctions should accept an array or a variadic argument?
There was a problem hiding this comment.
I'm fine either way. do you prefer one over the other?
|
|
||
| // TODO: why are workers pushing to frontend? | ||
| // TODO: simplifying this for now to just leave the bare minimum hooks | ||
| // hookFunctions.push(hookFunctionsPush()); |
There was a problem hiding this comment.
Can we delete those stale TODOs while we're at it?
Summary
This PR moves telemetry events out of workflow-execute-additional-data and WorkflowRunner, and into execution lifecycle hooks, to make these temetry hooks consistent across various modes.
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)