fluent / fluent-logger-node

A structured logger for Fluentd (Node.js)
Apache License 2.0
259 stars 83 forks source link

JS Date values sent as empty objects #132

Open mitsos1os opened 5 years ago

mitsos1os commented 5 years ago

Hi guys,

We have noticed that when trying to send a JSON object that contains a field of Date instance from the regular Date type in Javascript, it is serialized as an empty object. I would expect that the proper behavior should be to convert to ISO Date string.

This is happening due to the fact that fluent-logger is using msgpack-lite to encode the objects to be sent. However when a message is encoded it is using a hardcoded codec that is initialized in the instantiation of the module and cannot be accessed externally. See here

So the codec is created without any options, (skipping even the default codecs because preset flag is skipped) and when the type of the Date value given to serialized is checked with typeof, Object is returned and an attempt to browse its keys recursively and encode them results to an empty object, since Date instances do not have any browseable keys.

A solution would be to either provide the codec to the instance so it can be configured and extra packers / unpackers could be added, inherited from constructor options from the fluentd logger class, or even just simply enable the default presets of msgpack-lite to support JS native data types

okkez commented 5 years ago

Please show me the code to reproduce your situation.

mitsos1os commented 5 years ago

it is as simple as just sending a date instance object to be logged.

ex:

'use strict';

const FluentLogger = require('fluent-logger');

const logger = FluentLogger.createFluentSender('SOME_PREFIX', {
      host: '127.0.0.1',
      port: 24224,
      timeout: 3.0,
      reconnectInterval: 60000, // 1 minute
      eventMode: 'PackedForward'
    });

logger.emit('msgLabel', {
    msg: 'log message',
    date: new Date(), // the date object will be serialized as `{}`
}, new Date());
okkez commented 5 years ago

Thanks, I will check this later.

okkez commented 5 years ago

Hmm, msgpack-lite can convert JavaScript Date object to messagepack binaries. But it uses extension types.

Fluentd (in_forward plugin) cannot receive those messagepack binaries that use extension types defined by msgpack-lite.

mitsos1os commented 5 years ago

So it is due to the inability of the actual FluentD daemon to understand the extension types that are not properly handled? Because the actual date packer just uses Number formatting of the dates if I am not mistaken

okkez commented 5 years ago

Yes, Fluentd cannot handle extension types defined by msgpack-lite. Therefore we cannot use the default codec of msgpack-lite to send events which include JavaScript Date object to Fluentd.

I'm trying to consider handle JavaScript Date object in the event.

mitsos1os commented 5 years ago

Hmmmm, I understand. I believe that since Date could be commonly logged in javascript since it is a native type, it should be logged as ISOString as a generic form at least.

This should probably be supported by default but having also available a way to provide some custom packers to the final encoder would be useful for extra customization of the logging serialization format

okkez commented 5 years ago

I could not find a way to convert Date object to ISOString while invoking msgpack.encode. Because msgpack-lite does not have such an API.

msgpack-lite has an API to customize codec, but it will generate MessagePack binary using extension types. We cannot define a customized codec(to convert Date to ISOString) to generate MessagePack binary without extension types.

If you have good ideas to define customized codec without extension types, please show me the sample code.

My snippet is here:

'use strict';

class EventTime {
  constructor(epoch, nano) {
    this.epoch = epoch;
    this.nano = nano;
  }

  static pack(eventTime) {
    const b = Buffer.allocUnsafe(8);
    b.writeUInt32BE(eventTime.epoch, 0);
    b.writeUInt32BE(eventTime.nano, 4);
    return b;
  }

  static unpack(buffer) {
    const e = buffer.readUInt32BE(0);
    const n = buffer.readUInt32BE(4);
    return new EventTime(e, n);
  }

  static now() {
    const now = Date.now();
    return EventTime.fromTimestamp(now);
  }

  static fromDate(date) {
    const t = date.getTime();
    return EventTime.fromTimestamp(t);
  }

  static fromTimestamp(t) {
    const epoch = Math.floor(t / 1000);
    const nano = t % 1000 * 1000000;
    return new EventTime(epoch, nano);
  }
};

const msgpack = require('msgpack-lite');

function toISOString(date) {
  return date.toISOString();
}

const codec = msgpack.createCodec({useraw: true});
codec.addExtPacker(0x00, EventTime, EventTime.pack);
codec.addExtUnpacker(0x00, EventTime.unpack);
codec.addExtPacker(0xb8, Date, toISOString);

let d = new Date();

console.log(msgpack.encode(d, {codec: codec}));
let a = msgpack.encode({"key": d}, {codec: codec});
console.log("Date:codec", a);
let b = msgpack.encode({"key": d});
console.log("Date", b);
console.log(msgpack.encode(d));
console.log(msgpack.encode(d, {codec: codec}));

let e = EventTime.fromDate(d);
console.log(msgpack.encode(e, {codec: codec}));
let y = msgpack.encode({"key": e}, {codec, codec});
console.log("EventTime", msgpack.encode(e, {codec: codec}));
console.log(msgpack.encode("string123"));
console.log(msgpack.encode(new String("123")));

On the other hand, I don't want to add functionality to convert Date to ISOString automatically in fluent-logger layer. Because it will cause a performance issue.

Thus we should convert a Date object to ISOString before emitting events like followings:

'use strict';

const FluentLogger = require('fluent-logger');

const logger = FluentLogger.createFluentSender('SOME_PREFIX', {
      host: '127.0.0.1',
      port: 24224,
      timeout: 3.0,
      reconnectInterval: 60000, // 1 minute
      eventMode: 'PackedForward'
    });

logger.emit('msgLabel', {
    msg: 'log message',
    date: (new Date()).toISOString(), 
}, new Date());
mitsos1os commented 5 years ago

I think just adding the extra packer as you did above is a good choice. I don't think the performance overhead is that limiting. Also if we convert it manually, in case of composite data objects, we will have to browse the object in all depths to process all JS Date objects. Also since Date is a Native JS data type, I believe we will find this quite regularly

Currently, the existing solution simply logs dates as empty object, so we have loss of information.

We could also support an extra param in construction of FluendDSreamer so that accepts an extra param with a property, ex: packers that will contain an object of types and their packing / unpacking mechanisms in some kind of convention format.