-
Notifications
You must be signed in to change notification settings - Fork 427
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
fix: post script connection error + handle in logs #2259
Conversation
environment_id: environment.id, | ||
timestamp: Date.now(), | ||
content: `Post connection script failed with the error: ${errorString}` | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took the liberty to delete v1 logs, because I'm stripping everything down soon anyway
@@ -41,7 +41,7 @@ export async function externalPostConnection( | |||
const activityLogId = await createActivityLog(log); | |||
|
|||
const logCtx = await logContextGetter.create( | |||
{ id: String(activityLogId), operation: { type: 'post-connection-script' }, message: 'Start action' }, | |||
{ id: String(activityLogId), operation: { type: 'auth', action: 'post_connection' }, message: 'Start external post connection' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if "Start external post connection" makes sense. Would rather it say "Start external post connection script".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops yes you are right
@@ -74,8 +75,20 @@ async function execute(createdConnection: RecentlyCreatedConnection, provider: s | |||
const handler = handlers[`${provider.replace(/-/g, '')}PostConnection`]; | |||
|
|||
if (handler) { | |||
logCtx = await logContextGetter.create( | |||
{ operation: { type: 'auth', action: 'post_connection' }, message: 'Start internal post connection' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note above about adding "script.
"external" vs "internal" might be confusing to the user. Maybe for both it is just "Start post connection script". I think originally we didn't log this (which was intentional) and maybe since it is internal we should log it and on failure we just keep an internal logger
log so we can know about it. A user might be confused to see this log that they wouldn't necessarily know anything about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My own feeling about this is that it's part of the system that interact with the third parties so it should be logged. And it's not even enough imo since we don't handle the buffered logs in the proxy call.
A user might be confused to see this log that they wouldn't necessarily know anything about.
I feel like it's even worse if we only show errors to the users, they could think it never works
Also, no logs can mean "nothing was executed" or "something was a success "or "something crashed", which is not super nice for observability. And stdout don't scale indefinitely either.
Happy to involve @bastienbeurier on this. The context, we currently log:
- external post connection script: failure and success
- internal post connection: failure
We don't we log successful internal post connection, should we do it or keep it as is?
(side note: we never log the absence of post connection)
Alternative, should this log be part of the auth:create_connection log instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should err on the side of transparency, logging everything but being very explicit about what is executed by the platform script vs. what is executed by a user script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording and process changes. Let me know what you think.
Post connection script are currently not handled. https://github.com/NangoHQ/nango/pull/2225/files#diff-b5bb1e099cf24eec3ed6f52521ac05474b29cbef782cbf0b0db8dae7a2dee3ecR39-R41 |
provider_config_key: connection.provider_config_key, | ||
environment_id: connection.environment_id | ||
}, | ||
activityLogId, | ||
fileLocation | ||
fileLocation, | ||
input: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should removed since Post Connection Script don't take any input
Describe your changes
Fixes NAN-1093
I haven't paid enough attention when reviewing, there was a few things not working in post connection and in logs
PSC:
connection_id
,and missinginput
(@TBonnin I didn't wanted to change the default for input and I'm sure you already touching it in your own PR)Logs:
auth
)Test