Closed rikwasmus closed 8 years ago
Testable by sending in a gelf message with a for instance: {"version":"1.1", ......, "timestamp":1.455235639119e+09,.....}
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.
CLA signed.
OK, added comment to let it recheck CLA, can someone look at it now? :)
Thanks for signing the CLA :)
I am +1 on the idea. Code looks OK.
Can you add tests for this? Alternately, can someone report that fractional-second support works for them?
I have to admit I have too little experience with spec tests to add to this reliably. However, I can show reproducing code:
/opt/logstash$ ./vendor/jruby/bin/jruby -e 'require "bigdecimal";puts Time::at(BigDecimal.new("1457454860.4164")).strftime("%N");'
000000000
/opt/logstash$ ./vendor/jruby/bin/jruby -e 'require "bigdecimal";puts Time::at(1457454860,416400).strftime("%N");'
416400000
I am also +1 on this fix, I also prefer this method of using the frac
method instead of to_f
to avoid potentially loosing precision, but my only reservation is that if for any reason the timestamp value end up as an integer or a float, the frac
method will crash. Dealing with BigDecimal
now is implementation specific and we cannot make that guarantee in the future. I suggest 2 things:
1- proceed with a short-term fix using the proposed frac
method but also add an explicit cast to BigDecimal which will be future proof and handle any numeric types:
event.timestamp = LogStash::Timestamp.at(event["timestamp"].to_i, BigDecimal.new(event["timestamp"]).frac * 1000000)
2- also add proper handling of BigDecimal
in Timestamp.at
so that any other usage will benefit this fix upon next release.
This is the proper fix as discussed in elastic/logstash#4565 - @guyboertje @alex88, agree?
Also this would replace the proposed #30 and #34.
:+1:
I made the modifications per my proposal and created a new PR #35 that will replace this one. Thanks @rikwasmus for your initial work, your commits will be preserved in the new PR.
closing, fix is in #35.
Observed in the bundled .deb packages 2.1.1 & 2.2.0 from https://www.elastic.co/downloads/logstash, the JRuby interpreter there cannot correctly deal with a single BigDecimal argument in ::Time.at(), converted to giving 2 arguments so as much of the fractional part of the BigDecimal can be kept as possible.