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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
59a0b3b
[RUMF-1310] handle extra stacktrace parsing cases
Jul 19, 2022
f8bed8d
rename test mock data
Jul 19, 2022
d77b3f2
Merge branch 'main' into william/extend-chrome-stack-parsing-coverage
liywjl Jul 19, 2022
5c82d5f
make parse regex more generic and not clojurescript specific
Jul 20, 2022
ec92931
compose chrome regex into strings
Jul 26, 2022
c98d128
update unit test description and edge cases
Jul 26, 2022
48e6d91
Merge branch 'main' into william/extend-chrome-stack-parsing-coverage
liywjl Jul 26, 2022
022fa29
Update packages/core/src/domain/tracekit/computeStackTrace.ts
liywjl Jul 26, 2022
e165ce0
remove disallow-side-effects comment and add exception for RegExp
Jul 26, 2022
1b71e03
format disallowSideEffects prettier
Jul 26, 2022
de7b9ab
add prefix file to variable names
Jul 26, 2022
64caa73
Merge branch 'main' into william/extend-chrome-stack-parsing-coverage
liywjl Jul 26, 2022
19f4414
Merge branch 'main' into william/extend-chrome-stack-parsing-coverage
BenoitZugmeyer Jul 26, 2022
bc29a36
Merge branch 'main' into william/extend-chrome-stack-parsing-coverage
liywjl Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions packages/core/src/domain/tracekit/computeStackTrace.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,60 @@ Error: foo
})
})

it('should parse Chrome error that only contain file name, with no path prefix', () => {
const stack = `Error: RTE Simulation
at foo$bar$oof$rab (events.cljs:1060:12)
at func1$func2$func3$func4 (std_interceptors.js:128:19)
at eval (std_interceptors.jsx:132:29)`

const stackFrames = computeStackTrace({ stack } as Error)
expect(stackFrames.stack.length).toEqual(3)
expect(stackFrames.stack[0]).toEqual({
args: [],
column: 12,
func: 'foo$bar$oof$rab',
line: 1060,
url: 'events.cljs',
})
expect(stackFrames.stack[1]).toEqual({
args: [],
column: 19,
func: 'func1$func2$func3$func4',
line: 128,
url: 'std_interceptors.js',
})
expect(stackFrames.stack[2]).toEqual({
args: [],
column: 29,
func: 'eval',
line: 132,
url: 'std_interceptors.jsx',
})
})

it('should parse Chrome anonymous function errors', () => {
const stack = `Error: RTE Simulation
at https://datadoghq.com/somefile.js:8489:191
at chrome-extension://<id>/content/index.js:85:37379`

const stackFrames = computeStackTrace({ stack } as Error)
expect(stackFrames.stack.length).toEqual(2)
expect(stackFrames.stack[0]).toEqual({
args: [],
column: 191,
func: '?',
line: 8489,
url: 'https://datadoghq.com/somefile.js',
})
expect(stackFrames.stack[1]).toEqual({
args: [],
column: 37379,
func: '?',
line: 85,
url: 'chrome-extension://<id>/content/index.js',
})
})

it('should parse Chrome 15 error', () => {
const stackFrames = computeStackTrace(CapturedExceptions.CHROME_15 as any)

Expand Down
34 changes: 29 additions & 5 deletions packages/core/src/domain/tracekit/computeStackTrace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ export function computeStackTrace(ex: unknown): StackTrace {
const stackProperty = tryToGetString(ex, 'stack')
if (stackProperty) {
stackProperty.split('\n').forEach((line) => {
const stackFrame = parseChromeLine(line) || parseWinLine(line) || parseGeckoLine(line)
const stackFrame =
parseChromeLine(line) || parseChromeAnonymousLine(line) || parseWinLine(line) || parseGeckoLine(line)
if (stackFrame) {
if (!stackFrame.func && stackFrame.line) {
stackFrame.func = UNKNOWN_FUNCTION
Expand All @@ -28,11 +29,14 @@ export function computeStackTrace(ex: unknown): StackTrace {
stack,
}
}

const url = '((?:file|https?|blob|chrome-extension|native|eval|webpack|<anonymous>|\\w+\\.|\\/).*?)'
const position = '(?::(\\d+))'
liywjl marked this conversation as resolved.
Show resolved Hide resolved
const CHROME_LINE_RE =
// 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 🎉 )

new RegExp(`^\\s*at (.*?) ?\\(${url}${position}?${position}?\\)?\\s*$`, 'i')

// eslint-disable-next-line local-rules/disallow-side-effects
const CHROME_EVAL_RE = new RegExp(`\\((\\S*)${position}${position}\\)`)

function parseChromeLine(line: string): StackFrame | undefined {
const parts = CHROME_LINE_RE.exec(line)
Expand Down Expand Up @@ -61,6 +65,26 @@ function parseChromeLine(line: string): StackFrame | undefined {
}
}

const CHROME_ANONYMOUS_FUNCTION_RE =
// eslint-disable-next-line local-rules/disallow-side-effects
new RegExp(`^\\s*at ?${url}${position}?${position}??\\s*$`, 'i')

function parseChromeAnonymousLine(line: string): StackFrame | undefined {
const parts = CHROME_ANONYMOUS_FUNCTION_RE.exec(line)

if (!parts) {
return
}

return {
args: [],
column: parts[3] ? +parts[3] : undefined,
func: UNKNOWN_FUNCTION,
line: parts[2] ? +parts[2] : undefined,
url: parts[1] ?? undefined,
liywjl marked this conversation as resolved.
Show resolved Hide resolved
}
}

const WINJS_LINE_RE =
/^\s*at (?:((?:\[object object\])?.+) )?\(?((?:file|ms-appx|https?|webpack|blob):.*?):(\d+)(?::(\d+))?\)?\s*$/i

Expand Down