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

refactor(core): Decouple post workflow execute event from internal hooks (no-changelog) #10232

Closed

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Jul 29, 2024

Follow-up to: #10221

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jul 29, 2024
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

perhaps we should prefix all telemetry events with telemetry. and audit events with audit. instead of trying to use the same event for both.

executionId,
success: runData.status === 'success',
isManual: runData.mode === 'manual',
audit: {
Copy link
Member

Choose a reason for hiding this comment

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

why not emit two separate events?

Copy link
Contributor

Choose a reason for hiding this comment

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

Other option would be to keep the event payload generic enough so both telemetry-event-relay and audit-event-relay can then derive whatever data they need from that. Tho that makes it then implicit what happens based on the sent event. Then again that's usually the point of using event buses: to decouple consumers from producers. Now the coupling is there, as we explicitly define the data separately for each consumer.

success: data.status === 'success',
isManual: data.mode === 'manual',
userId: additionalData.userId,
audit: {
Copy link
Member

Choose a reason for hiding this comment

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

same here.

@ivov
Copy link
Contributor Author

ivov commented Jul 30, 2024

prefix all telemetry events

I thought of prefixing event names but ruled this out, because there is only a small minority of events that are sent to both relays with different payloads, because this adds coupling to all events and relays, and because I expect that sending multiple events with different payloads at all callsites will be noisy.

For these edge cases I'd like to aim for unifying both payloads as they're usually similar enough, but that would require a breaking change. Sending a big payload from which relays pick what they need is error prone (we have no tests) and causes the payload type to be unclear. Payloads with nesting in edge cases like this (although adding a bit of coupling) are meant as a workaround until we can unify.

@ivov ivov requested a review from tomi July 30, 2024 15:08
Comment on lines +305 to +318
// audit and telemetry
userId?: string;

// audit only
executionId: string;
success: boolean;
workflowId: string;
isManual: boolean;
workflowName: string;
metadata?: Record<string, string>;

// telemetry only
workflow: IWorkflowBase;
runData?: IRun;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified to

Suggested change
// audit and telemetry
userId?: string;
// audit only
executionId: string;
success: boolean;
workflowId: string;
isManual: boolean;
workflowName: string;
metadata?: Record<string, string>;
// telemetry only
workflow: IWorkflowBase;
runData?: IRun;
userId?: string;
executionId: string;
workflow: IWorkflowBase;
runData?: IRun;

as metadata doesn't seem to be passed in any emit and all the other fields can be derived from workflow and runData. That way there's no need to duplicate the logic for deriving those fields

@ivov
Copy link
Contributor Author

ivov commented Jul 31, 2024

Too many conflicts to safely resolve, I'll reopen from master soon enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants