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(browser): Add ContextLines integration for html-embedded JS stack frames #8670

Merged
merged 7 commits into from
Jul 31, 2023

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jul 28, 2023

To get source context around browser-side stack frames, the Sentry backend tries to download the JS files during ingestion/symbolication and applies the context lines then. However, sometimes our backend cannot access the requested HTML (or gets a different version of it) e.g. due to missing authentication credentials. Therefore, this PR adds a new browser integration - ContextLines.

It can be used to add source code lines to and around stack frames that point towards JS in html files (e.g. in <script> tags or onclick handlers). The integration does not apply these context lines to frames pointing to actual script files as these cannot be accessed within the browser.

Limitations:

  • Firefox: Errors from inline JS (e.g. in onclick handlers) have the wrong line number, as the line number of the error isn't relative to the entire HTML document but to the isolated script.
  • Webkit: Seems like some inline html JS errors are simply thrown as "Script error." which we filter out in InboundFilters. These errors continue to not get reported to Sentry by default.
  • Pretty sure we might be off by one or two lines in either direction, depending on how the browser formats documentElement.innerHtml. Using this instead of outerHtml and adding tags manually seems to have improved stability in my testing but I'm not sure if it does the trick for every browser.

Given the limited use case and the additional bundle size costs, we're not going to enable this integration by default but we'll document it and let users opt in to use it. Hence, the package is also exported from @sentry/integrations instead of @sentry/browser and it won't be part of the browser CDN bundles.

closes #8656

@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.07 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.4 KB (0%)
@sentry/browser - Webpack (gzipped) 21.95 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 69.79 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.28 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.33 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 220.89 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 85.99 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 60.48 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 30.41 KB (+0.01% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 65.14 KB (0%)
@sentry/react - Webpack (gzipped) 21.96 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 92.91 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 50.89 KB (0%)

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

nice one!

@Lms24 Lms24 force-pushed the lms/feat-browser-context-lines branch from cb37c44 to 55531d7 Compare July 31, 2023 08:52
@Lms24 Lms24 self-assigned this Jul 31, 2023
@Lms24 Lms24 force-pushed the lms/feat-browser-context-lines branch from 55531d7 to 5bea408 Compare July 31, 2023 09:13
@Lms24 Lms24 marked this pull request as ready for review July 31, 2023 09:46
@Lms24
Copy link
Member Author

Lms24 commented Jul 31, 2023

Hmm I'm realizing that we should move this to @sentry/integrations because right now it'd be included by default in the CDN bundles. Instead it should be its own CDN bundle that users can add to their setup if they're using our CDN/loader.

@mydea
Copy link
Member

mydea commented Jul 31, 2023

Hmm I'm realizing that we should move this to @sentry/integrations because right now it'd be included by default in the CDN bundles. Instead it should be its own CDN bundle that users can add to their setup if they're using our CDN/loader.

ahh, makes sense! That's fine I'd say 👍

@Lms24 Lms24 force-pushed the lms/feat-browser-context-lines branch from 069cfec to cbfaa35 Compare July 31, 2023 11:26
@Lms24 Lms24 merged commit ab3515c into develop Jul 31, 2023
80 checks passed
@Lms24 Lms24 deleted the lms/feat-browser-context-lines branch July 31, 2023 12:12
const html = doc.documentElement.innerHTML;

const htmlLines = ['<!DOCTYPE html>', '<html>', ...html.split('\n'), '</html>'];
if (!htmlLines.length) {
Copy link
Member

Choose a reason for hiding this comment

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

m: This is always gonna evaluate to false if we have these default values in htmlLines.

Bringing be to a different point: Why do we have these default values? Would it make sense to use document.documentElement.outerHTML instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I'll adjust the length check!

I started with outerHTML but for some reason, the resulting string is formatted differently (less line breaks) as when using innerHTML.

@lforst
Copy link
Member

lforst commented Jul 31, 2023

Or even new XMLSerializer().serializeToString(document) like https://stackoverflow.com/a/35917295/21967320 suggests. That even grabs the <!DOCTYPE html> in front.

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.

Better stacktrace data for non-file routes behind login wall
3 participants