brettwooldridge / HikariCP

光 HikariCP・A solid, high-performance, JDBC connection pool at last.
Apache License 2.0
19.87k stars 2.92k forks source link

(MetricRegistry) fails when micrometer.io MeterRegistry is used #1234

Open orangefiredragon opened 6 years ago

orangefiredragon commented 6 years ago

In CodahaleHealthChecker.registerHealthChecks casting final MetricRegistry metricRegistry = (MetricRegistry) hikariConfig.getMetricRegistry(); does not consider that instance of getMetricRegistry() can be MeterRegistry from http://micrometer.io

Environment

HikariCP version: 3.1.0
JDK version     : 1.8.0_111
Database        : PostgreSQL
Driver version  : 42.2.4

⚠️ Please verify that your issue still occurs on the latest version of HikariCP before reporting.


Have you searched the CLOSED issues already? How about checking stackoverflow?

brettwooldridge commented 5 years ago

micrometer.o does not have inherent support for Codahale/DropWizard health checks. You cannot call setHealthCheckRegistry() with a micrometer registry.

devminded commented 5 years ago

This actually still happens. As I understand it one should be able to register a CodaHale HealthCheckRegistry healthcheck and a Micrometer MeterRegistry metrics collector at the same time. I might have misunderstood how to use it.

Gradle

dependencies {
    implementation 'com.zaxxer:HikariCP:3.3.1'
    implementation 'io.dropwizard.metrics:metrics-healthchecks:4.0.5'
    implementation 'io.prometheus:simpleclient:0.6.0'
    implementation 'io.micrometer:micrometer-registry-prometheus:1.1.3'
}

Code

private static HealthCheckRegistry startHealthChecks(ServletContext servletContext) {
    HealthCheckRegistry registry = new HealthCheckRegistry();
    registry.register("deadlocks", new ThreadDeadlockHealthCheck());
    servletContext.setAttribute("HEALTH_REGISTRY", registry);
    LOGGER.info("Health checks started.");
    return registry;
}

private static MeterRegistry startMetrics() {
    MeterRegistry metricsRegistry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT);
    new ClassLoaderMetrics().bindTo(metricsRegistry);
    new JvmMemoryMetrics().bindTo(metricsRegistry);
    new JvmGcMetrics().bindTo(metricsRegistry);
    new JvmThreadMetrics().bindTo(metricsRegistry);
    new ProcessorMetrics().bindTo(metricsRegistry);
    new LogbackMetrics().bindTo(metricsRegistry);
    new UptimeMetrics().bindTo(metricsRegistry);
    LOGGER.info("Instrumentation started.");
    return metricsRegistry;
}

private static Optional<HikariDataSource> createDataSource(Properties properties, HealthCheckRegistry healthCheckRegistry, MeterRegistry meterRegistry) {
    try {
        HikariConfig config = new HikariConfig(properties);
        config.setInitializationFailTimeout(-1);  // -1 = always start the pool
        config.setLeakDetectionThreshold(60000);
        config.setHealthCheckRegistry(healthCheckRegistry);
        config.setMetricRegistry(meterRegistry);
        return Optional.of(new HikariDataSource(config));
    } catch (Throwable t) {
        LOGGER.error("Could not create datasource.", t);
    }
    return Optional.empty();
}

Result

java.lang.ClassCastException: io.micrometer.prometheus.PrometheusMeterRegistry cannot be cast to com.codahale.metrics.MetricRegistry
  at com.zaxxer.hikari.metrics.dropwizard.CodahaleHealthChecker.registerHealthChecks(CodahaleHealthChecker.java:61)
  at com.zaxxer.hikari.pool.HikariPool.setHealthCheckRegistry(HikariPool.java:324)
  at com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:124)
  at com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
  at XYZ.createDataSource(XYZ.java:46)
  at ...
brettwooldridge commented 5 years ago

@orangefiredragon @devminded This is a valid point. At the time of original implementation, only Codahale/DropWizard had the concept of a health check. Therefore, health check was never abstracted in the same way of metrics where (via IMetricsTracker etc).

Pull requests welcome.

muddlebee commented 4 years ago

@brettwooldridge can I work on this? any pointers

devminded commented 4 years ago

@anweshknayak I have not done any work on this. In-fact, I forgot about it until now. I ended up working around it by registering/deregistering my own health checks myself, not the prettiest solution but I was in a hurry.