pkallos / winston-firehose

NodeJS module, winston logging transport which writes to AWS Firehose.
MIT License
13 stars 9 forks source link

Handle log callback #18

Closed laardee closed 5 years ago

laardee commented 5 years ago

For example, running the example snippet from readme fails to put both of the log messages to the Firehose stream because callback handling is missing from the send. (tried with Winston v3.2.1)

var winston = require('winston');
var WFirehose = require('winston-firehose');

// register the transport
var logger = winston.createLogger({
    transports: [
      new WFirehose({
        'streamName': 'firehose_stream_name',
        'firehoseOptions': {
          'region': 'us-east-1'
        }
      })
    ]
  });

// log away!!
// with just a string
logger.info('This is the log message!');

// or with meta info
logger.info('This is the log message!', { snakes: 'delicious' });

The execution freezes, because the first callback is not returned, and only the first message is pushed to the stream. This PR adds callback handling by setting it to be executed in the next iteration of the event loop.

pkallos commented 5 years ago

Hey, thanks for the contribution, looks good to me. Can you confirm that logger.info('This is the log message!', { snakes: 'delicious' }); will still log the message meta { snakes: 'delicious' }?

pkallos commented 5 years ago

And can you think of a way to update the spec tests so that this error would have been caught?

pkallos commented 5 years ago

@laardee deployed as 2.0.5, thanks for your contribution.

laardee commented 5 years ago

Thanks for merging and publishing!

This is a sS3 output from the firehose I used for testing, so yes, snakes are still delicious.

{"timestamp":"2019-09-06T06:54:21.976Z","message":"This is the log message!","level":"info"}{"timestamp":"2019-09-06T06:54:22.013Z","snakes":"delicious","level":"info","message":"This is the log message!"}

About the tests, in firehoseLoggerSpec the aws-sdk firehose could be mocked and spied instead of firehoser. That way the whole module would be tested without mocked firehoser.

pkallos commented 5 years ago

Thanks again @laardee , please let me know if 2.0.5 is working happily. Seems I introduced a regression when upgrading winston from 2.x -> 3.x, I will add a spec test to hopefully catch this type of issue in the future.

The botched versions have been removed or deprecated in the npm registry.