SentryParser counts Error message lines in `error.framesToPop`

[krystofwoldrich]

Capture an error with framesToPop property set to 1.

Changing this would be a breaking change and could affect issues grouping.

Expected Result

The top frame will be removed. We skip parsed frames instead of stack lines.

Actual Result

Nothing is removed. The skipped line is an error message https://github.com/getsentry/sentry-javascript/blob/739d904342aaf9327312f409952f14ceff4ae1ab/packages/utils/src/stacktrace.ts#L23

[AbhiPrasad]

Oh shoot - good catch cc @timfish

[timfish]

I'm just trying to understand what's not working as expected here.

The skipped line is an error message

skipFirst is specifically used to skip the error message.

The logic to handle framesToPop is here: https://github.com/getsentry/sentry-javascript/blob/59db749cf4d1ad2165819e571fbc42945aea0f4c/packages/browser/src/eventbuilder.ts#L91-L127

We've got a couple of tests that use framesToPop, are these wrong or incomplete? https://github.com/getsentry/sentry-javascript/blob/59db749cf4d1ad2165819e571fbc42945aea0f4c/packages/browser/test/unit/tracekit/react.test.ts#L4-L8

[krystofwoldrich]

@timfish The skipFirst is now used to skip the first x lines of the error stack string which is true, but I believe it should not skip the lines of the error stack string but actual parsed frames.

The value passed to the skipFirst is framesToPop, if set to one I would expect the first frame to be skipped but all frames are present because only the message from the stack string is skipped.

https://github.com/getsentry/sentry-javascript/blob/59db749cf4d1ad2165819e571fbc42945aea0f4c/packages/browser/test/unit/tracekit/react.test.ts#L4-L8

The tests I believe are wrong as they expect no frames to be skipped when framesToPop is set to 1.

Skipping the lines of the string is unpredictable as the message can have multiple lines.

[timfish]

@krystofwoldrich many thanks for the detailed explanation and bearing with me 🙂

[timfish]

It looks like the framesToPop property on the error doesn't make a difference with the current parser.

If we look waaaay back when this test was added: https://github.com/getsentry/sentry-javascript/commit/2979b2c

And browser the source at that point: https://github.com/getsentry/sentry-javascript/tree/2979b2c5b09a80980a8765272dde246564c82dfd

We can see that we used tracekit virtually unmodified from when it was forked and I can see that at that point it did parse the frames first and then pop a frame: https://github.com/getsentry/sentry-javascript/blob/2979b2c5b09a80980a8765272dde246564c82dfd/packages/browser/src/tracekit.ts#L880-L883

But even back then, 4 frames were expected in the test case: https://github.com/getsentry/sentry-javascript/blob/2979b2c5b09a80980a8765272dde246564c82dfd/packages/browser/test/unit/tracekit/custom.test.ts#L305-L336

I think the current stack line parser regex is stricter than back in 2019 and since we drop lines that could not be parsed, this automatically strips the Invariant Violation: Minified React error line from the frames. This is why the tests continued to pass after many modifications and updates to how stack parsing works. We could (and probably should) strip out all the framesToPop code and the parser tests would all continue to pass.

Do you have an example stack string where the parsing isn't working as expected? We currently have a single test case that hasn't been modified in 4 years and has continued to pass.

[krystofwoldrich]

@timfish I've opened a PR as it might be easier to resolve this than talking theory.

• https://github.com/getsentry/sentry-javascript/pull/10560

I believe there were two things mixed from the beginning.

1. Skipping a line of the stack string which otherwise would be parsed as a valid frame although it's not a frame.

   • Example: Minified React error #31; visit https://reactjs.org/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7B%7D&args[]= for

2. Skipping the actual frame, React uses invariant.js which sets framesToPop=1 to remove the invariant frame as this will be the same in all the errors.