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.

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

Switched time string comparation to Timestamp comparable to avoiding problems due to precision loss #141

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Oct 28, 2021

Release notes

Fix spec and unit tests after introduction of nanosecond precision in Logstash with elastic/logstash#12797

What does this PR do?

This PR switched time comparations in specs and unit test from String to org.logstash.Timestamp to avoid problems due to loss of precision.

The PR elastic/logstash#12797 introduced nanosecond precision into Logstash, a similar problem happened also into JDBC plugin and here is implemented the same solution described in the original comment.

Why is it important/What is the impact to the user?

It doesn't impact the user, it's only related to tests

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

  • run ./gradlew test on a JDK 11

Related issues

Use cases

Screenshots

The problem could be seen at https://app.travis-ci.com/github/logstash-plugins/logstash-filter-date/jobs/545405353#L528

Logs

logstash_1_f52dffb2a8a0 | <=====--------> 42% EXECUTING [8s]> :compileTestJava<=====--------> 42% EXECUTING [9s]<===========--> 85% EXECUTING [9s]> :test<===========--> 85% EXECUTING [10s]<===========--> 85% EXECUTING [11s]<===========--> 85% EXECUTING [12s]> :test > Resolve files of :testRuntimeClasspath> :test > 0 tests completed<===========--> 85% EXECUTING [13s]> :test > Executing test org.logstash.filters.parser.JodaParserTest<===========--> 85% EXECUTING [14s]> :test > 2 tests completed> :test > Executing test org.logstash.filters.parser.UnixEpochParserTest> :test > 16 tests completed> :test > 162 tests completed> :test > 178 tests completed> :test > Executing test org.logstash.filters.DateFilterTest<===========--> 85% EXECUTING [15s]<===========--> 85% EXECUTING [16s]<===========--> 85% EXECUTING [17s]<===========--> 85% EXECUTING [18s]> :test > 180 tests completedlogstash_1_f52dffb2a8a0 | > Task :test
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | org.logstash.filters.DateFilterTest > testIsoStrings FAILED
logstash_1_f52dffb2a8a0 |     java.lang.AssertionError at DateFilterTest.java:210
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | org.logstash.filters.DateFilterTest > testUnixLongs FAILED
logstash_1_f52dffb2a8a0 |     java.lang.AssertionError at DateFilterTest.java:210
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | org.logstash.filters.DateFilterTest > testIsoStringsInterpolateTz FAILED
logstash_1_f52dffb2a8a0 |     java.lang.AssertionError at DateFilterTest.java:210
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | org.logstash.filters.DateFilterTest > testUnixInts FAILED
logstash_1_f52dffb2a8a0 |     java.lang.AssertionError at DateFilterTest.java:210
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | <===========--> 85% EXECUTING [18s]> :test > 186 tests completed, 4 failed> :test > Executing test org.logstash.filters.DateFilterTestlogstash_1_f52dffb2a8a0 | org.logstash.filters.DateFilterTest > testPatternStringsInterpolateTzNoYear FAILED
logstash_1_f52dffb2a8a0 |     java.lang.AssertionError at DateFilterTest.java:210
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | org.logstash.filters.DateFilterTest > testUnixStrings FAILED
logstash_1_f52dffb2a8a0 |     java.lang.AssertionError at DateFilterTest.java:210
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | <===========--> 85% EXECUTING [18s]> :test > 188 tests completed, 6 failedlogstash_1_f52dffb2a8a0 | 188 tests completed, 6 failed
loggstash_1_f52dffb2a8a0 | logstash_1_f52dffb2a8a0 | 
logstash_1_f52dffb2a8a0 | <===========--> 85% EXECUTING [19s]> :testlogstash_1_f52dffb2a8a0 | > Task :test FAILED

@andsel andsel mentioned this pull request Oct 28, 2021
5 tasks
@andsel
Copy link
Contributor Author

andsel commented Oct 28, 2021

@yaauie in the comment you noted that the implementation of Data filter needs to switch from Joda to java.time, this PR is intended to be a bugfix to get the tests green again.

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

LGTM ❇️

end
end

RSpec::Matchers.define :be_a_logstash_timestamp_equivalent_to do |expected|
Copy link
Contributor

Choose a reason for hiding this comment

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

we've now added this matcher to two plugins, and should probably consider adding it to our shared devutils spec helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HI @yaauie thanks for pointing it out, I've created issue elastic/logstash-devutils#98 to track it.
I've aligned to master and bumped the version of this plugins to be published, so I ask you a quick second review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the dependency to devutils >= 2.3 to reuse the be_a_logstash_timestamp_equivalent_to matcher

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

Actual changes LGTM.

I don't think we need to bump the version, and we could improve the changelog to be more use-centric.

logstash-filter-date.gemspec Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@andsel andsel requested a review from yaauie November 2, 2021 08:27
@andsel andsel changed the title Switched time string compartion to Timestamp comparable to avoid probems due to precision Switched time string comparation to Timestamp comparable to avoid probems due to precision Nov 2, 2021
@andsel andsel changed the title Switched time string comparation to Timestamp comparable to avoid probems due to precision Switched time string comparation to Timestamp comparable to avoiding problems due to precision loss Nov 2, 2021
Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

Thank you for getting those shared assertions upstream into devutils!

@@ -24,7 +24,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'logstash-input-generator'
s.add_development_dependency 'logstash-codec-json'
s.add_development_dependency 'logstash-output-null'
s.add_development_dependency 'logstash-devutils'
s.add_development_dependency 'logstash-devutils', '>= 2.3'
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@andsel andsel merged commit 9fdbe23 into logstash-plugins:master Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants