-
Notifications
You must be signed in to change notification settings - Fork 452
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
feat(server): Enrich webhook errors #3319
base: master
Are you sure you want to change the base?
Conversation
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.
Same points already raised by Thomas, otherwise looks good 👍🏻
Can you take the opportunity to log success and error the same way in deliver()
?
@bodinsamuel handled deliver errors in a36990b |
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.
💪🏻
if (response.status >= 200 && response.status < 300) { | ||
await logCtx.info( | ||
await logCtx.http( | ||
`${webhookType} webhook sent successfully to the ${type} ${url} and received with a ${response.status} response code${endingMessage ? ` ${endingMessage}` : ''}.`, |
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.
message for HTTP log should be METHOD URL
(yes it's not good everywhere :D)
@@ -134,6 +134,7 @@ export interface MessageRow { | |||
response: { | |||
code: number; | |||
headers: Record<string, string>; | |||
body?: unknown; |
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.
Just had a thought before going to bed, you need to add this prop to schema.ts as keyword "index": false and "doc_values": false otherwise it will blow up
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.
Fixed in 2c9e4f9
Enriches webhook errors with much more information about the request that was made, to help customers with debugging.
Of course they should log these requests themselves, but there are certainly also cases like gateway errors where they wouldn't receive these and it could be good to see the full request from our side:
In that screenshot above, I fixed this small bug:
How I tested it: