Add Cassandra driver Actuator metrics #2
Conversation
...ramework/boot/actuate/autoconfigure/metrics/cassandra/CassandraMetricsAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...ramework/boot/actuate/autoconfigure/metrics/cassandra/CassandraMetricsAutoConfiguration.java
Show resolved
Hide resolved
...c/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsSettings.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsSettings.java
Outdated
Show resolved
Hide resolved
...ramework/boot/actuate/autoconfigure/metrics/cassandra/CassandraMetricsAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...ork/boot/actuate/autoconfigure/metrics/cassandra/CassandraMetricsAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
...ramework/boot/actuate/autoconfigure/metrics/cassandra/CassandraMetricsAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/boot/actuate/metrics/cassandra/CassandraMetricsBinder.java
Outdated
Show resolved
Hide resolved
| }); | ||
| } | ||
|
|
||
| private long convertToTimeUnit(double value) { |
There was a problem hiding this comment.
You should create a unit or IT tests that are covering conversion of time, similar to:
https://github.com/riptano/java-driver-spring/blob/master/starter/src/test/java/com/datastax/oss/spring/configuration/metrics/DriverMetricsTest.java#L96-L158
There was a problem hiding this comment.
I will, if we decide to bring the config setting back.
| public void bindTo(@NonNull MeterRegistry meterRegistry) { | ||
| Assert.isTrue(this.session.getMetrics().isPresent(), "Metrics on the CqlSession needs to be present"); | ||
|
|
||
| this.session.getMetrics().get().getRegistry().getMetrics().forEach((name, m) -> { |
There was a problem hiding this comment.
to validate if we are correctly registering different types of metrics, we should create an IT test that checks at least one metric of every type. Maybe you could follow this test:
https://github.com/riptano/java-driver-spring/blob/master/integration-tests/src/test/java/com/datastax/oss/spring/configuration/exampletests/CassandraMetricsMicrometerIT.java
It iterates over all metrics and validates that they are registered properly. I was able to catch some bugs when implementing original version using this test.
There was a problem hiding this comment.
I've added some more example metrics to the test here.
|
@adutra and @tomekl007 I've added a few more tests and I think I've addressed the feedback. I'd appreciate one last quick check when you have a chance and then I'll move the PR to Spring's repo. |
| * on the classpath. | ||
| */ | ||
| @Test | ||
| void cassandraInstrumentationIsDisabledIfDriverClassesNotOnClasspath() { |
There was a problem hiding this comment.
I think that we should also test the case when MetricsRegistry AND CqlSession is not on the classpath. In that case the MeterRegistry.getBean should return null.
There was a problem hiding this comment.
It doesn't make sense to try to test with CqlSession filtered from the classpath. The ApplicationContextRunner tries to run with CassandraMetricsAutoConfiguration, which requires a CqlSession bean. If CqlSession isn't on the classpath, trying to spin up a context with CassandraMetricsAutoConfiguration will throw an exception indicating it can't find a required bean.
I can change the test so that it removes MetricRegistry from the classpath, but I'm mocking the CqlSession bean with a MetricRegistry since the tests don't have a fully initialized CqlSession bean with metrics configuration. I don't think removing MetricRegistry from the classpath at this point will prevent the metrics from being bound to the Micrometer MeterRegistry.
Or am I missing something?
There was a problem hiding this comment.
It doesn't make sense to try to test with CqlSession filtered from the classpath. The ApplicationContextRunner tries to run with CassandraMetricsAutoConfiguration, which requires a CqlSession bean. If CqlSession isn't on the classpath, trying to spin up a context with CassandraMetricsAutoConfiguration will throw an exception indicating it can't find a required bean.
It shouldn't throw an exception, it should instead not be executed as part of the configuration process, since that class has a @ConditionalOnClass({ MeterRegistry.class, CqlSession.class }) annotation.
This specific test does exactly that: it includes CassandraMetricsAutoConfiguration in the context, but excludes CqlSession from the classpath, to emulate the situation where the driver is not on the classpath.
I think what @tomekl007 wants is just another test similar to this one, but in which MeterRegistry would also be absent from the classpath, to emulate the situation where the user has excluded Micrometer and the driver from the classpath.
I personally don't think that this is a common situation, but imo it doesn't hurt to add a test for that.
There was a problem hiding this comment.
I understand the intent. But I'm seeing the exception when I run the tests if I don't provide a CqlSession bean to the test's ApplicationContextRunner, i.e.
this.contextRunner.withBean(CqlSession.class, () -> mockSession(true))If I omit the bean, I get a NoSuchBeanDefinitionException with
No qualifying bean of type 'com.datastax.oss.driver.api.core.CqlSession' available.
Does it make sense to create the context with the mocked bean, but test without CqlSession on the classpath?
There was a problem hiding this comment.
I think I see the problem....
There was a problem hiding this comment.
Ok, with the latest changes, I can spin up the context with CqlSession and MetricRegistry missing from the classpath. The Micrometer metrics are indeed disabled, however the Micrometer MeterRegisrty is not null, it simply has no meters in its map.
| void autoConfiguredCassandraIsInstrumented() { | ||
| this.contextRunner.withBean(CqlSession.class, () -> mockSession(true)).run((context) -> { | ||
| MeterRegistry registry = context.getBean(MeterRegistry.class); | ||
| // Assert a typical Driver Meter metric |
There was a problem hiding this comment.
nit: can we extract assertions of metrics to separate methods for readability? i.e:
https://github.com/datastax/spring-boot/pull/2/files#diff-8991f02ba760e2c1590560601a62f5a1R59-R61
to assertMeterMetric(), then assertGaugeMetric(), etc
|
Official PR opened here: spring-projects#21624. |
Rename `CompositeMeterRegistryAutoConfiguration` to `MeterRegistryAutoConfiguration` since it can also create non-composite registries. Closes spring-projectsgh-22988
Create a new `JarFileWrapper` class so that we can wrap and existing `JarFile` and offer a version that can be safely closed. Prior to this commit, we provided wrapper functionality in the `JarFile` class itself. Unfortunately, because we override `close` and also create a lot of wrappers this caused memory issues when running on Java 11. With Java 11 `java.util.zip.ZipFile` class uses `FinalizableResource` for any implementation that overrides `close()`. This means that any wrapper classes will not be garbage collected until the JVM finalizer thread runs. Closes spring-projectsgh-22991
This commit adds a check to the `layertools extract` command to ensure that the jar file being processed is readable and has a valid directory. Fixes spring-projectsgh-22993
Refine method visibility in an attempt to fix test issues on Java 11+. See spring-projectsgh-22991
Remove Mockto from JarFileWrapperTests since it seems to be failing on later versions of Java. See spring-projectsgh-22991
Update `Repackager` to ensure that `getLayout` is called before we backup the source file. This restores earlier behavior that some custom `ModuleFactory` implementations were relying on. Closes spring-projectsgh-22995
* pr/23002: Mention correct JUnit 5 annotations in Kotlin testing section Closes spring-projectsgh-23002
No description provided.