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

Call captureStackTrace to filter boom from traces #68

Merged
merged 2 commits into from Sep 22, 2015

Conversation

@kpdecker
Copy link
Contributor

kpdecker commented Sep 15, 2015

Fixes #67


internals.internal = function (message, data, statusCode, ctor) {

var error = (data instanceof Error ? exports.wrap(data, statusCode, message) : internals.create(statusCode || 500, message, ctor));

This comment has been minimized.

Copy link
@arb

arb Sep 20, 2015

Contributor

Can you rewrite this as an if else? I find executing functions inside ternary operators a little hard to follow for the humans.

This comment has been minimized.

Copy link
@kpdecker

kpdecker Sep 21, 2015

Author Contributor

This is simply porting the existing code defined in the original method.

This comment has been minimized.

Copy link
@arb

arb Sep 21, 2015

Contributor

Understood, but since you're in there anyway... 🍰

This comment has been minimized.

Copy link
@kpdecker

kpdecker Sep 21, 2015

Author Contributor

Will update later tonight.

return internals.internal(message, data, statusCode, exports.internal);
};

internals.internal = function (message, data, statusCode, ctor) {

This comment has been minimized.

Copy link
@arb

arb Sep 20, 2015

Contributor

How about a more descriptive name? generate or build or something better that I can't think of.

This comment has been minimized.

Copy link
@kpdecker

kpdecker Sep 21, 2015

Author Contributor

This is the helper method to create internal errors. Could potentially call it createInternal or similar but on first look it seems like matching it to the external API of exports.internal makes more sense to keep parity with the API methods.

This comment has been minimized.

Copy link
@arb

arb Sep 21, 2015

Contributor

It's just not very descriptive of what it does. If someone wants to add another status code based boom type, they'll have to go looking to see what internals.internal does.

This comment has been minimized.

Copy link
@kpdecker

kpdecker Sep 21, 2015

Author Contributor

createInternal then?

This comment has been minimized.

Copy link
@arb

arb Sep 21, 2015

Contributor

👍

This comment has been minimized.

Copy link
@gergoerdosi

gergoerdosi Sep 21, 2015

Contributor

I think createInternal is still confusing. 5xx errors are server errors. The word "internal" is only used for the 500 status code.

This comment has been minimized.

Copy link
@gergoerdosi

gergoerdosi Sep 21, 2015

Contributor

Also, internals.internal() accepts arguments in different order than internals.create(). Is there a reason for that?

Since all calls to internals.internal() pass a status code, wouldn't be easier to just do everything in internals.create() by checking the status code?

@arb arb self-assigned this Sep 21, 2015
@arb arb added the feature label Sep 21, 2015
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 21, 2015

Do we want to expose something to make this behavior configurable?

@kpdecker

This comment has been minimized.

Copy link
Contributor Author

kpdecker commented Sep 21, 2015

Regarding making this configurable, what is the use case for consumers of boom (vs. authors of boom)? From the consumer perspective I can't think of a case where they should care about the internals. From the author perspective, you do loose some potential data but this seems to be a reasonable price considering the relatively limited internal complexity of the library and the benefit provided to users.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 21, 2015

Dropping the boom part of the stack makes sense to me. I don't think a setting it useful here.

@kpdecker

This comment has been minimized.

Copy link
Contributor Author

kpdecker commented Sep 22, 2015

Changed the internal.internal method to internal.serverError

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 22, 2015

This looks great. How do you think this should be versioned? My gut tells me major since we are changing the shape of the error stack and someone could be relying on the way it used to work.

Thoughts @hueniverse?

@kpdecker

This comment has been minimized.

Copy link
Contributor Author

kpdecker commented Sep 22, 2015

The error stack's content was never api, right? Major seems unnecessary.
Minor or patch would be a toss up in my mind but I could argue for either
one and feel like it's an ok choice.
On Tue, Sep 22, 2015 at 7:52 AM Adam Bretz notifications@github.com wrote:

This looks great. How do you think this should be versioned? My gut tells
me major since we are changing the shape of the error stack and someone
could be relying on the way it used to work.

Thoughts @hueniverse https://github.com/hueniverse?


Reply to this email directly or view it on GitHub
#68 (comment).

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Sep 22, 2015

I would also go with minor or patch, not part of "public api" and would be weird to rely on the fact that it is in there imo

@arb arb modified the milestone: 2.9.0 Sep 22, 2015
arb added a commit that referenced this pull request Sep 22, 2015
Call captureStackTrace to filter boom from traces
@arb arb merged commit 62f51e6 into master Sep 22, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@arb arb deleted the capture-stack branch Sep 22, 2015
@arb arb restored the capture-stack branch Sep 22, 2015
@arb arb deleted the capture-stack branch Sep 22, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 22, 2015

Minor.

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.