-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
Add Cassandra driver Actuator metrics #23008
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
* Copyright 2020 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.springframework.boot.actuate.autoconfigure.metrics.cassandra; | ||
|
||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
import com.datastax.oss.driver.api.core.CqlSession; | ||
import com.datastax.oss.driver.api.core.config.DefaultDriverOption; | ||
import com.datastax.oss.driver.api.core.metrics.DefaultNodeMetric; | ||
import com.datastax.oss.driver.api.core.metrics.DefaultSessionMetric; | ||
import com.datastax.oss.driver.internal.metrics.micrometer.MicrometerMetricsFactory; | ||
import io.micrometer.core.instrument.MeterRegistry; | ||
|
||
import org.springframework.boot.actuate.autoconfigure.metrics.MetricsAutoConfiguration; | ||
import org.springframework.boot.actuate.autoconfigure.metrics.export.simple.SimpleMetricsExportAutoConfiguration; | ||
import org.springframework.boot.autoconfigure.AutoConfigureAfter; | ||
import org.springframework.boot.autoconfigure.AutoConfigureBefore; | ||
import org.springframework.boot.autoconfigure.EnableAutoConfiguration; | ||
import org.springframework.boot.autoconfigure.cassandra.CassandraAutoConfiguration; | ||
import org.springframework.boot.autoconfigure.cassandra.CqlSessionBuilderCustomizer; | ||
import org.springframework.boot.autoconfigure.cassandra.DriverConfigLoaderBuilderCustomizer; | ||
import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; | ||
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; | ||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.context.annotation.Configuration; | ||
|
||
/** | ||
* {@link EnableAutoConfiguration Auto-configuration} for metrics on all available | ||
* {@link CqlSession Cassandra sessions}. | ||
* | ||
* @author Erik Merkle | ||
* @since 2.4.0 | ||
*/ | ||
@Configuration | ||
@AutoConfigureAfter({ MetricsAutoConfiguration.class, SimpleMetricsExportAutoConfiguration.class }) | ||
@AutoConfigureBefore({ CassandraAutoConfiguration.class }) | ||
@ConditionalOnClass({ MeterRegistry.class, MicrometerMetricsFactory.class }) | ||
@ConditionalOnBean({ MeterRegistry.class }) | ||
public class CassandraMetricsAutoConfiguration { | ||
|
||
@Bean | ||
public DriverConfigLoaderBuilderCustomizer cassandraMetricsConfigCustomizer() { | ||
return (builder) -> builder.withString(DefaultDriverOption.METRICS_FACTORY_CLASS, "MicrometerMetricsFactory") | ||
.withStringList(DefaultDriverOption.METRICS_SESSION_ENABLED, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tried to write some more test where someone would want to override those settings. Unfortunately that didn't work as calling
I think we need a way to support this use case, we can't blindly set those values without a way for users to customise them. Is there a way to know that a key has been registered already (and therefore not apply anything) or a way for users to override whatever value has been set? I'd prefer the former as it makes the auto-configuration backs off in a consistent way. If only the latter is possible, that would mean that the customizer should be ordered to run after the auto-configured one which is more work than it should IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, this is not ideal. Let me look into it a bit on the driver side. I added all of the metrics here, thinking that if a client application wants driver metrics enabled, there isn't a lot of harm in just turning them all on. And if driver metrics are not desired, the client would simply not enable the actuator. I'll come up with something that allows the metrics list to be overridden. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah but it's more to do with the fact that if the user has expressed an opinion, we must be able to see that and not apply any option. So it's not about overriding really, it's about knowing that something in that area was already configured and therefore the auto-configuration should not apply. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unfortunately the driver rejects duplicate configuration keys when building the final config. This was done mostly to mimic the behavior of preexisting Guava-backed maps. We could of course change or reimplement But for now: it seems that we should stick with Erik's suggestion: if you want Cassandra metrics in, add the And for the future: we plan to propose to change the driver configuration logic in SB to the more recent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened JAVA-2893 FYI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That wasn't my main request by the way. My main request was to be able to determine whether a key was already set or not.
Sorry but I am not keen to do that. The auto-configuration must not be an on/off switch. We should let users overrides some decisions one way or the other. We can explore other arrangement where this is possible but given that it is configured on the driver right now, we don't have a lot of options. I think also that's a general problem with the current approach. How do you compose driver configuration then? If the only thing you could do is set a property with no way to know whether or not it has been registered, this is quite limitative IMO. Have I missed something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With
Yes, the current programmatic config API wasn't our masterpiece, I admit. As I said, the plan is to evangelize around the new one ( The concrete consequence of this imo is that this PR cannot be merged for 2.4. I suggest that we table it for now and revisit it later, after the 4.10 release of the driver. |
||
Stream.of(DefaultSessionMetric.values()).map(DefaultSessionMetric::getPath) | ||
.collect(Collectors.toList())) | ||
.withStringList(DefaultDriverOption.METRICS_NODE_ENABLED, Stream.of(DefaultNodeMetric.values()) | ||
.map(DefaultNodeMetric::getPath).collect(Collectors.toList())); | ||
} | ||
|
||
@Bean | ||
public CqlSessionBuilderCustomizer cassandraMetricsBuilderCustomizer(MeterRegistry registry) { | ||
return (builder) -> builder.withMetricRegistry(registry); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Copyright 2020 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
/** | ||
* Auto-configuration for Cassandra metrics. | ||
*/ | ||
package org.springframework.boot.actuate.autoconfigure.metrics.cassandra; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
/* | ||
* Copyright 2020 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.springframework.boot.actuate.autoconfigure.metrics.cassandra; | ||
|
||
import java.time.Duration; | ||
|
||
import com.datastax.oss.driver.api.core.ConsistencyLevel; | ||
import com.datastax.oss.driver.api.core.CqlSession; | ||
import com.datastax.oss.driver.api.core.cql.SimpleStatement; | ||
import com.datastax.oss.driver.api.core.metrics.DefaultNodeMetric; | ||
import com.datastax.oss.driver.api.core.metrics.DefaultSessionMetric; | ||
import io.micrometer.core.instrument.MeterRegistry; | ||
import org.junit.jupiter.api.Test; | ||
import org.testcontainers.containers.CassandraContainer; | ||
import org.testcontainers.containers.wait.CassandraQueryWaitStrategy; | ||
import org.testcontainers.junit.jupiter.Container; | ||
import org.testcontainers.junit.jupiter.Testcontainers; | ||
|
||
import org.springframework.boot.actuate.autoconfigure.metrics.test.MetricsRun; | ||
import org.springframework.boot.autoconfigure.AutoConfigurations; | ||
import org.springframework.boot.autoconfigure.cassandra.CassandraAutoConfiguration; | ||
import org.springframework.boot.test.context.runner.ApplicationContextRunner; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
/** | ||
* Tests for {@link CassandraMetricsAutoConfiguration}. | ||
* | ||
* @author Erik Merkle | ||
*/ | ||
@Testcontainers(disabledWithoutDocker = true) | ||
public class CassandraMetricsAutoConfigurationIntegrationTests { | ||
|
||
@Container | ||
static final CassandraContainer<?> cassandra = new CassandraContainer<>().withStartupAttempts(5) | ||
.withStartupTimeout(Duration.ofMinutes(10)).waitingFor(new CassandraQueryWaitStrategy()); | ||
|
||
private final ApplicationContextRunner contextRunner = new ApplicationContextRunner().with(MetricsRun.simple()) | ||
.withConfiguration( | ||
AutoConfigurations.of(CassandraMetricsAutoConfiguration.class, CassandraAutoConfiguration.class)) | ||
.withPropertyValues( | ||
"spring.data.cassandra.contact-points:" + cassandra.getHost() + ":" | ||
+ cassandra.getFirstMappedPort(), | ||
"spring.data.cassandra.local-datacenter=datacenter1", "spring.data.cassandra.read-timeout=20s", | ||
"spring.data.cassandra.connect-timeout=10s"); | ||
|
||
/** | ||
* Cassandra driver metrics should be enabled by default as long as the desired | ||
* metrics are enabled in the Driver's configuration. | ||
*/ | ||
@Test | ||
void autoConfiguredCassandraIsInstrumented() { | ||
this.contextRunner.run((context) -> { | ||
MeterRegistry registry = context.getBean(MeterRegistry.class); | ||
// execute queries to peg metrics | ||
CqlSession session = context.getBean(CqlSession.class); | ||
SimpleStatement statement = SimpleStatement.newInstance("SELECT release_version FROM system.local") | ||
.setConsistencyLevel(ConsistencyLevel.LOCAL_ONE); | ||
for (int i = 0; i < 10; ++i) { | ||
assertThat(session.execute(statement).one()).isNotNull(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need to loop 10 times here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I loop here just to peg the request count more than once and verify in the assertion below. 10 was just some arbitrary value that was more than 1. |
||
} | ||
// assert Session metrics | ||
String sessionMetricPrefix = session.getContext().getSessionName() + "."; | ||
assertThat( | ||
registry.get(sessionMetricPrefix + DefaultSessionMetric.CONNECTED_NODES.getPath()).gauge().value()) | ||
.isEqualTo(1d); | ||
assertThat(registry.get(sessionMetricPrefix + DefaultSessionMetric.CQL_REQUESTS.getPath()).timer().count()) | ||
.isEqualTo(10L); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was a bit confused by that. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testcontainers wait strategy does execute a query, whereas the wait strategy implementation in the Spring Boot Cassandra Autoconfigure test does not. The query is basic (it just queries the database version) and should succeed when the Cassandra cluster is up and ready. If the query fails or times out, it's likely due to the cluster not being available, which would mean that this integration test likely won't succeed either. The wait strategy implementation I copied and removed just waits for a Cassandra session builder to be built, which doesn't guarantee that the cluster is ready to respond to queries (although I haven't had that happen in practice locally). Maybe the ideal solution is to update the wait strategy implementation in Testcontainers so that it is more similar to the implementation in the Spring Boot test here, and then just use that as necessary for Spring Boot Cassandra ITs? |
||
assertThat(registry.get(sessionMetricPrefix + DefaultSessionMetric.BYTES_SENT.getPath()).counter().count()) | ||
.isGreaterThan(1d); | ||
assertThat( | ||
registry.get(sessionMetricPrefix + DefaultSessionMetric.BYTES_RECEIVED.getPath()).counter().count()) | ||
.isGreaterThan(1d); | ||
// assert Node metrics | ||
String nodeMetricPrefix = sessionMetricPrefix + "nodes." + cassandra.getHost() + ":" | ||
+ cassandra.getMappedPort(9042) + "."; | ||
assertThat(registry.get(nodeMetricPrefix + DefaultNodeMetric.BYTES_SENT.getPath()).counter().count()) | ||
.isGreaterThan(1d); | ||
|
||
}); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can import and use the class here,
@ConditionalOnClass
is evaluated without loading the class itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the class name here because the driver configuration is looking for that, not the class itself. Are you suggesting that I do something like:
I could do that if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just a note for you. See this comment. And yes, it's better to use the class than a "magic string". I've already polished this locally so no need to do anything.