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

fix: action and webhook logs improvements #2634

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented Aug 23, 2024

  • Added request/response details to action operation payload
  • Removed duplicates logs. We were indeed logging more or less the same thing at different levels (handler in jobs, trigger function and/or controller).
  • Log messages for actions and webhooks are now shorter and mostly without data (ex: 'Started action A', 'The action failed')
    Details like connection, provider, truncated response, error payload are now in the log payload

I want to add request/response details for proxied requests made within a script but it requires modifying the persist API which is ingesting the logs coming from the runner. I will do that in next PR. It would also be useful for the validation warning message

Note: You can ignore the [Object Object], 100 log in the screenshot below. It is me manually logging something in the test script

Action operation:
Screenshot 2024-08-23 at 11 16 01

Action started log
Screenshot 2024-08-23 at 11 16 45

Action successful log
Screenshot 2024-08-23 at 11 17 15

Failed action operation:
Screenshot 2024-08-23 at 11 17 49

Action failed log
Screenshot 2024-08-23 at 11 18 11

Same for webhook.
Note that operation is not enriched with request/response. @samuel I'll ping you to see if we can add it
Screenshot 2024-08-23 at 11 25 26

Issue ticket number and link

https://linear.app/nango/issue/NAN-1525/improve-actions-error-logs

Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Works well, way cleaner 👌🏻
Minor comments, also there is no message when a sync is starting, it was like that before but it feels inconsistent

@@ -105,11 +105,6 @@ export async function startAction(task: TaskAction): Promise<Result<void>> {
}
}
export async function handleActionSuccess({ nangoProps }: { nangoProps: NangoProps }): Promise<void> {
const logCtx = await logContextGetter.get({ id: String(nangoProps.activityLogId) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be a relieve for the infra I think 👌🏻

Comment on lines +116 to +117
connection: connection.connection_id,
integration: connection.provider_config_key
Copy link
Collaborator

@bodinsamuel bodinsamuel Aug 26, 2024

Choose a reason for hiding this comment

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

those 2 things are already logged in the operation (applicable for multiple lines)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we easily show them in the message payload then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

they are shown on top of the Operation drawer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes but this is to also show this info in the individual message payload.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally we don't store those information again just for display reason, you can pass the operation to the message drawer if needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it. Let me remove them. This info is not absolutely required in the message drawing for now

packages/shared/lib/clients/orchestrator.ts Show resolved Hide resolved
@@ -340,6 +339,20 @@ class SyncController {
}

next(err);
} finally {
const reqHeaders = getHeaders(req.headers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

On your other PR you also need this + the redacted
Maybe you should consolidate everything with this new function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes that's my goal

@TBonnin
Copy link
Collaborator Author

TBonnin commented Aug 26, 2024

also there is no message when a sync is starting, it was like that before but it feels inconsistent

I will work on sync logs improvements when I am done with actions

- Added request/response to action operation payload
- Removed duplicates logs. We were indeed logging more or less the same
  thing at different levels (handler in jobs, trigger function and/or
  controller).
- Log messages for actions and webhooks are now shorter and mostly without data
(ex: 'Started action A', 'The action failed')
Details like connection, provider, truncated response, error payload are
now in the log payload

I want to add request/response details for proxied requests made within
a script but it requires modifying the persist API which is ingesting
the logs coming from the runner. I will do that in next PR
@TBonnin TBonnin force-pushed the tbonnin/action-logs-improvements branch from d4bb39c to 61438a2 Compare August 26, 2024 17:35
@TBonnin TBonnin merged commit 9e9747b into master Aug 26, 2024
32 checks passed
@TBonnin TBonnin deleted the tbonnin/action-logs-improvements branch August 26, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants