search

namshi / winston-graylog2

Graylog2 transport for winston, a nodejs logging module
MIT License
126 stars 62 forks source link

Fix incorrect Error objects when logging and Added formatters #62

Closed surbhi-mahajan closed 6 years ago

surbhi-mahajan commented 6 years ago

Bugfix: Resolved issue related to incorrect error object when logging to graylog server. Example : - log.error(new Error('Error message')) prints empty message to graylog server as prelog method receive empty string as message . So, to include correct error message and error stack in user-defined log message, prelog function is now called with error message and meta object including error stack and other properties. winston version used: 2.4.4

Feature: Replaced prelog option with formatter option as it automatically supports various formatters like json, raw etc and various styling by reusing winston's log method.

surbhi-mahajan commented 6 years ago

@smithclay Build failed because prelog option is replaced with the formatter option in this PR and unit test related to prelog option was failed

odino commented 6 years ago

@surbhi-mahajan could you update the tests?

@schfkt would you mind having a look at this?

schfkt commented 6 years ago

@odino sure, I'll look into it..

schfkt commented 6 years ago

@surbhi-mahajan one more question:

log.error(new Error('Error message')) prints empty message

Why do you need to pass an error object as the first argument to the error method instead of an actual log message (which, I guess, has to be string)? It looks weird.

I think, that was the reason for an empty message for that case. To be clear, here's the correct way to call error method:

log.error("Something went wrong", new Error("Error"));
surbhi-mahajan commented 6 years ago

@schfkt FYI

log.error(new Error('something went wrong'))
log.error('something went wrong', new Error('something went wrong'))

In both cases, Winston provides error object to winston-graylog in the form of meta object and to get error stack and other options in log message field, there is need to call prelog method with meta object including an error object and other options

schfkt commented 6 years ago

@surbhi-mahajan

and to get error stack and other options in log message field, there is need to call prelog method with meta object including an error object and other options

Again, you can just pass that meta object into prelog function here https://github.com/namshi/winston-graylog2/blob/master/lib/winston-graylog2.js#L101 Much simpler change.

surbhi-mahajan commented 6 years ago

@schfkt I agreed graylog does not support colorize, prettyPrint but it supports all other options like json, logstash, raw, timestamp, stringify, custom formatter (i.e prelog in this case) etc. My point is by using common.log method of Winston, it automatically supports above formats and common.log method also passes meta ,message and other options to custom formatter (i.e prelog option) for us. In this way, user does not need to provide his own custom formatter (i.e prelog option) in case he wants any above formats (json, raw, logstash etc).

Yes, above approach that you have mentioned is simpler and it also preprocess meta and message both But I think it will be great if we reuse common.log method provided by Winston as it automatically supports various formatters and pass meta data to custom formatters for us. Moreover, another benefit is code consistency between various custom and built-in transports as formatter option is also used in built-in transports.

For reusability point of view, I used common.log method of winston to pass message, meta to custom formatter and this way also supports various formatters.

schfkt commented 6 years ago

@surbhi-mahajan what's the point of storing a message in graylog with json format? That way you'll end up with the duplicated data in full_message field (but formatted as json), and everything else from meta object saved as a separate field. See this example:

graylog

And the same for logstash:

logstash

Both these options don't make any sense for graylog. Log message must be a message, not an object containing all the data you log. There's meta object exactly for that purpose (it's fields are stored separately in graylog, which allows you to do search on them).

schfkt commented 6 years ago

@surbhi-mahajan So, looks like this pull request was abandoned. I'm closing it and here's why:

  1. The tests were just made incorrect. See https://github.com/namshi/winston-graylog2/pull/62#pullrequestreview-150414500
  2. As explained above, there's no point in changing the transport in order to do some custom logic that's not even related to graylog. If you want to log an error, do it like so:
const err = new Error("Something wrong happened!");
logger.error("Oh noes!", err);

Which will result in the following entry in graylog:

image