bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
797 stars 187 forks source link

Support for winston@3.0 #163

Closed ringzhz closed 6 years ago

ringzhz commented 6 years ago

Hey. We're about to upgrade our logging platform and wanted to leverage winston@3.0.0 which is in rc1 now. Looks like it has some breaking changes that need adjustment in express-winston.

https://github.com/winstonjs/winston/blob/master/UPGRADE-3.0.md

I'll create a PR if you're not already in development.

bithavoc commented 6 years ago

@ringzhz please do, what would the the strategy here, keep two branches?

makrandgupta commented 6 years ago

Having two branches would make sense. Once winston v3 is stable then you can simply merge them into master. Let me know if you need help with this. I am currently overhauling the logging system in our platform as well. Would love to have this before we roll out our production servers

bithavoc commented 6 years ago

feel free to create pull requests to https://github.com/bithavoc/express-winston/tree/winston-3. or commit directly to that branch, you're a contributor now.

makrandgupta commented 6 years ago

sweet. i'm guessing that I cant rely on the existing tests yet since they will also need to be updated?

bithavoc commented 6 years ago

correct

makrandgupta commented 6 years ago

@bithavoc @rosston I'm moving the discussion from efbb61f68b7adb891ac83f7b4ef5cc0914f3a8b4 to this issue for simplicity sake

makrandgupta commented 6 years ago

So I took another crack at it and discovered that winston@3.0.0 no longer maintains a separate meta field when logging the error. If it does find some metadata passed to log, then it simply merges that up with the other info into one object and then passes it to the write method. This means that all the data inside result.log.meta.req ends up in result.req so a bunch of the existing tests no longer make sense

Another issue I ran into is this:

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 uncaughtException listeners added. Use emitter.setMaxListeners() to increase limit

So I tried to do as described in https://github.com/winstonjs/winston/issues/744 but that didnt help. So yeah any further help beyond this point would be really appreciated

padiazg commented 6 years ago

hi, Found this problem yasterday, no meta fields saved but merged with the message itself.

Solved initializing winston with: Currect usage? Metadata is included in message when using Winston 3

const logger = new createLogger({
  format: format.printf(info =>  info.message ),
  transports: [
    ...
  ]
});

The "format" solves the merging of meta with message, forcing winston to output just the message and not the generated (composed?) one,

Finally changing line 337 from:

options.winstonInstance.log(level, msg, meta);

to

options.winstonInstance.log({level, message: msg, meta});

After that, it logs right as needed.

The problem was the new format for calling winston.log(), and the way winston handles the output of the message. Documentation says that it supports the old format : winston.log(message, level, meta), but it doesn´t works as espected.

Cxarli commented 6 years ago

I temporarily fixed this with

const winston = require('winston');
const expressWinston = require('express-winston');
const winstonLog = require('./node_modules/winston/lib/winston/common').log;

app.use(expressWinston.logger({
    transports: [
        new winston.transports.Console({
            colorize: true,

            // HACK: Remove when `express-winston` supports the latest `winston`
            // HACK: See https://github.com/bithavoc/express-winston/issues/163
            formatter: function(opts) {
                opts.formatter = undefined;
                return winstonLog.apply(this, arguments);
            },
        }),
    ],

    meta: false,
    // expressFormat: true,
    msg: `{{req.ip}} {{req.method}} {{req.url}} {{res.statusCode}} {{res.responseTime}}ms`,
    colorize: true,
}));

Would be nice if it could be fixed soon

fernandocanizo commented 6 years ago

@C-Bouthoorn thanks but I get: TypeError: winston.Logger is not a constructor

Cxarli commented 6 years ago

@fernandocanizo Weird, my piece of code doesn't seem to use that directly. It seems express-winston version 2.0.0 broke my workaround.

However, replacing new winston.Logger with winston.createLogger on node_modules/express-winston/index.js:190 seems to fix it. Update It fixes only that error, but the output is still JSON instead of a normal message. See my next post for another workaround

Cxarli commented 6 years ago

Update: this seems to work fine for now

const winston = require('winston');
const expressWinston = require('express-winston');

let winstonOptions = {
    format: winston.format.combine(
        winston.format.colorize(),
        winston.format.timestamp(),
        winston.format.align(),
        winston.format.printf(info => `${info.timestamp} ${info.level}: ${info.message}`)
    ),

    transports: [
        new winston.transports.Console(),
    ],
};

let expressWinstonOptions = {
    meta: false,
    msg: `{{req.ip}} {{req.method}} {{req.url}} {{res.statusCode}} {{res.responseTime}}ms`,
    colorize: true,
};

// HACK: Remove when `express-winston` fixes this
// HACK: See https://github.com/bithavoc/express-winston/issues/163
expressWinstonOptions.winstonInstance = winston.createLogger(winstonOptions);
app.use(expressWinston.logger(expressWinstonOptions));
rosston commented 6 years ago

winston@3 is now supported in v3 of express-winston (just published). Check winston@3's changelog and upgrade guide to get an idea of its breaking changes.