Add support for codecs - Githubissues

josephlewis42 commented 6 years ago

Add the ability for the user to overload the built-in "codecs" and use real ones Logstash provides. Fixes #10

output_format should really be deprecated at this point but there isn't really a clean upgrade path.

colinsurprenant commented 5 years ago

Thanks @josephlewis42 Is there a particular reason we just don't always use codecs here instead of introducing use-codec? for BWC if output-format is set, set codec to the corresponding value, otherwise use the codec option value?

josephlewis42 commented 5 years ago

Hi @colinsurprenant,

The main reason was to avoid a breaking change so it could get out faster.

I can roll this into #21 as a breaking change if you'd be willing to take a look at it afterward. I have a customer that needs that one merged because the authentication mechanisms this plugin uses are very deprecated.

colinsurprenant commented 5 years ago

@josephlewis42 I'd prefer we keep these concerns separate.

So, what I was suggesting should IMO be BWC; keep the current output_format option but make it optional without a default, and add the codec option with the plain codec by default.

if users sets output_format to plain, just set codec to plain and if user set it to json use the json_line codec which should provide the same BWC functionality but get rid of the legacy encoding and rely only on codecs. LMKWYT!

colinsurprenant commented 5 years ago

@josephlewis42 LMK if my proposal above makes sense and if you are able to move that forward? (or alternatively if you'd like help with that). Thanks!

josephlewis42 commented 5 years ago

Hi @colinsurprenant that sounds way better than what I was doing! I'm preparing for a conference next week, but do have a little time today where I can take a look getting this branch up to date with master then getting in the codecs.

colinsurprenant commented 5 years ago

Great stuff @josephlewis42 - Thanks! Overall looks good, just a few inline comments nitpicks.

colinsurprenant commented 5 years ago

@josephlewis42 Oh - I also just looked at the build failures and they seems related to the rebase which introduced the new config options which will need to up corrected in the tests here.

josephlewis42 commented 5 years ago

Hi @colinsurprenant,

I was at a conference last week, but your original comments weren't forgotten! I had to dig a bit through LogStash::Plugin and LogStash::Config::Mixin but now we're using real codecs even when emulating the legacy behavior.

I also fixed the debug logging you recommended.

Cheers! - Joseph

colinsurprenant commented 5 years ago

@josephlewis42 Thanks! One last thing: I suggest we use nil instead of blank ""

config :output_format, :validate => [ "json", "plain", nil ], :default => nil, :deprecated => 'Use codec instead.'

Adjust docs and specs to include cases with :output_format => nil and no :output_format option.

After that we should be good to merge. Sorry for the delay.

x25 commented 5 years ago

bump! please, merge it :) @josephlewis42 @colinsurprenant

colinsurprenant commented 5 years ago

@josephlewis42 let me know if you would like to complete this otherwise I can certainly help with the last bits. Thanks.

colinsurprenant commented 5 years ago

I rebooted this PR in #42

colinsurprenant commented 5 years ago

Closing - thanks a lot for your work on this @josephlewis42 .

colinsurprenant commented 5 years ago

FYI published v4.1.0 with codec support.

logstash-plugins / logstash-output-google_cloud_storage

Add support for codecs #34