Fix default codec instantiation #68
Conversation
Signed-off-by: Tero Saarni <[email protected]>
| @codec = LogStash::Codecs::Plain.new({"format" => @message}) | ||
| end | ||
|
|
||
| if @codec.nil? |
There was a problem hiding this comment.
@tsaarni why defaultnil assigment and then the nil(as if nil is a special value placeholder constant) instead of using the same path suggested in https://github.com/elastic/logstash/pull/11401/files which was listed in the issue you reported (elastic/logstash#11434) ?
Assigning the codec to nil to then change it register time is not a good practice.
There was a problem hiding this comment.
I used nil as a special value placeholder in attempt to avoid class identity comparison altogether. Comparing class identity by their string representation seemed bit awkward to me, but I'm happy with any fix as long as it works! :)
There already is a string-based alternative submitted here few years back but for some reason it was not merged #55. If the original submitter @jsvd is around maybe he could reconsider resurrecting the PR, or I can re-submit his fix here and add him as co-author.
There was a problem hiding this comment.
BTW: What I really wanted to do was to set the default :codec to LogStash::Codecs::Plain.new({ "format" => @message }) but I could not achieve that, because obviously I have no access to instance variable @message when defining default codec for the class. If that was possible, it would be possible to avoid instantiating codec in register like introduced here long time ago 2c73283 (change which introduced the class identity comparison).
There was a problem hiding this comment.
If you run tests on actual main, it fails for that check. To get the test back to green the string comparison works fine like in #65 which is why also this PR is green.
There was a problem hiding this comment.
If you run tests on actual main, it fails for that check. To get the test back to green the string comparison works fine like in #65 which is why also this PR is green.
Sorry, I did not understand, #65 has failed because of the codec problem as well. Maybe you meant #55? I agree, string comparing works, I'm happy if we get that in!
|
Hi @tsaarni given that PR #69 fixes this I foresee two paths:
|
|
Let's merge #69 and close this one, thank you for fixing the bug! |
|
Great, merged, so now you could rebase the next PR to review and move forward with the others :-) |
2 participants
This PR sets the default codec (temporarily) to
nilto allow instantiating the default codec withformatby testing fornilinstead of testing for the class identity, which has not been working since logstash 7.2.0 due to elastic/logstash#11434.Previously #55 was created with another workaround for the class comparison issue, but it was never merged. There has not been follow-up to this issue since then (as far as I can find).
Fixes #51