Skip to content
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

bump dropwizard-metrics to 4.0.2 #943

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@jifwin
Copy link

commented Feb 3, 2018

No description provided.

@datastax-bot

This comment has been minimized.

Copy link
Member

commented Feb 3, 2018

Hi @jifwin, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

Sincerely,
DataStax Bot.

@datastax-bot

This comment has been minimized.

Copy link
Member

commented Feb 3, 2018

Thank you @jifwin for signing the Contribution License Agreement.

Cheers,
DataStax Bot.

@jifwin

This comment has been minimized.

Copy link
Author

commented Feb 3, 2018

Any idea on why DowngradingConsistencyRetryPolicyIntegrationTest.should_try_next_host_on_connection_error fails? It doesn't seem to be related with changes I made.

@olim7t

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Hi,

Unfortunately this change breaks backward compatibility: Metrics 4 requires Java 8, but we still support Java 6 in our 3.x series. Our next major version, 4.0.0 (currently in late alpha stage) will require Java 8, and depend on Metrics 4 out of the box.

In the meantime, if you really need something in Metrics 4, I think it's possible to upgrade from your application code. Add the dependency to the new version, and disable the built-in JMX reporting with Cluster.builder().withoutJMXReporting(). If you still want JMX you can initialize the reporter manually: JmxReporter.forRegistry(cluster.getMetrics().getRegistry())...
(N.B: I haven't fully tested this approach, but from the Metrics release notes it looks like there are no other runtime incompatibilities)

@NersesAM

This comment has been minimized.

Copy link

commented Feb 20, 2018

Just leaving my comment here for maintainers. Not sure where else to do that.

I would personally like to have this merged even if there wasn't java 6 compatibility issue. But I don't think it is the right thing to do for such a core library as cassandra driver. Lots of users will not be able to upgrade to v4 because it is very new and not all libraries are compatible with it yet, jut like this driver.

I think the whole metrics thing needs to be revisited. It is better to provide hooks that people can plug in whatever metrics library they choose to use. Some default implementations can be provided, but the driver itself must not depend on any library. Dropwizard metrics v5 yet again has changed the top level package and if we want to avoid mess with versions, driver should not be dependant on any metrics lib.

@tolbertam

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

It is better to provide hooks that people can plug in whatever metrics library they choose to use.

Very recently with the release of java driver 4.0.0-alpha3 we introduced a metrics component that is more featureful and extensible than the existing one in 3.x. The interfaces provided are not strictly dependent on dropwizard metrics (although we use similar terms in our interfaces, see here), so in theory one could develop and plug-in an alternative implementation that uses a different metrics library.

I am curious though, are there popular alternatives to dropwizard metrics? I'm so accustomed myself to using it and it's always been adequate such that i've never explored alternatives. I don't expect that we would provide alternative implementations, unless there are popular alternatives that users are asking for.

driver should not be dependant on any metrics lib.

The driver's use of dropwizard metrics is not a hard dependency per se. While we list it in our maven dependencies, if you pass withoutMetrics along with your Cluster.Builder, the metrics library is never engaged, so you could technically use the driver without it. We should consider listing it as an optional dependency in 4.x, especially since metrics are opt-in with 4.x.

@NersesAM

This comment has been minimized.

Copy link

commented Feb 21, 2018

I am curious though, are there popular alternatives to dropwizard metrics? I'm so accustomed myself to using it and it's always been adequate such that i've never explored alternatives. I don't expect that we would provide alternative implementations, unless there are popular alternatives that users are asking for.

Micrometer. Spring Boot 2 is moving their default metrics library from DropWizard to Micrometer. Once Spring Boot 2 adoption picks up micrometer will go to masses. And breaking changes in dropwizard library itself makes a clear case for not having it as a hard requirement if one wants to have metrics.

I couldn't find Cluster.Builder equivalent in 4.x branch. Seems you are going to rely on typesafe config. Will it be possible to implement my own implementation of MetricUpdaterFactory and use for example Micrometer instead of Dropwizard?

Also you are using HDRHistogram library in 4.x branch. FYI Dropwizard v4 comes with a new Reservoir which you can consider using instead of HDR (or make it configurable). (SlidingTimeWindowArrayReservoir)[http://metrics.dropwizard.io/4.0.0/manual/core.html#sliding-time-window-reservoirs]

@olim7t

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

I couldn't find Cluster.Builder equivalent in 4.x branch.

We've merged Cluster and Session in 4.x. The equivalent is CqlSessionBuilder (but yes, most of the config is through Typesafe now).

Will it be possible to implement my own implementation of MetricUpdaterFactory and use for example Micrometer instead of Dropwizard?

Yes, at first sight I don't see anything preventing it, we should definitely explore this further (I've created JAVA-1759 to that effect). The dependency to Dropwizard can't be completely eliminated because of Session.getMetricsRegistry(), but an unused metrics registry is not a big overhead.

FYI Dropwizard v4 comes with a new Reservoir which you can consider using instead of HDR

SlidingTimeWindowArrayReservoir stores every datapoint, which wouldn't work well in our high-throughput environment. HDR is more appropriate for our use case.

@tolbertam

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

Micrometer. Spring Boot 2 is moving their default metrics library from DropWizard to Micrometer.

Neat, I was not aware of this library, it looks like it GA'd this week, so it's still very new but I agree it could see a lot of traction. 👍 for looking into ways so supporting alternative metrics libraries in java driver 4.x.

@tolbertam

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

Given @olim7t 's comment about Metrics 4.x's dependency on Java 8, I think we will stick to Metrics 3.2 as a dependency for now, so I'm closing this.

However as also mentioned, Metrics 4.x will work with the driver with some caveats. Since I suspect a number of users will want to do this, I added some instructions to our documentation via #958 for how to use Metrics 4.x with the driver. I experimenting a bit with this and it appears to work well.

@tolbertam tolbertam closed this Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.