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

fix: Improve Error Casting #1014

Merged
merged 8 commits into from
May 7, 2024
Merged

fix: Improve Error Casting #1014

merged 8 commits into from
May 7, 2024

Conversation

metal-messiah
Copy link
Member

@metal-messiah metal-messiah commented May 3, 2024

Improve detection of line and column to allow errors that throw without a stack -- such as some chrome errors -- to still get basic error location information

Overview

The primary goal of this PR was to address an issue where certain browsers (Chrome) can emit errors without a stack property under certain circumstances. These errors are emitted with a line and column, but the previous implementation of the agent blindly ignored that data if the error was an instance of the Error class. This PR:

  • Checks if the error has a stack before accepting it blindly
  • Centralizes the error casting behavior
  • Removes the need for intercepting fn-err messages for SPA decoration by diverting that behavior to the SPA instrument class
    • This was necessary to keep in the agent because function errors can propagate through the agent faster than the event handler can, including errors thrown in the createTracer API, and if we rely solely on the errorEvent, these errors can actually throw after the SPA interaction has already closed. The agent will decorate the errors directly in the SPA feature now, so that by the time they are seen in the error feature, they already have interaction information tied to them.
  • Removes the need to de-dupe in the error instrument class since fn-err and errorEvent no longer clash.

Related Issue(s)

NR-262956

Testing

New unit & wdio tests have been added

Copy link

github-actions bot commented May 3, 2024

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent Asset Previous Size New Size Diff
lite loader 31.38 kB / 10.99 kB (gzip) 31.42 kB / 11 kB (gzip) 0.13% / 0.1% (gzip)
lite async-chunk 50.99 kB / 16.59 kB (gzip) 50.99 kB / 16.59 kB (gzip) 0% / 0% (gzip)
pro loader 52.11 kB / 17.68 kB (gzip) 51.97 kB / 17.67 kB (gzip) -0.27% / -0.08% (gzip)
pro async-chunk 93.54 kB / 28.48 kB (gzip) 93.72 kB / 28.52 kB (gzip) 0.19% / 0.13% (gzip)
spa loader 60.02 kB / 20.1 kB (gzip) 60.08 kB / 20.15 kB (gzip) 0.1% / 0.26% (gzip)
spa async-chunk 108.54 kB / 32.87 kB (gzip) 108.99 kB / 32.95 kB (gzip) 0.41% / 0.24% (gzip)
lite-polyfills loader 125.19 kB / 40.25 kB (gzip) 125.23 kB / 40.26 kB (gzip) 0.03% / 0.02% (gzip)
lite-polyfills async-chunk 65.79 kB / 18.97 kB (gzip) 65.79 kB / 18.97 kB (gzip) 0% / 0% (gzip)
pro-polyfills loader 147.25 kB / 46.57 kB (gzip) 146.85 kB / 46.48 kB (gzip) -0.28% / -0.21% (gzip)
pro-polyfills async-chunk 127.92 kB / 33.39 kB (gzip) 128.12 kB / 33.43 kB (gzip) 0.15% / 0.11% (gzip)
spa-polyfills loader 155.24 kB / 48.68 kB (gzip) 155.04 kB / 48.66 kB (gzip) -0.12% / -0.04% (gzip)
spa-polyfills async-chunk 144.64 kB / 37.99 kB (gzip) 145.12 kB / 38.08 kB (gzip) 0.33% / 0.24% (gzip)

Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 87.06%. Comparing base (44f229e) to head (def5525).
Report is 3 commits behind head on main.

Files Patch % Lines
src/features/jserrors/instrument/index.js 33.33% 2 Missing ⚠️
src/features/jserrors/shared/cast-error.js 94.87% 2 Missing ⚠️
src/features/jserrors/aggregate/index.js 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1014      +/-   ##
==========================================
+ Coverage   86.64%   87.06%   +0.42%     
==========================================
  Files         154      155       +1     
  Lines        7126     7122       -4     
  Branches     1414     1434      +20     
==========================================
+ Hits         6174     6201      +27     
+ Misses        829      806      -23     
+ Partials      123      115       -8     
Flag Coverage Δ
integration-tests 90.82% <80.64%> (+0.09%) ⬆️
unit-tests 74.80% <85.10%> (+0.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented May 3, 2024

Static Badge

Last ran on May 07, 2024 11:46:07 CDT
Checking merge of (def5525) into main (44f229e)

@metal-messiah metal-messiah marked this pull request as ready for review May 7, 2024 02:18
Copy link
Contributor

@patrickhousley patrickhousley left a comment

Choose a reason for hiding this comment

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

Looks good. I just have the one question about how our decoration of the error object could clash with multiple agents running on the same page.

@metal-messiah metal-messiah merged commit d1dd20c into main May 7, 2024
51 checks passed
@metal-messiah metal-messiah deleted the improve-errors-casting branch May 7, 2024 18:49
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