Refactor Stack Parsing

[timfish]

Currently the JavaScript stack parsing is made up of:

• Highly modified version of Tracekit for the browser
• Mildly modified version of node-stack-parse for node.js

These are currently working well but there is scope for improvement:

• Multiple refactors have resulted in unused fields/code paths and unnecessary abstractions
• Downstream SDKs cannot configure the parser
  • In Electron we need to use both Chrome and node.js line parsers in the renderer (Missing stacktrace in renderer process when using Node.js API integration sentry-electron#308)
  • SDK users might want to exclude certain parsers to reduce bundle size

This quick PoC shows that the stack line parsers can be moved into individual functions that will allow features to be configurable.

Rough Plan

Make incremental improvements, starting with non-breaking changes:

• Refactor and improve tests and make some minor parser simplifications
  • ref(browser): Simplify TraceKit parser and reorganize tests #4514
• Remove unnecessary types and more simplifications
  • ref(browser): Replace TracekitStackFrame with Sentry StackFrame #4523
• Split line parsers into individual functions
  • ref(browser): Split stack line parsers into individual functions and simplify further #4555
• Migrate node-stack-parse to the common parser
  • Improve Node tests
    • ref(node): Increase stack parse test coverage #4564
  • Move context line fetching to an integration
    • ref(node): Refactor node source fetching into integration #3729
  • Update docs to deprecate frameContextLines
    • feat(nodejs): Deprecate frameContextLines option and move to integration sentry-docs#4746
  • Run node CI tests on Windows
    • ref(node): Run Node CI tests on Windows #4616
  • Refactor node parser to be line orientated and use common parser
    • ref(node): Refactor node stack parsing to use common parser #4612
• More stack parse improvements and consolidation
  • ref(stackparse): More stack parse improvements and consolidation #4669

Then breaking changes for next major version:

• Add stackParser to init options so it can be overridden in downstream SDKs
  • This will require modifications to exports in eventbuilder
  • ref: Expose configurable stack parser #4902
• Remove usages of deprecated event.stacktrace
  • Remove usage of deprecated event.stacktrace #4885
• Drop default Opera support since its not in the list of supported browsers
  • Remove stack parser support for Opera pre v15 #4923
• Remove frameContextLines from NodeOptions since this will now be passed to the integration
  • [v7] Remove deprecated frameContextLines #4884
• Remove eventbuilder exports which were at some point used in @sentry/electron
  • [v7] Remove eventbuilder exports which were historically used in @sentry/electron #4887

A stack parser that can handle both Chrome and Node.js frames might be constructed like:

import { createStackParser } from '@sentry/utils';
import { chrome } from '@sentry/browser';
import { node} from '@sentry/node';
import { Exception } from '@sentry.types';

const stackParser = createStackParser(chrome, node);

try {
  throw new Error('bad things');
} catch(e) {
  const ex: Exception = stackParser(e);
}

[timfish]

At some point this week I'm guessing #3729 is going to become a blocker to progress.

I've run the tests locally both on macOS and Windows and everything passes so I'm at a bit of a loss as to how to fix this!

[timfish]

When we finally allow users to pass in/configure stack line parsers we run into the issue of order/priority. The order that the line parsers are attempted is key so we need to add some kind of priority.

Currently the line parser type is:

type StackLineParser = (line: string) => StackFrame | undefined;

We could add a property to the function and change the type to something like:

interface StackLineParser {
  (line: string): StackFrame | undefined;
  priority: number;
}

You could configure this like:

const myParser: StackLineParser = Object.assign(
  line => {
    // parser here
  }, 
  { priority: 10 }
);

But that would add quite a bit to the bundle.

A helper function might make parser creation take up less bundle size:

function createParser(fn:  (line: string): StackFrame | undefined, priority: number): StackLineParser {
  return Object.assign(fn, { priority });
}

const myParser = createParser( line => { /* parser here */ }, 10);

Alternatively, a plain object interface is an option:

interface StackLineParser {
  fn: (line: string): StackFrame | undefined;
  p: number;
}

[AbhiPrasad]

Alternatively, a plain object interface is an option:

Could we maybe use an array? I used a similar strategy for the envelope types: https://github.com/getsentry/sentry-javascript/pull/4527/files#diff-98566cdbe00d44614c2fa27bf3e2ffabc236cf66528242495ea172d7fcb6ca0aR18-R25

[timfish]

That would work.

Those array type definitions are 👌

[timfish]

Trying to decide how to expose the stack parser config to Browser SDK consumers and the Electron SDK.

I can put it in the init options for the browser SDK, and then it'll need to be passed down to every function in eventbuilder. It's a bit messy but can't really think of a better way.

Do you see any point exposing the parser for other SDKs?

[AbhiPrasad]

Do you see any point exposing the parser for other SDKs?

React Native is the only one that comes to my mind here, but they seem to just use browser's stuff https://github.com/getsentry/sentry-react-native/blob/33232d9ef157e4c4059f618cee348f9bd16af8fd/src/js/backend.ts#L45-L61

[AbhiPrasad]

@timfish this should be good to close right?

[timfish]

Yes!