aerogear / keycloak-metrics-spi

Adds a Metrics Endpoint to Keycloak
Apache License 2.0
526 stars 151 forks source link

Seems incompatible with Keycloak 21 #155

Closed JakeTheSnake closed 1 year ago

JakeTheSnake commented 1 year ago

Description

After having migrated to Keycloak 21 (Quarkus) I get "Internal Server Error" whenever I try to login. The logs show messages like these:

ERROR [io.quarkus.vertx.http.runtime.QuarkusErrorHandler] (executor-thread-7) HTTP Request to /auth/realms/jake/metrics failed, error id: fdebc248-5921-4c51-8db2-2bcbcff3607e-57: org.jboss.resteasy.spi.UnhandledException: java.lang.IllegalArgumentException: Failed to register Collector of type Counter: keycloak_login_attempts_total is already in use by another Collector of type Counter

Keycloak 21 changed metrics implementation from SmallRye to Micrometer, maybe this clashes with this project somehow?

Steps to reproduce

  1. Install and enable keycloak-metrics-spi v2.5.3 on Keycloak 20
  2. Migrate to Keycloak 21
  3. Attempt to login to the admin console
logopk commented 1 year ago

I noticed this too, but I thought it might only affect the metrics call from prometheus (thus I disabled the provider).

logopk commented 1 year ago

Caused by #92

JaniszM commented 1 year ago

Same problem here 👍

Ekouyoja commented 1 year ago

Confirm, same problem here

fawadasaurus commented 1 year ago

It appears that key cloak now natively incudes a /metrics endpoint in prometheus format with Keycloak 21 so it may be that this project is no longer needed for Keycloak 21+: https://www.keycloak.org/2023/02/keycloak-2100-released.html

logopk commented 1 year ago

I don't see any metrics for specific realms in there - just the ones that existed before (strangely more or less consitently renamed ) .

gbatalski commented 1 year ago

There are no keycloak specific metrics within the default implementation, so these metrics were different

sschnabe commented 1 year ago

IHMO this repository is abandoned.

We relied on this implementation, but decided to rewrite it for Keycloak 21: kokuwaio/keycloak-event-metrics This implementation adds event metrics to Micrometer from Quarkus at /metrics.

Differences:

Feel free to provide feedback.

Bilbergia commented 1 year ago

The problem seems to be that with Keycloak 21, now both Keycloak and keycloak-metrics-spi try to initialize a prometheus exporter with HotSpot metrics.

If you start Keycloak 21.0.0 with both metrics enabled and this extension, the first Exception is

2023-03-04 16:24:03,682 ERROR [org.keycloak.services.error.KeycloakErrorHandler] (executor-thread-0) Uncaught server error: java.lang.IllegalArgumentException: Failed to register Collector of type ClassLoadingExports: The Collector exposes the same name multiple times: jvm_classes_loaded at io.prometheus.client.CollectorRegistry.assertNoDuplicateNames(CollectorRegistry.java:71) at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:51) at io.prometheus.client.Collector.register(Collector.java:308) at io.prometheus.client.hotspot.DefaultExports.register(DefaultExports.java:43) at io.prometheus.client.hotspot.DefaultExports.initialize(DefaultExports.java:28) at org.jboss.aerogear.keycloak.metrics.PrometheusExporter.<init>(PrometheusExporter.java:209) at org.jboss.aerogear.keycloak.metrics.PrometheusExporter.instance(PrometheusExporter.java:214) at org.jboss.aerogear.keycloak.metrics.MetricsFilter.filter(MetricsFilter.java:58) at org.jboss.aerogear.keycloak.metrics.MetricsFilterProvider.filter(MetricsFilterProvider.java:22) at org.jboss.resteasy.core.interception.jaxrs.ContainerResponseContextImpl.filter(ContainerResponseContextImpl.java:367) at org.jboss.resteasy.core.ServerResponseWriter.executeFilters(ServerResponseWriter.java:252) at org.jboss.resteasy.core.ServerResponseWriter.writeNomapResponse(ServerResponseWriter.java:101) at org.jboss.resteasy.core.ServerResponseWriter.writeNomapResponse(ServerResponseWriter.java:74) at org.jboss.resteasy.core.SynchronousDispatcher.writeResponse(SynchronousDispatcher.java:594) at org.jboss.resteasy.core.SynchronousDispatcher.invoke(SynchronousDispatcher.java:524) at org.jboss.resteasy.core.SynchronousDispatcher.lambda$invoke$4(SynchronousDispatcher.java:261) at org.jboss.resteasy.core.SynchronousDispatcher.lambda$preprocess$0(SynchronousDispatcher.java:161) at org.jboss.resteasy.core.interception.jaxrs.PreMatchContainerRequestContext.filter(PreMatchContainerRequestContext.java:364) at org.jboss.resteasy.core.SynchronousDispatcher.preprocess(SynchronousDispatcher.java:164) at org.jboss.resteasy.core.SynchronousDispatcher.invoke(SynchronousDispatcher.java:247) at io.quarkus.resteasy.runtime.standalone.RequestDispatcher.service(RequestDispatcher.java:73) at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.dispatch(VertxRequestHandler.java:151) at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.handle(VertxRequestHandler.java:82) at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.handle(VertxRequestHandler.java:42) at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1284) at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:173) at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:140) at io.quarkus.vertx.http.runtime.StaticResourcesRecorder$2.handle(StaticResourcesRecorder.java:84) at io.quarkus.vertx.http.runtime.StaticResourcesRecorder$2.handle(StaticResourcesRecorder.java:71) at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1284) at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:173) at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:140) at io.quarkus.vertx.http.runtime.VertxHttpRecorder$6.handle(VertxHttpRecorder.java:430) at io.quarkus.vertx.http.runtime.VertxHttpRecorder$6.handle(VertxHttpRecorder.java:408) at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1284) at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:173) at io.vertx.ext.web.impl.RoutingContextImpl.next(RoutingContextImpl.java:140) at org.keycloak.quarkus.runtime.integration.web.QuarkusRequestFilter.lambda$createBlockingHandler$0(QuarkusRequestFilter.java:82) at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:576) at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2449) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1478) at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29) at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.base/java.lang.Thread.run(Thread.java:833)

But @gbatalski prepared a PR https://github.com/aerogear/keycloak-metrics-spi/pull/157 with some updates that fixes that issue as well.

pb82 commented 1 year ago

thanks @Bilbergia I wonder if there is a way to make this work on both Keycloak 21 and earlier versions. We could catch the exception and carry on, but could it happen that the SPI initializes first and Keycloak fails registering its exporter?

logopk commented 1 year ago

From what I see the PR does only update version numbers in pom.xml and moves to a newer prometheus and java version . The above error (keycloak_login_attempts_total already in use by another Collector) is not handled by any code change.

But the newly compiled jar works flawlessly.

ghenadiibatalski commented 1 year ago

@logopk i only needed to remove _total from the metric name. as i understand, _total is a reserved suffix now. The keycloak 21 ships the prometheus 0.16.0 so i updated the PR to keycloak 21 I don't know, if it could be backported for both versions. But the previous build is good enough for older Keycloak and the new one is compatible from 21 upwards.

Bilbergia commented 1 year ago

@pb82 The PrometheusExporter should already be build up by quarkus-micrometer during augmentation phase, so before Keycloak starts to initialize providers like keycloak-metrics-spi

pb82 commented 1 year ago

@ghenadiibatalski has submitted a fix and I've verified it with KC 19, 20 and 21. I want to get it merged and released as soon as possible.

logopk commented 1 year ago

@pb82 shouldn't pom.xml be updated too to go back to Java 11?

pb82 commented 1 year ago

Yes :facepalm: Sorry, I forgot that, too used to gradle. Will send a PR.

logopk commented 1 year ago

Thanks @pb82