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

Feature request: improve regex in stack trace parsing #2121

Closed
1 of 2 tasks
dreamorosi opened this issue Feb 21, 2024 · 5 comments · Fixed by #2194
Closed
1 of 2 tasks

Feature request: improve regex in stack trace parsing #2121

dreamorosi opened this issue Feb 21, 2024 · 5 comments · Fixed by #2194
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility good-first-issue Something that is suitable for those who want to start contributing logger This item relates to the Logger Utility

Comments

@dreamorosi
Copy link
Contributor

Use case

When formatting an error using the Logger utility I want to get the location of the error in my logs. To do so, the Logger utility parses the stack trace of an error and extracts the relevant info.

For example, the following stack trace:

'Error: Things keep happening!
   at /home/foo/bar/file-that-threw-the-error.ts:22:5
   at SomeOther.function (/home/foo/bar/some-file.ts:154:19)'

yields this location: /home/foo/bar/file-that-threw-the-error.ts:22:5.

At the moment we are using a regex expression (/\((.*):(\d+):(\d+)\)\\?$/) that has a couple of potential issues that could lead to unexpected behavior:

  • Greedy Quantifier: The .* part of the regex uses a greedy quantifier. This means it will match as many characters as possible, which could lead to unexpected results if there are multiple colons in the string. For example, in the string "(foo:bar:1:2)", the .* part will match "foo:bar", not just "foo".
  • Escaping Backslash: The \\? part of the regex matches an optional backslash at the end of the string. However, if since we're trying to match a single backslash, we only need to escape it once, not twice.

Solution/User Experience

The API/DX for the logger should remain the same and the change should be backwards compatible.

We should however improve the expression and change it to /\((.*?):(\d+):(\d+)\)\$?$/, this would:

  • Remove the greedy quantifier: .* -> .*?
  • Correct the escaping of the backslash: \\?$ -> \?$

Additionally, we should review the unit tests of the method associated with this expression and make sure they're exhaustive enough.

Alternative solutions

No response

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@dreamorosi dreamorosi added good-first-issue Something that is suitable for those who want to start contributing help-wanted We would really appreciate some support from community for this one logger This item relates to the Logger Utility feature-request This item refers to a feature request for an existing or new utility confirmed The scope is clear, ready for implementation labels Feb 21, 2024
@karthikeyanjp
Copy link
Contributor

I would like to contribute. Will submit a PR with the regex and unit tests.

@dreamorosi
Copy link
Contributor Author

Hi @karthikeyanjp, thank you for reaching out.

I'll assign the issue to you and help merge the PR once ready.

@dreamorosi dreamorosi removed the help-wanted We would really appreciate some support from community for this one label Mar 9, 2024
@karthikeyanjp
Copy link
Contributor

karthikeyanjp commented Mar 9, 2024

Referring to format of error locations as mentioned in https://nodejs.org/api/errors.html#errorstack & https://v8.dev/docs/stack-trace-api , we can support the following pattern:

  1. plain-filename.js:line:column or /absolute/path/to/file.js:line:column or <transport-protocol>:///url/to/module/file.mjs:line:column wrapped in ().
  2. location with multiple colon. Example at Script.runInThisContext (node:vm:130:12)
  3. location from an eval statement.
    Example:
Error: unknown:
at eval (eval at <anonymous> (file:///home/foo/bar/some-file.ts:1:35), <anonymous>:1:7)
at <anonymous> (/home/foo/bar/file-that-threw-the-error.ts:52:3)

For pattern (3), the current & above proposed regex will extact eval at <anonymous> (file:///home/foo/bar/some-file.ts:1:35), <anonymous>:1) instead of /home/foo/bar/file-that-threw-the-error.ts:52: as there are two nested locations embedded within ().

I am proposing the regex /\(([^)]*?):(\d+):(\d+)\)\\?$/ . I tested and it works for all the cases.

Submitted PR #2194 .

@dreamorosi please let me know your thoughts on the new regex.

@dreamorosi dreamorosi linked a pull request Mar 11, 2024 that will close this issue
9 tasks
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Mar 11, 2024
Copy link
Contributor

This is now released under v2.0.3 version!

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility good-first-issue Something that is suitable for those who want to start contributing logger This item relates to the Logger Utility
Projects
Development

Successfully merging a pull request may close this issue.

2 participants