fix(core): Start insights collection timer for webhook instances #15964
Conversation
n8n-assistant
bot
added
core
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
guillaumejacquart
force-pushed
the
pay-2901-insights-on-webhook-instances
branch
from
742a1e6 to
cf36bad
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 1 issue across 4 files. Review it in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
packages/cli/src/modules/insights/__tests__/insights.module.test.ts
Outdated
Show resolved
Hide resolved
guillaumejacquart
force-pushed
the
pay-2901-insights-on-webhook-instances
branch
3 times, most recently
from
34ac412 to
d1b4786
Compare
| if (!this.instanceSettings.isLeader && this.instanceSettings.instanceType !== 'webhook') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Won't this check incorrectly exclude a follower from flushing? As far as I understand, we want:
- leader flushes, compacts, prunes
- follower flushes
- webhook flushes
There was a problem hiding this comment.
So for now, we don't want followers to flush.
We want the instance that gets the workflow execution hooks triggered to buffer and flush, which means (unless I'm wrong):
- the main leader instance
- the webhook instance, as we've realized recently that webhook instance are the only instance receiving the hook (when workflow is triggered by a webhook)
I'm not sure follower instance also get the workflowExecuteAfter hook ran, but I hope note, otherwise we would get duplicated insights, because they would be saved when the buffer is full (even if the flushing timer is not running)
There was a problem hiding this comment.
I'm not sure follower instance also get the workflowExecuteAfter hook ran, but I hope not
Can we verify this please? I'd expect followers enqueue just like leaders and webhook instances do.
There was a problem hiding this comment.
Yes definitely, I'm doing that now !
There was a problem hiding this comment.
@ivov So I checked and you are right, followers can have the workflowExecuteAfter fired, in those cases:
- if they received the HTTP trigger of the workflow (from the load balancer)
- if they trigger the execution themselves (like manual executions) - although we don't save manual execs for insights
So I think you were right that followers need to buffer / flush as well, because all workflow execution will be scheduled / run by either:
- the main leader
- one of the followers
- the webhook instance
There was a problem hiding this comment.
@ivov I've implemented and tested the insights collection flushing for followers. All good when manually testing
guillaumejacquart
force-pushed
the
pay-2901-insights-on-webhook-instances
branch
from
fc804c6 to
d650fd2
Compare
|
✅ All Cypress E2E specs passed |
|
Got released with |
Summary
Before this PR, insights module did not start the collection flushing timer for webhook instances, meaning executions would only be saved when the buffer is full, instead of also every X seconds - 30 by default.
This PR starts the flushing timer for webhook instances as well, and split the timers starting between collection and the others (compaction and pruning) that are done in the main instances
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-2901/insights-on-webhook-instances
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)