Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default codec instantiation #68

Closed
wants to merge 1 commit into from
Closed

Fix default codec instantiation #68

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/logstash/outputs/syslog.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,17 @@ class LogStash::Outputs::Syslog < LogStash::Outputs::Base
# syslog message format: you can choose between rfc3164 or rfc5424
config :rfc, :validate => ["rfc3164", "rfc5424"], :default => "rfc3164"

default :codec, nil

def register
@client_socket = nil

if ssl?
@ssl_context = setup_ssl
end

if @codec.instance_of? LogStash::Codecs::Plain
if @codec.config["format"].nil?
@codec = LogStash::Codecs::Plain.new({"format" => @message})
end

if @codec.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why PR #55 wasn't merged, but seems the right way to fix it. May in some weeks @jsvd could confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad sorry, #69

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsvd confirmed that the #55 was closed for some other reason, mainly because the Travis CI changes were already included.

@codec = LogStash::Codecs::Plain.new({ "format" => @message })
end
@codec.on_event(&method(:publish))

Expand Down