Stringify key :message in the base formatter

[jcmfernandes]

Doing the following (an example taken from the README
file)

logger.debug message: 'test', foo: 'bar'

will fail for any logger whose the formatter's class
inherits from LogStashLogger::Formatter::Base when
LogStashLogger.configuration.max_message_size is
set. This happens because event['message'.freeze].byteslice
(previously on line 49) will fail as
event['message'.freeze] will return nil.

[dwbutler]

Nice catch. And good call on treating the input as immutable as well.

I dove into the source code of LogStash::Event and discovered that most of the "built in" fields are expected to use string keys. So it might make sense to convert some or all of the keys to strings.

Example of the problem:

> LogStash::Event.new(tags: ['foo']).tags
# => nil
> LogStash::Event.new('tags' => ['foo']).tags
# => ["foo"]

[jcmfernandes]

Thanks! 😺

I agree. Should we deep stringify hashes or just root keys?

[stereobooster]

Constanize strings as well e.g.

MESSAGE_FIELD = 'message'.freeze
...
event_data[MESSAGE_FIELD]

[dwbutler]

My recollection is that Ruby will optimize this sort of code anyway, and therefore extracting to a constant isn't necessary. Can't find a source for this.

[stereobooster]

Yep. I missed this optimisation somehow. Here is the proof (at least for ruby 2.2.6p396 (2016-11-15 revision 56800)):

irb(main):001:0> "asd".object_id
=> 70178538223500
irb(main):002:0> "asd".object_id
=> 70178538189060
irb(main):003:0> "asd".freeze.object_id
=> 70178538223620
irb(main):004:0> "asd".freeze.object_id
=> 70178538223620

[stereobooster]

but for ruby 2.0.0p247 (2013-06-27 revision 41674)

irb(main):001:0> "asd".object_id
=> 70313257930780
irb(main):002:0> "asd".object_id
=> 70313241344880
irb(main):003:0> "asd".freeze.object_id
=> 70313241338520
irb(main):004:0> "asd".freeze.object_id
=> 70313241348500
irb(main):005:0> "asd".freeze.object_id

And taking into account that library claims to support ruby 2.0 constanization still makes sense

[dwbutler]

Thanks for the legwork. And good point, LogStashLogger support goes back to 1.9, so it should definitely do whatever optimizations are necessary.

[dwbutler]

There is a very specific set of keys that LogStash::Event expects to be strings. There wouldn't be any value in stringifying all keys, or deep converting keys.

Since this fixes an immediate bug, I'll merge it as-is and implement a more generic solution later.

[jcmfernandes]

Can we get this merged @dwbutler or would you like to see some changes to it? Let me know 😉

[jcmfernandes]

Thank you!

[dwbutler]

Fix released in 0.24.1. Thanks!