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

[RUMF-1310] handle extra stacktrace parsing cases #1647

Merged
merged 14 commits into from
Jul 27, 2022

Conversation

liywjl
Copy link
Contributor

@liywjl liywjl commented Jul 19, 2022

Motivation

Some users have reported cases in which we fail to parse the error stack in Chrome:

#957
#1592

Changes

After some digging, it appears there are 2 Chrome cases we do not handle:

  • closureScripe logs file URL differently (only displaying the filename and not the path)
  • anonymous function in some cases Chrome seems to ignore the text that indicates the stack was called by an anonymous function

Solution

  • closureScripe: match URLs that match a filename without prefix
    Add \w+\. to URL regex

  • anonymous function: ignore regex trying to identify function
    Remove (.*?) from regex, and create new parse function

Testing

Add unit tests to handle cases

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2022

Codecov Report

Merging #1647 (64caa73) into main (a818fb4) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1647      +/-   ##
==========================================
- Coverage   90.74%   90.71%   -0.03%     
==========================================
  Files         128      128              
  Lines        4687     4697      +10     
  Branches     1053     1056       +3     
==========================================
+ Hits         4253     4261       +8     
- Misses        434      436       +2     
Impacted Files Coverage Δ
...ages/core/src/domain/tracekit/computeStackTrace.ts 94.36% <100.00%> (-4.00%) ⬇️
packages/core/src/tools/timeUtils.ts 100.00% <0.00%> (+2.70%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@liywjl liywjl marked this pull request as ready for review July 26, 2022 09:56
@liywjl liywjl requested a review from a team as a code owner July 26, 2022 09:56
packages/core/src/domain/tracekit/computeStackTrace.ts Outdated Show resolved Hide resolved
// eslint-disable-next-line max-len
/^\s*at (.*?) ?\(((?:file|https?|blob|chrome-extension|native|eval|webpack|<anonymous>|\/).*?)(?::(\d+))?(?::(\d+))?\)?\s*$/i
const CHROME_EVAL_RE = /\((\S*)(?::(\d+))(?::(\d+))\)/
// eslint-disable-next-line local-rules/disallow-side-effects
Copy link
Member

Choose a reason for hiding this comment

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

🔨 warning: ‏You should not disable this rule, as it guarantees that our code will stay mostly tree-shakable. In this case, we have two possibilities:

  • allow new RegExp by changing the rule here
  • build the RE in a function, like:
let chromeLineRe: RegExp | undefined
function getChromeLineRe() {
  if (!chromeLineRe) chromeLineRe = new RegExp(...)
  return chromeLineRe
}

We know that new RegExp is side-effect free, so the first solution could be good. One small drawback is that Webpack+Terser fails to treeshake regex constructed with variables like you do. This can be observed like that:

$ cat src/index.js
const a = new RegExp("foo");

const bar = "bar";
const b = new RegExp(`foo${bar}`);

$ npx --package webpack-cli webpack --mode production
assets by status 21 bytes [cached] 1 asset
./src/index.js 84 bytes [built] [code generated]
webpack 5.72.0 compiled successfully in 99 ms

$ cat dist/main.js
new RegExp("foobar");

a has been treeshaken in the resulting file, but b not entirely.

As this is a minor issue, and Terser might be improved someday, I suggest changing the rule nonetheless. (Note that rollup does not have this issue 🎉 )

@liywjl liywjl requested a review from BenoitZugmeyer July 26, 2022 13:54
@liywjl liywjl merged commit 1a5ada9 into main Jul 27, 2022
@liywjl liywjl deleted the william/extend-chrome-stack-parsing-coverage branch July 27, 2022 13:02
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.

5 participants