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

Update OtlpMeterRegistryTest to run builds on Java 19 #3431

Merged
merged 1 commit into from Sep 27, 2022

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Sep 26, 2022

The following tests are failing:

  • ofEmptyDoesNotAllocate() and andEmptyDoesNotAllocate() in TagsTest are failing as their allocated bytes have been changed from 0 to 16 somehow. I'm not sure what makes the difference, so it might be better to investigate further with a dedicated issue if possible.
  • OtlpMeterRegistryTest.distributionSummaryWithHistogramBuckets() is failing as String representation for double has been changed slightly. See "Double.toString(double) and Float.toString(float) may Return Slightly Different Results" item in https://jdk.java.net/19/release-notes

For the first two tests, I just split the tests into two, one for prior versions and the other for 19+.

For the second test, I changed it to compare with double values, not with their String representation.

@shakuzen
Copy link
Member

For the second test, I changed it to compare with double values, not with their String representation.

That sounds reasonable. I think we should add a comment in the code why we're doing that. Otherwise surely I (and probably others) will forget and it might get buried or not easily found in git history.
We should also check if this causes any issues in metrics backends ingesting from some applications on Java 19+ and some on earlier versions. Some buckets may end up slightly different. If it's a problem, maybe we need to do something different in main code.

@shakuzen
Copy link
Member

I've opened #3436 for investigating the change in allocation.

@shakuzen shakuzen added the type: task A general task label Sep 27, 2022
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

I think we'll need these changes in the oldest applicable supported maintenance branch, since we will build those with JDK 19 also. That may require splitting the PR since OTLP was added in 1.9.x, but the Tags allocation tests were added to 1.8.x.

@izeye izeye changed the base branch from main to 1.9.x September 27, 2022 14:30
@sonatype-lift
Copy link
Contributor

sonatype-lift bot commented Sep 27, 2022

⚠️ 6 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@izeye
Copy link
Contributor Author

izeye commented Sep 27, 2022

@shakuzen Thanks for the feedback!

I extracted the TagsTest changes into #3437 for the 1.8.x branch and changed the base branch for this PR to the 1.9.x branch as you suggested.

@izeye izeye changed the title Update tests to run builds on Java 19 Update OtlpMeterRegistryTest to run builds on Java 19 Sep 27, 2022
@shakuzen shakuzen added this to the 1.9.5 milestone Sep 27, 2022
@shakuzen shakuzen merged commit 35be26e into micrometer-metrics:1.9.x Sep 27, 2022
shakuzen pushed a commit that referenced this pull request Sep 27, 2022
`ofEmptyDoesNotAllocate()` and `andEmptyDoesNotAllocate()` in TagsTest were failing as their allocated bytes have been changed from 0 to 16 somehow. I'm not sure what makes the difference, so it might be better to investigate further with a dedicated issue if possible.

See gh-3431
See gh-3436
@izeye izeye deleted the java19 branch September 27, 2022 15:00
@izeye
Copy link
Contributor Author

izeye commented Sep 27, 2022

For the record, I have seen the following error when I switched to the 1.9.x branch:

* What went wrong:
Could not open cp_settings generic class cache for settings file '/Users/user/IdeaProjects/micrometer/settings.gradle' (/Users/user/.gradle/caches/7.5.1/scripts/5qxrwyxrman55a9r3pyawbcqa).
> BUG! exception in phase 'semantic analysis' in source unit '_BuildScript_' Unsupported class file major version 63

It has been reproduced consistently, but after I switched to Java 17 and then back to Java 19, it's not reproducible anymore.

Based on gradle/gradle#20372, it seems that Gradle 7.6+ would support Java 19 officially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants