Skip to content

Conversation

@vladanpaunovic
Copy link
Contributor

@vladanpaunovic vladanpaunovic commented Aug 25, 2022

This PR adds Sentry integration to sentry-unplugin.

This will allow us to proactively monitor and get alerted about our customers errors.

Error page:

Screenshot 2022-08-25 at 02 31 49

Performance:

Screenshot 2022-08-25 at 02 20 54

Side note so I don't forget:
I would have expected to get distributed tracing when making requests to sentry API as we are sending sentry-trace. Let's discuss why this could happen. Different org maybe?

import "@sentry/tracing";

Sentry.init({
dsn: "https://1298ac2f87bc4aae99d9a9a94ce65b42@o447951.ingest.sentry.io/6681372",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this dns is intentionally not an env var. I decided to keep it here as this is a public info and we should be transparent about this.

Copy link

Choose a reason for hiding this comment

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

Agree!

@lforst
Copy link

lforst commented Aug 25, 2022

This PR is veeery cool. I love that we have tracing for the hooks 👌

I have to look at this in more detail tho: My concern right now is that if we use the top-level init instead of having a custom client, we'll pollute other sentry instances if they're running.

@lforst
Copy link

lforst commented Aug 25, 2022

Another thought. We should give users an option to turn this error reporting off. Especially during the build process, there are some things you don't necessarily want to leak (i.e. secrets etc.).

@lforst
Copy link

lforst commented Aug 25, 2022

I would have expected to get distributed tracing when making requests to sentry API as we are sending sentry-trace. Let's discuss why this could happen. Different org maybe?

Yeah that's it. You can have traces across projects but not across orgs afaik.

@Lms24
Copy link
Member

Lms24 commented Aug 25, 2022

That looks awesome @vladanpaunovic! The transaction looks just like I had imagined :D

Anyway, I'm +1 on making a custom client (let's try to have a minimal setup, no or only necessary integrations) and making this opt-out for users

@Lms24 Lms24 force-pushed the add-sentry-error-handling branch from 5f91271 to 809a576 Compare August 25, 2022 14:58
@lforst lforst mentioned this pull request Aug 25, 2022
29 tasks
@Lms24 Lms24 force-pushed the add-sentry-error-handling branch from 66de45c to 867fe26 Compare August 26, 2022 12:08
@Lms24 Lms24 requested a review from lforst August 26, 2022 12:08
@lforst lforst changed the title chore: add sentry feat: Add Sentry Aug 26, 2022
Comment on lines 132 to 134
} catch (e) {
// TODO: Maybe do some more sopthisticated error handling here
throw new Error(`Something went wrong while uploading file ${filename}`);
Sentry.captureException(e);
}
Copy link

@lforst lforst Aug 26, 2022

Choose a reason for hiding this comment

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

We should rethrow the errors happening here (and in the other api calls), otherwise they won't bubble up to the .catch in the post build hook.

@Lms24 Lms24 merged commit e5e0208 into main Aug 26, 2022
@Lms24 Lms24 deleted the add-sentry-error-handling branch August 26, 2022 15:00
lforst pushed a commit that referenced this pull request Aug 26, 2022
lforst pushed a commit that referenced this pull request Aug 26, 2022
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.

4 participants