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

Error event includes original error #246

Closed
wants to merge 4 commits into from
Closed

Conversation

@bjyoungblood
Copy link

bjyoungblood commented Nov 5, 2014

It would be nice for reporters to have access to the original error in case they want to do something with it.

In my case, I am writing a reporter for Rollbar, and their library requires the error object in order to do their own parsing.

@arb arb added the feature label Nov 5, 2014
@arb arb modified the milestones: 4.0.1, 4.1.0 Nov 5, 2014
@arb arb self-assigned this Nov 10, 2014
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 10, 2014

Because this could really impact downstream reporters I think you need to make this an option that defaults to false. Also, please add a test demonstrating that this works.

@@ -205,7 +205,8 @@ internals.Monitor.prototype._errorHandler = function (request, error) {
timestamp: request.info.received,
message: error.message,
stack: error.stack,
pid: process.pid
pid: process.pid,
originalError: error

This comment has been minimized.

Copy link
@arb

arb Nov 10, 2014

Contributor

Maybe rename to errorObject?

@bjyoungblood

This comment has been minimized.

Copy link
Author

bjyoungblood commented Nov 10, 2014

I've updated the PR with your suggestions.

I'm thinking for a future major release, it might be better to remove this option and have good-reporter filter out the errorObject by default. This works well enough for now though.

@bjyoungblood

This comment has been minimized.

Copy link
Author

bjyoungblood commented Nov 10, 2014

Rebased from master and fixed the test that was failing.

expect(eventsOne.length).to.equal(1);

expect(request.event).to.equal('error');
expect(request.errorObject).to.exist;

This comment has been minimized.

Copy link
@arb

arb Nov 10, 2014

Contributor

Please check the documentation for Code.expect here. Some of your assertions are no longer correct.

@@ -27,7 +27,8 @@ internals.defaults = {
requestsEvent: 'tail', // Sets the event used by the monitor to listen to finished requests. Other options: 'response'.
logRequestHeaders: false, // log all headers on request
logRequestPayload: false, // log payload of request
logResponsePayload: false // log payload of response
logResponsePayload: false, // log payload of response
logErrorObject: false, // send original error object to reporters

This comment has been minimized.

Copy link
@arb

arb Nov 10, 2014

Contributor

You'll also want to update that README explaining the new option.

@bjyoungblood

This comment has been minimized.

Copy link
Author

bjyoungblood commented Nov 15, 2014

Sorry about the delay in resolving these. I've been out of town.


server.start(function () {

var req = Http.request({

This comment has been minimized.

Copy link
@arb

arb Nov 19, 2014

Contributor

Just use a simple GET request here. A lot less code and a lot easier. All you really need to happen is the server to error. You can probably just do server.inject instead of starting up the whole HTTP stack.

@@ -208,6 +209,10 @@ internals.Monitor.prototype._errorHandler = function (request, error) {
pid: process.pid
};

if (this.settings.logErrorObject) {
event.errorObject = error;

This comment has been minimized.

Copy link
@arb

arb Nov 19, 2014

Contributor

Just call it error please to match the other properties.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 24, 2014

Closing due to inactivity.

@arb arb closed this Nov 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.