Fix request on error logger

[natanavra]

The err param in onResFinished(err) was always undefined no matter what the result of the request was.

From nodejs / express documentation, there is no event named error:

• https://expressjs.com/en/4x/api.html#res
• https://nodejs.org/api/http.html#http_class_http_serverresponse

The res object can be used as a the err holder, and set on the res object in the express error handler middleware

function onErrorMiddleware(err, req, res, next) {
    res.err = err;
    //code
}

If no err object on the response but a bad statusCode, we generate the error from the logger. I know this would show a somewhat confusing stack, but it's something.
Besides the stdSerializers.err handles string error as an array of chars.

[mcollina]

Good catch! Can you add a unit test for this?

[mcollina]

Thinking about it some more, I think we have to support both things, as other frameworks might emit errors as 'error' events, can you please maintain the previous functionality while adding this one?

[natanavra]

I was pretty sure this is an express only package, since I came here from express-pino-logger

[mcollina]

LGTM

[mcollina]

I think this should be >= 500, not 400.

[natanavra]

I think that you'd want to have logs of issues like: bad request, unauthorized, forbidden access

And besides, we want to log the request / response, and a 4xx is a request (client) error.

[mcollina]

Non of them are errors.
This is a flow for error conditions.

@jsumners what do you think?

[natanavra]

One more point on my side, if I'd look at a logs dashboard, I'd be looking to see 4xx responses as errors. These can indicate a broken client side library, broken flow somewhere, etc.

[jsumners]

Looks like it got resolved before I came around. I'm fine with the current solution.

[mcollina]

I’m very opposed to land this in the current form.

I’m also ok in removing the check on the status code (as it was before).

[natanavra]

Basically, we can leave it up to the user to set the err on the res, that would probably still result in 4xx errors being logged, as I guess most developers would call next(err4xx);.

[mcollina]

I think so, yes.

[natanavra]

Modified. Not sure it notified you, as the commit and your message happened pretty much the same time... so I'm sending a message.

[jsumners]

LGTM

[mcollina]

LGTM