Add raw error to errorLogger?

[rosston]

I'd like to put the raw error/exception object on the meta data that gets logged. I'd be happy to make a PR with the feature, but I feel like it can be done in a couple different ways.

Here's my problem:

Mongoose's ValidationError stores the specifics of each individual validation error within an errors property on the exception. However, errorLogger (using winston.exception.getAllInfo) only extracts the stack trace from the exception. This makes any error logs with a ValidationError difficult to work with since it's not possible to know what field is causing the error.

So, I can see two useful ways of adding the raw error/exception to the meta data:

1. Just add a rawError (or similar) field to the meta data that gets included on every logged error
2. Accept a function on the baseMeta property (or some other one?) that the user can provide to add custom fields that would take precedence over the default meta data fields. That gives a lot of flexibility (too much? right amount?), like this:

app.use(expressWinston.errorLogger({
  transports: [
    new winston.transports.Console({})
  ],
  <newMetaPropertyHandler>: function(err) {
    return {
      errors: err.errors,
      foo: err.foo,
      stack: err.mySpecialStackTrace // This will take precedence over the default `stack` property
    };
  }
}));

Like I said, I'd be happy to add a PR (with tests!), but I didn't want to go making changes when it seems like there are other valid options. So any feedback would be appreciated!

[rosston]

I don't have a PR ready for this yet, but here's what I plan to do:

• Add an errorWhitelist that works just like the requestWhitelist and responseWhitelist
• Add an errorFilter to the options that is used just like the requestFilter and responseFilter
• Put the result of the errorWhitelist properties being run through the errorFilter on an error property on the metadata object

[floatingLomas]

Alternatively, we could approach this by being able to supply an 'error transformer' function that would just transform a received error however anyone might want to transform it, with a default of just returning the given error.

[rosston]

I think that might be a clearer way. Something like error as a top-level property? And then there's a special handler that is provided the error object and can return whatever it wants to go on error?

The only tricky thing there is that the base error object in V8 has no toJSON method, no enumerable properties, and Object.getOwnPropertyNames returns ["stack", "message"], which excludes the (semi-important) name property. So if, say, the user decides to simply return the error as is, possibly nothing will show up. Or maybe we just always put the name and message properties on the error object?

I'm expecting to have time tomorrow to work on this specifically, so ideas are welcome!

[rosston]

After testing this error transform approach in a real application, I think it's a little too dangerous. I think libraries will sometimes throw errors with circular references (e.g., https://github.com/vpulim/node-soap), which causes winston to exceed the max call stack size when trying to serialize the error. In that case, a user of this module would be required to use the errorTransform if a library they use happens to throw errors with circular references.

I think it would be preferable to have safe defaults, so I'll probably open a new PR later with an errorWhitelist and errorFilter as described in https://github.com/bithavoc/express-winston/issues/95#issuecomment-166019364.

[cbaclig]

@rosston Any updates on this? I'm happy to open a PR, just wanted to check if any prior work outside of #99 had been done.

I'm using VError, and I've recently run into a scenario where I'd like to use something like errorTransform to add info from VError.info(err) to my logs.

[rosston]

@cbaclig No updates/progress on this. It eventually ended up being less of an issue at my day job, so I never got around to it. You're welcome to work on it if you want. 😄

I think my suggested approach in https://github.com/bithavoc/express-winston/issues/95#issuecomment-166019364 is probably the best way to go. I really prefer the idea of an errorTransform, but it seems too prone to developers accidentally introducing server crashes into their app by forgetting to remove circular-dependent properties. errorWhitelist + errorFilter provides a bit more safe-by-default functionality, I think.

[rosston]

Just realized json-stringify-safe exists, so forget what I said about circular references. We just need to use that module + errorTransform, and everything will be fine.

[cbaclig]

Given the addition of dynamicMeta in #124, instead of an errorTransform property, adding a dynamicMeta that effectively does the same thing seems like it'd accomplish the same thing and keep the interface more consistent between the "normal" logger and the errorLogger. The only difference would be that the function passed to dynamicMeta for the errorLogger would get an additional err parameter. e.g.

dynamicMeta: function(err, req, res) { return [Object]; } 

Thoughts?

[rosston]

I think I'm fine with this approach as a customization option, although err should probably be the last parameter to keep backward compatibility.

Is there any reason not to just always put the entire error (run through json-stringify-safe) in the metadata (on, say, a rawError key)?

[rosston]

Fixed in #139 (via dynamic meta in the error logger) and released in 2.2.0.