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

Request: Expose original (mangled) stacktrace when using --enable-source-maps #41131

Closed
lobsterkatie opened this issue Dec 10, 2021 · 12 comments
Closed
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@lobsterkatie
Copy link

lobsterkatie commented Dec 10, 2021

Is your feature request related to a problem? Please describe.

TL;DR: Using --enable-source-maps alongside a tool which also does sourcemapping causes conflicts.

(Full disclosure, I'm a maintainer of one such tool, the Sentry JS SDK.)

Background

Sourcemapping is in theory a deterministic process: all tools should produce the same demangled stacktrace. Therefore, at first glance, it doesn't make sense to have multiple tools sourcemapping your errors - if they're all going to get the same answer, what's the point? Just get the first tool (in this case, Node) to do the work and you're good.

The reality, though, is that while the result of two sourcemapping processes might be the same, the side effects can be totally different, and you might want both sets of side effects. (An example side effect: When Sentry is sourcemapping an error, it also harvests a few lines of original code around each stacktrace line, for better context, and stores them alongside the error.)

The problem is that in order to do sourcemapping you need the raw stacktrace, but if --enable-source-maps is on, Node only ever emits the sourcemapped one.

Use case

One of our customers hosts their site on Layer0. In order to be able to make their logs human-readable, Layer0 runs code with --enable-source-maps on. But then our SDK only gets the sourcemapped error, so it has nothing to process in order to harvest context lines. As a result, the customer gets much less useful stacktraces.

Additional use case

A commenter on another issue also wanted access to the raw stacktrace, in their case because they found sourcemap generation unreliable, and wanted to be able to see the original stacktrace in case their maps were broken and led to a garbled end result.

Describe the solution you'd like

The simplest solution would be for Error objects to have an optional rawStack property, which would hold the mangled stacktrace in cases where stack holds a sourcemapped one.

Describe alternatives you've considered

If adding a non-spec property to such a canonical class feels like too much, which I would understand, another option would be to make rawStack non-enumerable and add a getter method to the Error class. (Maybe that's no less blasphemous, IDK, but it feels one step more hidden than a publicly-accessible property.)

Or, if one didn't want to muck with the public API of the Error class at all, that getter method could instead be a utility function, accessing the same hidden property.

@lobsterkatie lobsterkatie added the feature request Issues that request new features to be added to Node.js. label Dec 10, 2021
@Trott
Copy link
Member

Trott commented Dec 10, 2021

@bcoe

@devmrfitz
Copy link

@lobsterkatie I'm facing the same issue. Has a workaround been found?

@lobsterkatie
Copy link
Author

@devmrfitz No, unfortunately. AFAICT, once the flag is enabled, there's no way to access the raw trace.

@bcoe
Copy link
Contributor

bcoe commented Feb 21, 2022

@lobsterkatie I agree that modifying the Error instance is a bit blasphemous. Because the stack trace API is defined in v8, it feels like it would be a bit weird Node.js sprinkling an additional stack trace related property on top of it (as you mention).

What if we went with your recommendation of a hidden property, but instead of a getter on the Error, there was a utility, e.g.,

import {getUnmappedStack} from 'errors';
unmappedStack = getUnmappedStack(someError); 

I think adding something like this for the benefit of Sentry and other telmetry libraries would be worthwhile.

Do you have any interest in contributing? It would be great to onboard another person to this part of the codebase, and would get things built faster.

@lobsterkatie
Copy link
Author

Sweet - thanks for being open to the idea!

I'm happy to contribute, but somewhat short on time as our team is stretched pretty thin at the moment. (Pause for shameless plug: Know any good senior JS devs? We are hiring!) I can try to give it a look this weekend or next, though.

@bcoe
Copy link
Contributor

bcoe commented Mar 11, 2022

@lobsterkatie I'm also swamped these days with job stuff unfortunately. Let me know if there's anything I can do to help get this in motion, unfortunately I'm not likely to have cycles soon.

@danielbankhead just a thought, this could be a good first Node.js contribution.

@danielbankhead
Copy link

Thanks @bcoe, I'm a bit swamped with work too - however, looking forward to making a proposal for easily importing the nearest package.json in a directory tree

@lobsterkatie
Copy link
Author

I'm definitely interested to work on this. Lemme at least look at it this weekend to see how complicated it looks. (In my head it's like five lines of code, which means in reality it's probably 500, LOL.) Once I have a sense of scale I'll have a better idea if/when I might be able to make some progress.

@bcoe
Copy link
Contributor

bcoe commented Mar 20, 2022

@lobsterkatie my guess is it's in the 50 line of code range. With contributing to Node.js, getting the environment up and running, and getting a feel for things, I find to usually be half the battle.

I think what you would do would be to basically:

  1. export a symbol here, which can be used as the hidden key: https://github.com/nodejs/node/blob/master/lib/internal/errors.js
  2. populate the original stack trace here, you'll find the logic you can call for the original prepare stack trace logic here.
  3. Add a method, I think util would be a good homef for it, getErrorUnmappedStack (_there already a couple helpers in util that perform operations with errors, e.g., util.getSystemErrorName.).

@lobsterkatie
Copy link
Author

With contributing to Node.js, getting the environment up and running, and getting a feel for things, I find to usually be half the battle.

Always is. 🙂

Thanks for the pointers! I'll let you know how it's going once I finish the setup process and get a chance to actually work on the code.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 26, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@targos targos moved this from Pending Triage to Stale in Node.js feature requests Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
Development

No branches or pull requests

5 participants