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

fix(node): Handle colons in stack trace paths #5517

Merged

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Aug 3, 2022

Closes #5151

So the regex doesn't just keep growing in complexity, this change drops the ability to parse stack frames that don't have column numbers. It's my understanding that modern versions of v8/node always have column numbers and this was left over for legacy support.

@AbhiPrasad
Copy link
Member

this change drops the ability to parse stack frames that don't have column numbers

Will this break in Node 8? Any way we can validate that?

Rest of the changes look great! Thanks for taking a look Tim!

@timfish
Copy link
Collaborator Author

timfish commented Aug 3, 2022

Will this break in Node 8? Any way we can validate that?

It looks like there were column numbers in stack lines as far back as Node v4 and probably even earlier, I just haven't looked back any further!

My only minor lingering concern is that there are specific cases where column numbers are not included. This seems highly unlikely!

@lobsterkatie
Copy link
Member

It sounds like a relatively safe change, given your research. I'm curious, though, what happens when stacktrace parsing fails?

@timfish
Copy link
Collaborator Author

timfish commented Aug 3, 2022

what happens when stacktrace parsing fails?

The lines are all considered independently. If it can't parse any lines you'll get zero frames for the stack trace.

Are you thinking about this?

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Aug 3, 2022

this change drops the ability to parse stack frames that don't have column numbers

Maybe we drop this for now and do it in v8 instead? We can just open a GH issue with the git diff and apply the patch when we start working on the major.

I think the concern is with the possibility of events with empty stacktraces.

We are tracking the major work in this issue - #5194

@AbhiPrasad
Copy link
Member

Actually scratch that - let's just do this, I'm confident that it'll be safe, and we can always revert if we need to.

@AbhiPrasad AbhiPrasad merged commit ce8d8f8 into getsentry:master Aug 3, 2022
@timfish timfish deleted the fix/node-stackparse-colon-in-path branch September 27, 2022 12:31
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.

[stacktrace parsing] Regex incorrectly truncating filenames with colons
3 participants