Refactor Stack Parsing

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:

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);
}

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!

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;
}

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

That would work.

Those array type definitions are 👌

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?

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

@timfish

@timfish this should be good to close right?

Yes!

Navigation Menu

Repository navigation

Refactor Stack Parsing #4543

Rough Plan

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Refactor Stack Parsing #4543

Description

Rough Plan

Activity

timfish commented on Feb 14, 2022

timfish commented on Feb 14, 2022

AbhiPrasad commented on Feb 14, 2022

timfish commented on Feb 14, 2022

timfish commented on Mar 2, 2022

AbhiPrasad commented on Mar 2, 2022

AbhiPrasad commented on Apr 26, 2022

timfish commented on Apr 26, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions