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

add sentry frontend error reporting & performance tracing #2452

Closed
wants to merge 2 commits into from

Conversation

syphar
Copy link
Member

@syphar syphar commented Mar 10, 2024

I believe since we're adding more and more frontend code, we should have error reporting & tracing too.

more info here: https://docs.sentry.io/platforms/javascript/

There are more ways to install the SDK, I don't know which is best for us. One alternative could be to bundle the SDK.

Also, in other projects the errors were quite noisy, especially coming from broken browser extensions, we will have to see if it's too much noise.

We will also have performance tracing, with combined backend / frontend transactions, plus metrics from the web vitals (https://docs.sentry.io/product/performance/web-vitals/ ).

Caveat: I'm not a frontend person, feel free to recommend any needed changes, my motivation is purely coming from wanting to have more error & performance monitoring

@syphar syphar requested a review from a team as a code owner March 10, 2024 08:30
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Mar 10, 2024
Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

Implementation looks fine. I wonder whether this is something that needs to be mentioned on the privacy policy, we appear to have sentry configured to strip IP, but still this will result in a connection to their cdn.

@syphar
Copy link
Member Author

syphar commented Mar 10, 2024

Implementation looks fine. I wonder whether this is something that needs to be mentioned on the privacy policy, we appear to have sentry configured to strip IP, but still this will result in a connection to their cdn.

You mean the request to fetch the SDK?
or the reporting generally?

I remember something that we just follow the crates.io privacy policy, which is also using sentry frontend side (at least for error reporting I saw it)

@Nemo157
Copy link
Member

Nemo157 commented Mar 10, 2024

Hmm, is crates.io bundling the sdk into their js? I did load it to check and didn't see any connection to the sentry cdn.

@syphar
Copy link
Member Author

syphar commented Mar 10, 2024

Hmm, is crates.io bundling the sdk into their js? I did load it to check and didn't see any connection to the sentry cdn.

yeah, probably just via NPM.

When errors happen there still would be a request to sentry.

@syphar
Copy link
Member Author

syphar commented Mar 10, 2024

we could also just vendor the file similar to the other JS frameworks

@Nemo157
Copy link
Member

Nemo157 commented Mar 10, 2024

Ah yes, inspecting the bundle I do see it embedded. We could do the same which would at least mean that there's no connection to load the sdk. There'd still be a connection for the sampled transactions though, unless we setup proxying for that.

It also doesn't look like crates.io is setting a dsn in production, so nothing ever gets sent.

@Nemo157
Copy link
Member

Nemo157 commented Mar 10, 2024

Ah, and that usage of sentry is mentioned explicitly in the crates.io section of the privacy policy, so I think we definitely should be adding it to the docs.rs section too for this https://foundation.rust-lang.org/policies/privacy-policy/#crates.io

@syphar
Copy link
Member Author

syphar commented Mar 10, 2024

Ah, and that usage of sentry is mentioned explicitly in the crates.io section of the privacy policy, so I think we definitely should be adding it to the docs.rs section too for this https://foundation.rust-lang.org/policies/privacy-policy/#crates.io

Yep, will do that. When I searched for the repo :)

Ah yes, inspecting the bundle I do see it embedded. We could do the same which would at least mean that there's no connection to load the sdk. There'd still be a connection for the sampled transactions though, unless we setup proxying for that.

I vendored the SDK now.

@syphar syphar requested a review from Nemo157 March 10, 2024 11:08
@GuillaumeGomez
Copy link
Member

Instead of imposing this extra JS code in the front-end for everyone, why not instead increase what the GUI tests cover (not much currently)?

@syphar
Copy link
Member Author

syphar commented Mar 11, 2024

Instead of imposing this extra JS code in the front-end for everyone, why not instead increase what the GUI tests cover (not much currently)?

I definitely see that the rust (and open source ) community in general is very concious about privacy, tracking, and in this case also frontend load.

I believe that in frontend code reporting errors and having test coverage are not exclusive and you actually need both, the same way that we still have error reporting in our backend code while we also have quite some tests.

Generally: test cover the things we expect, error reporting catches what we missed.
I also prefer knowing something is broken before a user has to report it. Not sure if the last claim is more coming from working on SaaS and how applicable this is necessarily to docs.rs.
And for me this extends to performance too, where I would like to know about the slow pieces in the app, and the user experience.

So still, happy to discuss further, perhaps we find common ground?

@GuillaumeGomez
Copy link
Member

I don't want to be a blocker if you think it's absolutely necessary, I'm just not a big fan having foreign JS to get potential user issues (which don't happen very rarely too) and adding even more code to all pages.

@syphar syphar closed this Oct 13, 2024
@syphar syphar deleted the sentry-frontend branch October 13, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants