Closed wpjunior closed 2 years ago
💚 CLA has been signed
@jsvd any chance that this PR gets merged?
Hi folks, what if we just handled this special case: if the stripped key is "@timestamp", convert the value to LogStash::Timestamp, and if that fails, convert it to BigDecimal? For example:
def strip_leading_underscore(event)
# Map all '_foo' fields to simply 'foo'
event.to_hash.keys.each do |key|
next unless key[0,1] == "_"
new_key = key[1..-1]
value = event.get(key)
if new_key == LogStash::Event::TIMESTAMP
event.set(new_key, coerce_timestamp_carefully(value))
else
event.set(new_key, value)
end
event.remove(key)
end
end
def coerce_timestamp_carefully(value)
LogStash::Timestamp.at(value)
rescue TypeError => e
# maybe it's a BigDecimal?
coerced_value = BigDecimal.new(value)
LogStash::Timestamp.at(coerced_value)
end
I think could be the right path but also the coercion of the BigDecimal
could make the pipeline crash. Considering the piece:
coerced_value = BigDecimal.new(value)
if value
is a malformed String for example "foo" then it raises a NumberFormatException
so in that case the event should be dropped
@yaauie I like the idea to tag the event instead of dropping with log.
I think we can't do event.tag("_gelf_move_field_failure", "<reason>")
according to https://github.com/elastic/logstash/blob/65e163fc5b5fa6a9d25a2272b3d075f112a8cb61/logstash-core/src/main/java/org/logstash/ext/JrubyEventExtLibrary.java#L285-L291 so should we add another subfield in @metadata
to fill with the reason?
This fix very likely breaks logstash: https://github.com/logstash-plugins/logstash-input-gelf/issues/70
The UDP server crashes when receives a GELF message like that
{"full_message": "my message", "_@timestamp": 12345678889}
I wrote a test case to show that behavior.
Thanks
Release notes
Safe parsing of
_@timestamp
to avoid crashing the plugin and LogstashWhat does this PR do?
This PR safely cast the values of
_@timestamp
field in GELF and in case of error skip the event, avoiding Logstash crashWhy is it important/What is the impact to the user?
Permit to handle correctly the numerical values in
_@timestamp
without stopping Logstash process in case of error.Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
Gemfile
:bin/logstash-plugin install --no-verify
pipeline.conf
as:output { stdout { codec => rubydebug { metadata => true } } }
echo -n '{ "version": "1.1", "host": "example.org", "shortmessage": "A short message", "level": 5, "@timestamp": "foo" }' | nc -u -w0 127.0.0.1 3333
and the parsing of such payload can't crash Logstash.
Logs