Does winston-cluster support winston.containers?

johnmcilwain commented 7 years ago

I am probably doing something dumb, but I can't get it to work. My sample code:

var cluster = require('cluster');
var winston = require('winston');
var winstonCluster = require('winston-cluster');

if (cluster.isMaster) {

    winston.loggers.add('logwinston', {
        console: { 'timestamp': true, colorize: true, label: 'category one' },
        file: { filename: 'logfile1' }
    });
    winston.loggers.add('logsystem', {
        console: { 'timestamp': false, colorize: false, label: 'category two' },
        file: { filename: 'logfile2' }
    });

    var category1 = winston.loggers.get('logwinston');
    category1.log('info', 'Hello distributed log files!');

    var category2 = winston.loggers.get('logsystem');
    category2.log('info', 'Hello distributed log files!');

    //Bind logging listeners to workers 
    winstonCluster.bindListeners();

    //Start workers 
    var cpuCount = require('os').cpus().length;
    for (var i = 0; i < cpuCount; i++) {
        cluster.fork();
    }

    //Master logic here 
    //... 

} else {

    //Replace default transport with cluster transport 
    winstonCluster.bindTransport();

    var category1 = winston.loggers.get('logwinston');
    category1.log('info', 'Child here');

    var category2 = winston.loggers.get('logsystem');
    category2.log('info', 'Child here');

    //Slave logic here 
    //... 

}

ryankurte commented 7 years ago

Hey @johnmcilwain,

With respect to your email, and issue #1, if winston works clustered now you may not need this. At the time that I wrote it, it didn't, and I needed to make sure logging would be handled by only one process (particularly for file writing), which it has been doing in production for a year or so now.

As it stands, it only overwrites the transports for the default handler, so does not currently support containers / instances.

You can see on line 70 how the master passes log events to the default winston instance, and on line 84 how the child transport is overwritten to use the cluster IPC.

If you need containers / instances you could probably alter the bindTransport function to take an optional winston instance to overwrite the underlying transport for a provided instance instead of the default instance, though I don't know what other magic occurs under the hood of winston.

Something like:

var bindTransport = exports.bindTransport = function(instance) {
    if(typeof instance !== 'undefined') {
       instance.remove(winston.transports.Console);
       instance.add(winston.transports.Cluster, {});
    } else {
        winston.remove(winston.transports.Console);
        winston.add(winston.transports.Cluster, {});
    }
}

Then in the child process:

var lw = winston.loggers.get('logwinston');
winstonCluster.bindTransport(lw);

Might work, though would route all messages to the default handler in the parent.

If you want these to pass back to separate instances in the parent, an additional field (instance, ie. 'logwinston') may need to be passed through the IPC and used to select which instance to write back to.

As an aside from that, I believe you will need to instantiate the loggers in both the parent and child processes (var category1 = winston.loggers.get('logwinston');).

If you decide to implement any of this, I would welcome a pull request!

Good luck,

Ryan

johnmcilwain commented 7 years ago

Thanks Ryan, I appreciate the feedback!

I'll give it a shot and if I get it working, I'll do a pull request.

-john

From: Ryan notifications@github.com Sent: Tuesday, November 29, 2016 1:12:55 AM To: ryankurte/winston-cluster Cc: johnnymac949; Mention Subject: Re: [ryankurte/winston-cluster] Does winston-cluster support winston.containers? (#4)

Hey @johnmcilwainhttps://github.com/johnmcilwain,

With respect to your email, and issue #1https://github.com/ryankurte/winston-cluster/issues/1, if winston works clustered now you may not need this. At the time that I wrote it, it didn't, and I needed to make sure logging would be handled by only one process (particularly for file writing), which it has been doing in production for a year or so now.

As it stands, it only overwrites the transports for the default handler, so does not currently support containers / instances.

You can see on line 70https://github.com/ryankurte/winston-cluster/blob/master/lib/winston-cluster.js#L70 how the master passes log events to the default winston instance, and on line 84https://github.com/ryankurte/winston-cluster/blob/master/lib/winston-cluster.js#L84 how the child transport is overwritten to use the cluster IPC.

If you need containers / instances you could probably alter the bindTransport function to take an optional winston instance to overwrite the underlying transport for a provided instance instead of the default instance, though I don't know what other magic occurs under the hood of winston.

Something like:

Then in the child process:

var lw = winston.loggers.get('logwinston'); winstonCluster.bindTransport(lw);

Might work, though would route all messages to the default handler in the parent.

If you want these to pass back to separate instances in the parent, an additional field (instance, ie. 'logwinston') may need to be passed through the IPC and used to select which instance to write back to.

As an aside from that, I believe you will need to instantiate the loggers in both the parent and child processes (var category1 = winston.loggers.get('logwinston');).

If you decide to implement any of this, I would welcome a pull request!

Good luck,

Ryan

- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ryankurte/winston-cluster/issues/4#issuecomment-263446651, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AHX4e1FIhp168Mf7eY38XpVrz4QMGN8kks5rC3wXgaJpZM4K8F7Q.

ryankurte / winston-cluster

Does winston-cluster support winston.containers? #4