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

[feat][TRIP-10326] Add sentry captureMessage to Google calendar services #72

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

RubLo
Copy link

@RubLo RubLo commented Oct 23, 2023

@RubLo RubLo requested a review from a team as a code owner October 23, 2023 14:58
Copy link
Collaborator

@haffla haffla left a comment

Choose a reason for hiding this comment

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

Since you have the error object at hand maybe just use Sentry.captureException(error)?

@RubLo
Copy link
Author

RubLo commented Oct 23, 2023

@haffla I thought about that, but adding an extra string gives us a nicer visualization in the sentry report when it arrives in the slack channel.

The error object is also available in the Sentry report page.

But if you don't like it I can also remove the extra string part

@haffla
Copy link
Collaborator

haffla commented Oct 23, 2023

The visualization was actually my concern. This will result in quite long strings and I am not sure what "....: ${error}" will return. I think Sentry displays errors in a good way.

@RubLo RubLo requested a review from haffla October 24, 2023 07:53
@@ -286,6 +289,8 @@ export default class GoogleCalendarService implements Calendar {
* 410 is when an event is already deleted on the Google cal before on cal.com
* 404 is when the event is on a different calendar
*/
Sentry.captureException(err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here we actually don't want to always report the error. Only if the error code is not equal to 404 and not equal to 410. Only when this code returns a rejected promise, should we report the error.

Copy link
Collaborator

@haffla haffla left a comment

Choose a reason for hiding this comment

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

Thanks :)

@RubLo RubLo merged commit 7612c2d into main Oct 24, 2023
4 checks passed
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