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

Merged
merged 13 commits into from
Aug 4, 2023

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Aug 1, 2023

Well here we go again 😅

This PR adds a "best-effort" ContextLines integration as an optional integration for the browser SDKs to pick up source code of and around stack frames pointing to code that's directly embedded in the current page's html.

As outlined in #8656 (comment) and my initial attempt (#8670), there are a number of limitations around this integration. Chances are users end up with context lines off by one or two (and sometimes off by a lot of lines). However, this approach should bring us on par with competitors and hopefully provide some more context around errors that previously didn't have any code in their stack traces.

for the time being, this

closes #8656

@Lms24 Lms24 force-pushed the lms/feat-browser-context-lines branch from de565e8 to 05b450b Compare August 1, 2023 15:02
@Lms24 Lms24 requested review from a team, stephanie-anderson, ale-cota, lforst and AbhiPrasad and removed request for a team, stephanie-anderson and ale-cota August 1, 2023 15:03

/** Processes an event and adds context lines */
public addSourceContext(event: Event): Event {
const doc = WINDOW.document;
Copy link
Member

Choose a reason for hiding this comment

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

m: should only run if the current hub has the ContextLines integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm good point. I guess the Node ContextLines integration should have this check as well, right? Seems like we do it in some integrations and in others it's missing.

Copy link
Member

Choose a reason for hiding this comment

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

yeah we need to include the check there too

packages/integrations/src/contextlines.ts Show resolved Hide resolved
packages/integrations/src/contextlines.ts Outdated Show resolved Hide resolved
@Lms24
Copy link
Member Author

Lms24 commented Aug 2, 2023

I made some more changes to the new integration after re-checking the Node ContextLines integration to further align code and behaviour

  • By default, now 7 lines of context are added in both directions (previously, it was 3 per direction (floor(7 / 2)))
  • Reused the utils function addContextToFrame as with the change above, behaviour was basically identical
  • Adjusted tests to addContextToFrame output (mostly identical but some smaller differences around empty pre/post contexts and edge case handling).

@Lms24 Lms24 changed the title feat(browser): Add ContextLines integration for html-embedded JS stack frames feat(integrations): Add ContextLines integration for html-embedded JS stack frames Aug 4, 2023
@Lms24 Lms24 merged commit 436fdeb into develop Aug 4, 2023
79 checks passed
@Lms24 Lms24 deleted the lms/feat-browser-context-lines branch August 4, 2023 11:14
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
2 participants