feat(server): Log webhooks from syncs separate from the sync #3312
Conversation
|
since we are improving the logging. Can we check if we could add some http logging that would help customers debug why calling the webhook endpoint failed? |
|
@TBonnin absolutely - I'll dig into that too |
| @@ -51,8 +51,8 @@ | |||
| "devDependencies": { | |||
| "@eslint/js": "9.17.0", | |||
| "@types/node": "20.12.2", | |||
| "@typescript-eslint/eslint-plugin": "8.19.1", | |||
| "@typescript-eslint/parser": "8.19.1", | |||
| "@typescript-eslint/eslint-plugin": "8.20.0", | |||
There was a problem hiding this comment.
Bumped these to try to fix the build failure you can see in the commit history. I figured I might as well keep the upgrade around.
packages/jobs/lib/execution/sync.ts
Outdated
| throw new Error('Missing connection.id'); | ||
| } | ||
|
|
||
| if (!nangoProps.syncConfig.id) { |
There was a problem hiding this comment.
These checks are how I ended up resolving the build issue in CI. More than happy to discuss further, but basically the lint error I was seeing in the build history on commits in this PR wasn't happening on my computer, and I could never get it to reproduce. This worked around it.
There was a problem hiding this comment.
I think you are just not up to date
There was a problem hiding this comment.
Just merged and am running through CI! 🤞🏻
packages/jobs/lib/execution/sync.ts
Outdated
| throw new Error('Missing connection.id'); | ||
| } | ||
|
|
||
| if (!nangoProps.syncConfig.id) { |
There was a problem hiding this comment.
I think you are just not up to date
packages/jobs/lib/execution/sync.ts
Outdated
| throw new Error(`Failed to send webhook for sync: ${nangoProps.syncConfig.sync_name}`); | ||
| } else { | ||
| await webhookLogCtx.success(); |
There was a problem hiding this comment.
do you think it could be self-contained inside sendSyncWebhook?
There was a problem hiding this comment.
Definitely - we'd just need to send a handful more parameters to sendSyncWebhook to help it have all the log context params it needs. I'm not sure if that's ideal with the package separation, but we've already got a function called sendSync so it's not like the webhook package doesn't know about syncs.
There was a problem hiding this comment.
@bodinsamuel See ee9c652 for version where the log context is created inside of sendSync.
Log webhooks from syncs in a different log context than the sync itself.
I'll be putting together future PR(s) to handle webhooks from refresh token failures and new connections.
See https://linear.app/nango/issue/NAN-1547/log-nango-webhooks
How I tested it