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

Metrics configuration - allow MetricsTracker instance to be set #112

Closed
jiri-pejchal opened this issue Jul 17, 2014 · 12 comments
Closed

Metrics configuration - allow MetricsTracker instance to be set #112

jiri-pejchal opened this issue Jul 17, 2014 · 12 comments

Comments

@jiri-pejchal
Copy link

I've noticed there is some preliminary (although currently unsupported) implementation of metrics recording.

The MetricTracker can be choosen in the configuration by setting the metricsTrackerClassName property.

For configuration in spring - it would be nice to have an option to use dependency injection and set directly an instance of MetricsTracker (instead of a class).

@brettwooldridge
Copy link
Owner

Noted.

@john-f-xoom
Copy link

Hello, I, too, would like to make the metric tracker setup more Spring-friendly. I think this can be accomplished without necessarily pulling in a dependency on Spring, and in a few different ways. I've forked the repository and have started work on a patch to do what I need with a minimal amount of changes to the code, but do you have any guidance to offer for what direction you would like such a change to take? If not, I'll go ahead and submit a pull request with my patch, anyway, and hope for the best.

@brettwooldridge
Copy link
Owner

There is no reason why an implementer of IMetricsTracker couldn't be injected into the HikariConfig ... all that is needed an the addition of a getter/setter along with a small change to HikariPool (which currently calls MetricsFactory.createMetricsTracker()).

That said, it has yet to be decided how metrics should be exposed ... and what we would want the API would look like. Or whether exposure would be purely through JMX.

Also undetermined is the granularity of metrics. Currently there is no plan for statement-level metrics because of the potential impact on the performance of the code even when metrics recording is turned off.

But we would be interested to see/hear your ideas...

@john-f-xoom
Copy link

Hi, I'm glad to know that you're willing to consider a change along these lines! The injection of an IMetricsTracker implementation as you describe is roughly what I had in mind as the minimal change that would un-block me.

Our project is using Spring for dependency injection and Coda Hale (now Drop Wizard) for metrics, and the product is a library for use by other groups within our organization. I need to satisfy the following requirements:

  1. Associate two Coda Hale timers and a Coda Hale gauge with each connection pool. The timers collect the timings currently fed into IMetricsTracker, and the gauge tracks the number of active connections.
  2. Let clients use Spring to provide the metric registry instance with which to register each pool's timers and gauge.
  3. Have a way to determine the connection pool from the names of the associated metrics. This seems possible either by letting the client specify the metric names, or by honoring a consistent convention in naming the metrics that puts the connection pool's name into the names at a well-defined (and easily extractable) place.

In addition to these requirements, a goal is to use metrics-spring to register annotated fields and methods in beans automatically with a metrics registry provided by the client. If the registration turns out not to be so seamless, though, that's okay.

Note that none of the requirements, nor the goal, necessarily require incorporating classes from the metrics library in the Hikari API, but that could be part of a more comprehensive approach to metrics.

Our project currently doesn't have any dependencies on JMX, and we would prefer to keep it that way, but if Hikari winds up mandating JMX for metrics, we probably would wind up piping them into the metrics library along with our other metrics. I would ask, though, why the IMetricsTracker interface arose apart from the MBean: was it because exposing those timings through JMX would have been too difficult or ugly, or would it have lost information?

I think the gauge for active connections would duplicate one value that Hikari already exposes through JMX, but it seems our project could synthesize it in the implementation of IMetricsTracker as well. One way or the other, we want it going through Coda Hale/Drop Wizard metrics, even if we have to pull it from JMX first.

Stepping back a bit, I see metrics as somewhat analogous to logging, in that it's a way to provide operational information about code outside of the code's purpose-oriented interface, and libraries should allow clients to consolidate such information across all the libraries in use. Metrics seems a little more complex and less well-established than logging is: more complex because some processing at the point of collection can be significant for performance (and requires decisions such as what kind of reservoir to use for statistical sampling), and less well-established because there isn't an interface to them as ubiquitous as slf4j is for logging. If I were starting a new project in this context, I probably would enshrine the Coda Hale/Drop Wizard metrics API as the way to obtain metrics from the code, but I imagine there could be disadvantages to such an approach. I don't know much about JMX, but people seem to dislike it.

Anyway, I'll work on the minimal change for now, but I'm open to a more comprehensive approach, too.

@brettwooldridge
Copy link
Owner

@john-f-xoom thanks for the detail. Here is my feedback.

Re: requirements...

Associate two Coda Hale timers and a Coda Hale gauge with each connection pool.
The timers collect the timings currently fed into IMetricsTracker, and the gauge tracks
the number of active connections.

No problem.

Let clients use Spring to provide the metric registry instance with which to register each
pool's timers and gauge.

No problem.

Have a way to determine the connection pool from the names of the associated metrics
This seems possible either by letting the client specify the metric names, or by honoring a
consistent convention in naming the metrics that puts the connection pool's name into the
names at a well-defined (and easily extractable) place.

I prefer the naming convention approach.

In addition to these requirements, a goal is to use metrics-spring to register annotated fields and
methods in beans automatically with a metrics registry provided by the client. If the registration
turns out not to be so seamless, though, that's okay.

I'm not clear on what this means. Do you you mean that by annotating fields/methods, they somehow participate in metrics collection? I'm not clear on how that works ... code generation? While possibly supported by metrics-spring, it is unlikely that HikariCP would participate in collection through this mechanism.

I would ask, though, why the IMetricsTracker interface arose apart from the MBean: was it
because exposing those timings through JMX would have been too difficult or ugly, or would
it have lost information?

IMetricsTracker serves two purposes. First, in HikariCP metrics collection is optional, and will probably always be. So having a direct [hard] reference to CodaHale classes in HikariPool would create a runtime dependency. IMetricsTracker provides a "buffer" that removes that dependency.

Second, here we get into a micro-optimization discussion, but it is probably important that you understand the lengths that we have gone for performance. Imagine the following code:

public Connection getConnection() {
   try {
      if (isRecordMetrics) {
         codaHaleConnectionTimer.time();
      }
      // obtain connection and return
   }
   finally {
      if (isRecordMetrics) {
         codaHaleConnectionTimer.stop();
      }
   }
}

Even when metrics are disabled, you are paying the penalty of two boolean checks. What we have instead is (roughly) this:

public Connection getConnection() {
   try {
      metricsTracker.recordConnectionRequest()
      // obtain connection and return
   }
   finally {
       metricsTracker..stop();
   }
}

This might seem like it would perform worse, because the methods are always being invoked. But when metrics collection is disabled, the implementation of IMetricsTracker contains only empty methods. The JIT dead-code elimination routines recognize this and and completely remove the calls, resulting in zero runtime impact -- as if those lines of code were never there.

The fact that IMetricsTracker "arose apart from the MBean" has nothing to do with JMX. MBean does not provide for the kind of metrics aggregation that CodaHale does -- histograms, gauges, etc. However, CodaHale does have the option to ultimately expose metrics though the JMX interface.

If I were starting a new project in this context, I probably would enshrine the Coda Hale/Drop
Wizard metrics API as the way to obtain metrics from the code, but I imagine there could be
disadvantages to such an approach.

In terms of a runtime dependency, an API that returns CodaHale instances does not induce a runtime requirement -- as long as the user never invokes against that API. Currently we have no API to expose metrics, but exposing them explicitly as CodaHale classes is probably the right way to go. On the collection-side, as noted above, we have to isolate CodaHale somewhat to avoid a dependency when a user choses not to use them.

I don't know much about JMX, but people seem to dislike it.

Like it or not, there are many third-party monitoring systems and tools that take advantage of JMX. Exposing metrics via JMX is not optional, it is a requirement. Luckily it is "free" as CodaHale inherently supports it. But that doesn't mean that direct programatic access via API shouldn't also be provided.

The other thing that JMX provides is discoverability. If you have several applications running in separate containers, they each have different ClassLoader scopes. Even if HikariCP (or CodaHale) provided a "static" registry, that registry would be "scoped" to that container (ClassLoader hierarchy). A monitoring agent that is "in-process" but "outside" of the container has no way to discover pools and their associated metrics. Because the JMX registry resides in the system ClassLoader it is global to the process, which allows pools, etc. to be discovered from "outside", across all containers. Finally, JMX provides true remote monitoring over TCP. Which is why it is widely used by enterprise monitoring solutions.

API access only makes sense for "single ClassLoader hierarchy" environments, such as standalone applications, but is largely unfeasible in application container environments such as servlet containers or JEE containers.

@brettwooldridge
Copy link
Owner

Looking further into metrics-spring, CodaHale, Spring, and HikariCP, I think what you actually need is a way to inject a MetricRegistry, not an instance of IMetricsTracker. And metrics-spring is altogether unnecessary (for HikariCP).

I have added code that allows you to access the IMetricsTracker from the HikariDataSource. This can be cast to a CodaHaleMetricsTracker, which is publishing the following API:

Timer getConnectionAcquisitionTimer();
Histogram getConnectionDurationHistogram();

But of course, using the API isn't necessary if you've injected the MetricRegistry from Spring. If you've injected the MetricRegistry, you can access the Timer, Histogram, etc. from the registry itself at pre-determined names (eg. "<poolname>-connectionWait").

@chrisvest
Copy link

You may want to support LatencyUtils as an alternative implementation, and not just Coda's Metrics: https://github.com/LatencyUtils/LatencyUtils

@brettwooldridge
Copy link
Owner

@chrisvest there are a few issues with using LatencyUtils. First and foremost, it only provides a Histogram, but does not support other types such as Timer or Gauge. Secondarily, the Histogram that it uses is not designed for high-concurrency. In the base-case, it is not thread-safe. It is unclear to me whether LatencyUtils uses the non-threadsafe version internally or not. The thread-safe version that is offered by HdrHistogram, while designed for threadsafety, is not particularly tuned for high-performance.

@john-f-xoom
Copy link

Hi, well, I went ahead with the pull request, anyway. It does what I need, and your earlier response actually led me to prefer it over enshrining Coda Hale in the API in this case. Of course, you're free to decline the pull request, but I'll work on a code sample that shows how I would like to use it with metrics-spring: I find it more convenient than obtaining the timer and histogram from the Coda Hale specialization of IMetricsTracker, but that might be just a matter of taste. The production code I have using Hikari in this manner has a bunch of other stuff that would obscure the example.

@eskatos
Copy link

eskatos commented Nov 3, 2014

Such a change would allow closer integration with other frameworks, not only with Spring.
👍

@brettwooldridge
Copy link
Owner

The tact that we are taking in the upcoming release of 2.2.0 is to offer a HikariConfig.setMetricRegistry(MetricRegistry) method. Thus currently only Codahale (Dropwizard) Metrics are supported at this time. If an alternative metrics suite emerges that offers the requiste metrics, we will create a polymorphic setter for that library.

@eskatos
Copy link

eskatos commented Nov 3, 2014

Cool.
This would fit very well in my projects (using Codahale/Dropwizard).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants