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

Use captureStackTrace to remove boom artifacts from stack traces #67

Closed
kpdecker opened this issue Sep 9, 2015 · 6 comments
Labels
Milestone

Comments

@kpdecker
Copy link
Contributor

@kpdecker kpdecker commented Sep 9, 2015

Using this https://code.google.com/p/v8-wiki/wiki/JavaScriptStackTraceApi the boom stack traces can safely remove their own frames so:

Debug: internal, implementation, error
    Error: child "text" fails because ["text" is required]
    at Object.exports.create (/Users/user/project/node_modules/boom/lib/index.js:21:17)
    at Object.exports.internal (/Users/user/project/node_modules/boom/lib/index.js:262:99)
    at Object.exports.badImplementation (/Users/user/project/node_modules/boom/lib/index.js:298:23)
    at postValidate (/Users/user/project/node_modules/hapi/lib/validation.js:194:26)
    at [object Object].internals.Any._validateWithOptions (/Users/user/project/node_modules/joi/lib/any.js:652:16)
    at [object Object].root.validate (/Users/user/project/node_modules/joi/lib/index.js:102:23)
    at exports.response (/Users/user/project/node_modules/hapi/lib/validation.js:213:20)
    at /Users/user/project/node_modules/hapi/lib/request.js:377:13
    at iterate (/Users/user/project/node_modules/hapi/node_modules/items/lib/index.js:35:13)
    at done (/Users/user/project/node_modules/hapi/node_modules/items/lib/index.js:27:25)
    at /Users/user/project/node_modules/hoek/lib/index.js:841:22
    at process._tickDomainCallback (node.js:381:11)

Could reduce to:

Debug: internal, implementation, error
    Error: child "text" fails because ["text" is required]
    at postValidate (/Users/user/project/node_modules/hapi/lib/validation.js:194:26)
    at [object Object].internals.Any._validateWithOptions (/Users/user/project/node_modules/joi/lib/any.js:652:16)
    at [object Object].root.validate (/Users/user/project/node_modules/joi/lib/index.js:102:23)
    at exports.response (/Users/user/project/node_modules/hapi/lib/validation.js:213:20)
    at /Users/user/project/node_modules/hapi/lib/request.js:377:13
    at iterate (/Users/user/project/node_modules/hapi/node_modules/items/lib/index.js:35:13)
    at done (/Users/user/project/node_modules/hapi/node_modules/items/lib/index.js:27:25)
    at /Users/user/project/node_modules/hoek/lib/index.js:841:22
    at process._tickDomainCallback (node.js:381:11)

Which helps remove noise from throws.

Potential downsides:

  • Only works under v8
  • Slight bit of additional complexity
  • Won't impact wrap API (Probably not a downside since it's BYO Error object so the user controls the trace already

General pattern is here:
https://github.com/chaijs/chai/blob/4e18d2a49394f21f49eaea97f556d6a17ecbcc7e/chai.js#L5015
https://github.com/wycats/handlebars.js/blob/master/lib/handlebars/exception.js

Glad to do the work to PR this if desired.

@hueniverse hueniverse added the request label Sep 9, 2015
@arb

This comment has been minimized.

Copy link
Contributor

@arb arb commented Sep 10, 2015

Makes sense to me. I don't think this module needs to care about anything that isn't V8 anyway. Will the result look the same if you use wrap or does captureStackTrace only filter the file(s) where you call from?

My only concern is what are the chances something useful could get filtered out?

@kanongil

This comment has been minimized.

Copy link
Member

@kanongil kanongil commented Sep 10, 2015

I'm doubtful about the usefulness of this. I like being pointed to exactly where the error was created. If you decide to go ahead, please bear in mind that reading the stack property is very slow in v8. For instance, I tested the handlebars implementation and it took 10x more time to generate than a plain Error.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Sep 10, 2015

I also like the full one but I can see the use case to remove it, if you do put it behind a flag or something?
Also I like perf :D

@kpdecker

This comment has been minimized.

Copy link
Contributor Author

@kpdecker kpdecker commented Sep 10, 2015

wrap: This shouldn't change. If it does, we did it wrong.

Regarding the usefulness, the whole intent of this API is to allow module developers to hide their internals. The only people who should care about the internals of boom are people working on boom and they can hack around that while debugging features. For everyone else showing the first frame as the first location that their code intended to throw vs. something an arbitrary number of frames down is very helpful for quickly debugging failures.

Regarding performance, I'll setup a before+after bench suite so we know what the impact may be on that front.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Sep 10, 2015

they can hack around that while debugging features does this imply an option of some sort or...?

kpdecker added a commit that referenced this issue Sep 15, 2015
@kpdecker

This comment has been minimized.

Copy link
Contributor Author

@kpdecker kpdecker commented Sep 15, 2015

This doesn't have much impact on performance when run in the context of the rest of the boom stack:

badRequest x 79,680 ops/sec ±1.93% (84 runs sampled)
trim x 63,649 ops/sec ±1.92% (87 runs sampled)
error x 222,179 ops/sec ±1.56% (89 runs sampled)
capture x 130,351 ops/sec ±1.13% (94 runs sampled)

Regarding hack around, I meant that the contributors to this project can comment out the line that has this effect if they are running into problems. The use case is rare enough and this library simple enough that the API overhead is not something that I would pursue but this would be a decision for @arb.

@arb arb modified the milestone: 2.9.0 Sep 22, 2015
@arb arb closed this in #68 Sep 22, 2015
@hueniverse hueniverse added feature and removed request labels Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.