Add Cassandra driver Actuator metrics #23008
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| /** | ||
| * {@link BeanPostProcessor} that configures Cassandra metrics. This is necessary as the | ||
| * CqlSessionBuilder bean provided by {@link CassandraAutoConfigure} must be overridden in | ||
| * order to enable driver metrics. The post-processing here will do just that. |
There was a problem hiding this comment.
There is a CqlSessionBuilderCustomizer that should be used for this. Please adapt the contract in the Cassandra API so that an override of the builder is not necessary.
There was a problem hiding this comment.
Thank you for the review. Currently, metrics binding in the driver happens a little deeper than the Session builder level (at the DriverContext level). I'll look into trying to modify the driver code to allow for using the Customizer.
| static class SimpleDriverConfigLoaderBuilderMetricsConfig { | ||
|
|
||
| @Bean | ||
| DriverConfigLoaderBuilderCustomizer customizer() { |
There was a problem hiding this comment.
If that represents what users have to do in order to get metrics support, I think something is missing.
There was a problem hiding this comment.
For Driver 4.x, metrics are not enabled by default. To enable metrics in the driver, the driver must be configured with the list of enabled metrics. This can be done programmatically, with System properties or by providing a driver config file. For this test, it was easiest to just provide a Driver config customizer bean. I can change the test to enable the metrics under test via System properties or with an external driver configuration file if you prefer.
Are you concerned with how metrics are enabled in the driver? Or should I be enabling metrics automatically in the Actuator?
There was a problem hiding this comment.
Are you concerned with how metrics are enabled in the driver? Or should I be enabling metrics automatically in the Actuator?
If we provide first-class support for metrics here, the user shouldn't have to write code to enable it ideally. Looking at other metrics we have, they are usually enabled by default, sometimes with a flag that indicates if metrics for that particular CqlSession is enabled or not. Concretely we should then have a enabled property somewhere in the spring.cassandra namespace that user can set in application.properties the usual way.
I don't have an opinion as whether the flag should be enabled by default or not but listing the metrics seem a bit tedious and inconsistent for an "out-of-the-box" scenario.
Paging @shakuzen to get some more feedback on this.
There was a problem hiding this comment.
I'm not very familiar with Cassandra driver specifics, but it's probably only metrics on specific queries which might have high cardinality or privacy concerns (if query parameters/values are tagged, for example) that we wouldn't want to enable by default, I think. Connection pool or node or session metrics seem generally useful and probably safe from the aforementioned concerns.
snicoll
added
the
status: waiting-for-feedback
|
@emerkle826 how is it going? Do you have an update for this change? Thanks! |
|
@snicoll I should have an update to this PR in a day or 2. I was waiting for the 4.9.0 release of the DataStax driver, which now has the ability to bind in the metrics with a Customizer. |
spring-projects-issues
added
status: feedback-provided
| }); | ||
| } | ||
|
|
||
| static final class CassandraWaitStrategy extends AbstractWaitStrategy { |
There was a problem hiding this comment.
Unrelated but rather than copy/pasting this, I wonder if it wouldn't be better if Testcontainers had some sort of support for this. Feel free to chime in in the issue I've just created.
There was a problem hiding this comment.
Testcontainers does heave a Cassandra wait strategy implementation here which is pretty similar to the implementation I copied. I could swap the implementations in Spring for the one Testcontainers provides.
That said, I have an issue to try to update Testcontainers from Driver 3.x to Driver 4.x here. My PR is under review, though it hasn't gotten a lot of traction. The reason I mention it is because I haven't yet addressed the Driver 3.x API tie in that is used in their wait strategy. From a Testcontainer perspective, they would prefer to remove the coupling to specific Cassandra Driver APIs (so driver version changes in the future don't impact Testcontainers). Providing a Driver API agnostic wait strategy would have to be included to achieve that.
snicoll
added
type: enhancement
There was a problem hiding this comment.
Thanks for the update and the change in the API. This is much simpler now and more aligned with what I was expecting.
I've tried to write some more tests as I want to make sure that users can easily override whatever the auto-configuration configures by default but I hit a problem. Can you please take a look and guide me?
|
|
||
| @Bean | ||
| public DriverConfigLoaderBuilderCustomizer cassandraMetricsConfigCustomizer() { | ||
| return (builder) -> builder.withString(DefaultDriverOption.METRICS_FACTORY_CLASS, "MicrometerMetricsFactory") |
There was a problem hiding this comment.
You can import and use the class here, @ConditionalOnClass is evaluated without loading the class itself.
There was a problem hiding this comment.
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:
return (builder) -> builder.withString(DefaultDriverOption.METRICS_FACTORY_CLASS, MicrometerMetricsFactory.class.getName())I could do that if you prefer.
There was a problem hiding this comment.
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.
| @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.
I've tried to write some more test where someone would want to override those settings. Unfortunately that didn't work as calling withString with the same key in another customizer leads to:
Caused by: java.lang.IllegalArgumentException: Duplicate key datastax-java-driver.advanced.metrics.session.enabled
at com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap$Builder.failIfDuplicateKeys(NullAllowingImmutableMap.java:226)
at com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap$Builder.build(NullAllowingImmutableMap.java:198)
at com.datastax.oss.driver.internal.core.config.typesafe.DefaultProgrammaticDriverConfigLoaderBuilder.buildConfig(DefaultProgrammaticDriverConfigLoaderBuilder.java:263)
at com.datastax.oss.driver.internal.core.config.typesafe.DefaultProgrammaticDriverConfigLoaderBuilder.lambda$build$2(DefaultProgrammaticDriverConfigLoaderBuilder.java:248)
at com.datastax.oss.driver.internal.core.config.typesafe.DefaultDriverConfigLoader.<init>(DefaultDriverConfigLoader.java:131)
at com.datastax.oss.driver.internal.core.config.typesafe.DefaultDriverConfigLoader.<init>(DefaultDriverConfigLoader.java:117)
at com.datastax.oss.driver.internal.core.config.typesafe.DefaultProgrammaticDriverConfigLoaderBuilder.build(DefaultProgrammaticDriverConfigLoaderBuilder.java:245)
at org.springframework.boot.autoconfigure.cassandra.CassandraAutoConfiguration.cassandraDriverConfigLoader(CassandraAutoConfiguration.java:114)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:154)
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.
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.
And if driver metrics are not desired, the client would simply not enable the actuator.
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.
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?
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 ProgrammaticDriverConfigLoaderBuilder to allow keys to be overridden. Just for completeness, I will open a Java driver ticket to explore that idea. But I'm wondering if it would do more harm than good: allowing keys to be overridden can lead to non-deterministic results. Can Spring Boot guarantee a 100% deterministic configuration order so that it would be safe to override a config key?
But for now: it seems that we should stick with Erik's suggestion: if you want Cassandra metrics in, add the java-driver-metrics-micrometer dependency and let SB configure it for you, otherwise, simply exclude the java-driver-metrics-micrometer dependency.
And for the future: we plan to propose to change the driver configuration logic in SB to the more recent OptionsMap API. It's 100% in-memory and type-safe. Note that this API accepts duplicate keys. We don't have an ETA for that though because this API has a few shortcomings that will be addressed in a future 4.10 driver release. (Also, I don't know if this would create a binary compatibility problem with SB 2.4 – but I digress).
There was a problem hiding this comment.
We could of course change or reimplement ProgrammaticDriverConfigLoaderBuilder to allow keys to be overridden.
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.
But for now: it seems that we should stick with Erik's suggestion
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.
How do you compose driver configuration then?
With DriverConfigLoader.compose.
this is quite limitative IMO.
Yes, the current programmatic config API wasn't our masterpiece, I admit. As I said, the plan is to evangelize around the new one (OptionsMap).
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.
| 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.
I was a bit confused by that. The waitStrategy runs some CQL requests as well, doesn't it? That makes this test a bit fragile IMO.
There was a problem hiding this comment.
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?
| 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.
Why do you need to loop 10 times here?
There was a problem hiding this comment.
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.
snicoll
added
the
status: waiting-for-feedback
|
I should also mention that I've started to polish the proposal already so there is no need to push additional commits. I'd like some feedback on what API I should be using so that users can override those settings if they want to. |
|
@emerkle826 @adutra any feedback on the review for me? In particular, not being able to see if the user has configured something is very annoying so I could use your insights. Thank you! |
|
@emerkle826 @adutra how is it going? Sorry for the ping again but if we want this feature to land in 2.4 we really need to find a solution for the problem described in the review. Can I help? |
snicoll
added
status: blocked
|
@adutra thanks for the feedback. I've moved this issue out of the |
|
@emerkle826 @adutra any feedback for this issue? An alternative would be to close this issue and another proposal can be submitted once the underlying infrastructure is available. |
snicoll
added
the
status: waiting-for-feedback
|
Hi, sorry for taking so long to chime in. First of all, driver 4.10 was released last week, and would really recommend that Spring Boot upgrades to that version. With 4.10 out, I would recommend switching from Once this is done, I think we could finish the work here (I say "think" because I didn't quite understand the concrete implications of the reservations expressed here). All in all, I would say that it's probably wiser to close this PR and we'll get back to it when possible. |
spring-projects-issues
added
status: feedback-provided
|
Alright, let's close it now. Let me know when you want to give this one another go. As for the reservation, if we auto-configure something, the user should have a way to be able to override this one way or the other. As long as this is possible, there is no problem but what I've experienced so far didn't let me do this (there are some details in this very issue). |
snicoll
added
status: declined
Motivation:
The DataStax Java driver for Cassandra can be configured to expose driver session operational metrics. Currently these metrics are not available through Spring Boot Actuator.
Modifications:
This commit adds support for Cassandra driver metrics through Spring Boot Actuator by enabling the Cassandra driver's Micrometer metrics module.
Result:
Users are now able to access Cassandra driver enabled metrics through Actuator.
NOTE: This PR uses the Cassandra driver's native Micrometer metrics as opposed to the previous approach here.