Adds support for Dropwizard 5 metrics. #1962
Conversation
pom.xml
Outdated
| <hibernate.version>5.4.24.Final</hibernate.version> | ||
| <javassist.version>3.27.0-GA</javassist.version> | ||
| <jndi.version>0.11.4.1</jndi.version> | ||
| <maven.release.version>2.5.3</maven.release.version> | ||
| <metrics.version>3.2.5</metrics.version> | ||
| <metrics5.version>5.0.0-rc11</metrics5.version> |
There was a problem hiding this comment.
You might naturally question managing this dependency version on an RC, but they've been RC'ing this for over two years now, so I think this is appropriate.
There was a problem hiding this comment.
This is still the case as of June 2023. I advanced the RC version to 17 as of a51f146.
| @@ -452,6 +460,7 @@ | |||
| javax.sql.rowset.spi, | |||
| com.codahale.metrics;resolution:=optional, | |||
| com.codahale.metrics.health;resolution:=optional, | |||
| io.dropwizard.metrics5;resolution:=optional, | |||
There was a problem hiding this comment.
I wasn't quite sure what this was, but given the pre-existing codahale entries, I figured this was appropriate to add to help with your release process.
| * | ||
| * @author Brett Wooldridge | ||
| */ | ||
| public class CodahaleMetricsTest extends TestMetricsBase<MetricRegistry> |
There was a problem hiding this comment.
This has your pre-existing tests from the formerly-named TestMetrics that couldn't be made generic.
| /** | ||
| * Test HikariCP/Dropwizard 5 metrics integration. | ||
| */ | ||
| public class Dropwizard5MetricsTest extends TestMetricsBase<MetricRegistry> |
There was a problem hiding this comment.
Correspondingly, this has similarly-patterned tests, but for Dropwizard 5.
|
|
||
| /** | ||
| * Test HikariCP/CodaHale metrics integration. | ||
| * Test HikariCP metrics integration. | ||
| * | ||
| * @author Brett Wooldridge | ||
| */ | ||
| public class TestMetrics |
There was a problem hiding this comment.
Logically, this got split into to TestMetricsBase and CodahaleMetricsTest. Retained here is the single Codahale/Dropwizard-independent test (for the fake registry).
Rats. When I run that locally on my branch, it works. I'm not sure what's wrong. I didn't see further logging from Travis. Am I missing something? |
|
Hi @brettwooldridge , just a friendly ping on this. |
|
@pennello, The travis-ci build for this is failing, any idea why? |
|
@lfbayer Thanks for the reply. I checked the Travis logs and saw only the following:
Is there a way to get more diagnostic information out of the CI build? When I run that locally on my branch, it works. I've got to be missing something simple. |
pennello
force-pushed
the
dropwizard5-metrics
branch
from
5ab7eec
to
5bb97e4
Compare
This update avoids an issue in at least JDK 18 in which `java.net.URLStreamHandler.equals` was inaccessible due to module java.base not "opens java.net". See below for more detail. [ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.129 s <<< FAILURE! - in com.zaxxer.hikari.osgi.OSGiBundleTest [ERROR] com.zaxxer.hikari.osgi.OSGiBundleTest.checkBundle Time elapsed: 0.09 s <<< ERROR! java.lang.ExceptionInInitializerError Caused by: java.lang.RuntimeException: Unable to make protected boolean java.net.URLStreamHandler.equals(java.net.URL,java.net.URL) accessible: module java.base does not "opens java.net" to unnamed module @484b61fc Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make protected boolean java.net.URLStreamHandler.equals(java.net.URL,java.net.URL) accessible: module java.base does not "opens java.net" to unnamed module @484b61fc
Previously, this matches implementation was ignoring the `name` passed
in and instead comparing the *literal* `"testMetricWait.pool.Wait"` to
the dynamically-created
`MetricRegistry.name("testMetricWait", "pool", "Wait")`.
This commit updates the matches implementation to instead compare the
`name` passed in.
This brings it in line with the other implementations in the same file.
pennello
force-pushed
the
dropwizard5-metrics
branch
from
5bb97e4
to
a51f146
Compare
But not health checks yet, though.
pennello
force-pushed
the
dropwizard5-metrics
branch
from
a51f146
to
3f377c0
Compare
Hi @brettwooldridge ,
Here is a PR that adds support for Dropwizard 5 metrics alongside existing support for Codahale and Micrometer.
I tried to keep the code style consistent with the existing code, and with new tests, sought to keep common as many tests as possible for Codahale and Dropwizard 5, which are of course very similar, but also frustratingly different when shared in the same codebase. (I did this by having a test abstract base class and parameterizing the metric registry type.)
The existence of
IMetricsTrackermade adding Dropwizard 5 support relatively straightforward. But I didn't see a similar abstraction for health checking. Hence I didn't add support for that yet. I figured that this is still a strict improvement and would thus still be worth submitting to you.I also made some library upgrades in order to get tests passing in a Java 18 environment.
Please let me know what you think, or if you think this might need any revisions before it's appropriate to get into
dev.For context, my company has been working on a Jetty 11 upgrade, which has necessitated a number of other nontrivial and sometimes backwards-incompatible upgrades, including Resteasy, Jakarta (over
javax), and Dropwizard. Hence, to maintain the useful Hikari metrics, we wanted to add support here for Dropwizard 5.