-
Notifications
You must be signed in to change notification settings - Fork 7k
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
chore: better logging for failed webhooks #14825
chore: better logging for failed webhooks #14825
Conversation
@Shpadoinkle is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
Graphite Automations"Add community label" took an action on this PR • (05/01/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (05/01/24)1 reviewer was added to this PR based on Keith Williams's automation. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
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.
LGTM 🚀
Thank you for the contribution.
console.error( | ||
`Error executing webhook for event: ${eventTrigger}, URL: ${webhook.subscriberUrl}, bookingId: ${evt.bookingId}, bookingUid: ${evt.uid}`, | ||
e | ||
); |
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.
@Shpadoinkle can you please replace this with logger?
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.
you can import logger from "@calcom/lib/logger";
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.
updated
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.
Requesting changes so this is blocked from merge until @Udit-takkar's requested change is implemented
70c1037
to
3792f31
Compare
3792f31
to
cab9982
Compare
cab9982
to
a6cbdb8
Compare
a6cbdb8
to
f6d3b9e
Compare
@keithwillcode @Udit-takkar anything else you need for this pr? |
f6d3b9e
to
9ee430d
Compare
@alishaz-polymath can you check if there is anything else wanted on this? |
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.
LGTM
@Udit-takkar what's do you think?
Base branch was modified
What does this PR do?
When a webhook fails to send it's payload. Current logs aren't helpful for finding out which booking failed.
Fixes # (issue)
#13279
Requirement/Documentation
Type of change
Chore
How should this be tested?
force a webhook to fail.
Logs should shed more light on the Booking it was for
Mandatory Tasks