Azure / fluentd-plugin-mdsd

Azure Linux monitoring agent (mdsd) output plugin for fluentd
Other
27 stars 16 forks source link

Add Time type to MessagePack packer and unpacker #44

Closed sbonebrake closed 6 years ago

sbonebrake commented 6 years ago

Fixes issue #43

I have not tested this locally as i cannot run the test suite on my mac. However, the following works in irb fine:

    require "msgpack"
    packer = MessagePack::Packer.new
    unpacker = MessagePack::Unpacker.new

    time_packer = packer.register_type(0x7f, Time) { |time| [time.to_i].pack("N") }
    time_unpacker = unpacker.register_type(0x7f) { |data| Time.at(data.unpack('N')[0]) }
    MessagePack::DefaultFactory.register_type( 0x7f, Time, packer: time_packer, unpacker: time_unpacker)

    sample_event = ["some_tag", Time.now, {"message" => "something"}]
    packer.pack(sample_event)
    unpacker.feed_each(packer.to_s) { |record| puts record }
    # Outputs:
    #some_tag
    #2017-10-24 15:33:13 -0700
    #{"message"=>"something"}
jasonzio commented 6 years ago

Loses sub-second precision. Can you round-trip using Time.to_f instead? Alternatively, convert to an array of two integers: time.tv_sec and time.tv_nsec. The out_mdsd plug-in is careful to convert a Time object to just such an array when it passes it to mdsd; it would be nice to retain that precision. The downside to such a conversion is that constructing a event (as you do) would still require the user to explicitly convert the time to an integer, as the fluentd in_forward event requires the timestamp to be an integer:

sample_event = ["some_tag", Time.now.to_i, {"EventTime" => Time.now }]
packer.pack(sample_event)
unpacker.feed_each(packer.to_s) { |record| puts record }
# Outputs:
#some_tag
#2017-10-24 15:33:13 -0700
#{"message"=>2017-10-24 15:33:13.123456789 -0700}
sbonebrake commented 6 years ago

Decided to add this functionality outside of this plugin because it doesn't directly interact with mdsd

fatal10110 commented 5 years ago

@sbonebrake

Decided to add this functionality outside of this plugin because it doesn't directly interact with mdsd

IMO I would disagree on that, since this plugin supports Time record value, but it forces you to use the msgpack format (that does not support Time value), so the only way to pass some Time value, is to use the emit_timestamp_name and use_source_timestamp (as a hack) that will have a special case parsing. I guess this plugin should at least depend on some lib that would implement to_msgpack_ext for Time class, since in most of the cases you would have Time value in your log/metric records.