update metrics library to 4.0 #30
Conversation
The 4.0 release of the metrics library (which changed groupId, from the original author to the Dropwizard project) is the first that has support for running under JDK9: https://github.com/dropwizard/metrics/releases/tag/v4.0.0 When running semantic-metrics under JDK9+, a warning (at DEBUG level) is emitted each time that FastForwardReporter emits metrics: ``` 20:29:01.073 DEBUG [fast-forward-reporter-0] MetricsModule: Failed to get metrics for FileDescriptorGaugeSet java.lang.reflect.InaccessibleObjectException: Unable to make public long com.sun.management.internal.OperatingSystemImpl.getOpenFileDescriptorCount() accessible: module jdk.management does not "opens com.sun.management.internal" to unnamed module @60438a68 at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:340) at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:280) at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:198) at java.base/java.lang.reflect.Method.setAccessible(Method.java:192) at com.codahale.metrics.jvm.FileDescriptorRatioGauge.invoke(FileDescriptorRatioGauge.java:48) at com.codahale.metrics.jvm.FileDescriptorRatioGauge.getRatio(FileDescriptorRatioGauge.java:35) at com.codahale.metrics.RatioGauge.getValue(RatioGauge.java:64) at com.spotify.apollo.metrics.MetricsModule$1.lambda$getMetrics$0(MetricsModule.java:113) at com.spotify.metrics.ffwd.FastForwardReporter.reportGauge(FastForwardReporter.java:252) at com.spotify.metrics.ffwd.FastForwardReporter.report(FastForwardReporter.java:214) at com.spotify.metrics.ffwd.FastForwardReporter.report(FastForwardReporter.java:202) at com.spotify.metrics.ffwd.FastForwardReporter$2.run(FastForwardReporter.java:373) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305) at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:834) ``` The only downside of this exception is that the FileDescriptionGaugeSet will report "NaN" values (as it has a fallback when exceptions are caught). v4.0 of the metrics library changed how FileDescriptorGaugeSet gets the underlying data to not need reflection.
|
Might be smart to bump the major version of semantic metrics as a part of this change - no API changes, but I think the semantic metrics library version number has usually loosely tracked the codahale metrics version number. |
|
worth noting that while the maven coordinates have changed to |
Java 7 reached end-of-life in April 2015, Java 8 itself is reaching end-of-life in January 2019 This change is motivated by the metrics 4.x library only being built for Java 8.
rename to avoid confusion with the existing FastForwardReporterTest. The class being tested is FastForwardHttpReporter, not FastForwardReporter.
the current test asserts that, after the reporter is started, `httpClient.sendBatch(..)` should be called at least twice. Since this method is called via something scheduled with a ScheduledExecutorService, the test can't 100% be sure how many times it will be called within a given time window. Sometimes this test fails because the method was called only once within the timeout, not at least twice. This change changes the assertion to be `atLeastOnce`, since the logic being tested does not really depend or care about how many times `httpClient.sendBatch(..)` is called - testing ScheduledExecutorService is not the goal of this test.
to mark the change in metrics library version being depended on
|
@mattnworb FYI I am looking at these now |
There was a problem hiding this comment.
@mattnworb some small observations (not necessarily blockers, just something I noticed):
memory=PS.*(i.e. memory=PS Survivor Space) is seemingly no longer published underwhat=jvm-memory-usage.- Nor is
gc=PS.*underwhat=jvm-gc-collection-time - Nor is
memory=Code Cacheunderwhat=jvm-memory-usage
Everything seems to run fine with these changes though! I just want to get to run by my team before approving.
|
what version of Java did you test this on? G1 is the default GC since Java 9 so I wonder if you are not seeing the “PS”-related metrics simply because the collector they are related to was not active in your test. |
|
I was testing on 11, so that makes sense |
|
Hey @mattnworb . I discussed with @dmichel1 . Can you make the version change to Once done we are comfortable approving this (as well as #29) , adding a |
The 4.0 release of the metrics library (which changed groupId, from the
original author to the Dropwizard project) is the first that has support
for running under JDK9:
https://github.com/dropwizard/metrics/releases/tag/v4.0.0
When running semantic-metrics under JDK9+, a warning (at DEBUG level) is emitted each
time that FastForwardReporter emits metrics:
The only downside of this exception is that the FileDescriptionGaugeSet
will report "NaN" values (as it has a fallback when exceptions are
caught).
v4.0 of the metrics library changed how FileDescriptorGaugeSet gets the
underlying data to not need reflection.