From 5d780742bcd3144d809f45e75eb57d7d661d9d7d Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Thu, 9 Nov 2023 19:02:18 +0900 Subject: [PATCH] Improve `JettyConnectionMetrics` connection type detection The `HttpConnection` class was relocated to a different package in Jetty 12 which was causing a `NoClassDefFoundError` when doing an instanceof check on it. The goal of it is to determine whether the connection instrumented is a server connection or a client connection. However, it specifically checks against `HttpConnection` and assumes that all server connections will be an instanceof `HttpConnection` and any not an instanceof it are client connections. This is brittle because Jetty supports more implementations of `Connection` on the server side than `HttpConnection` and there could in theory be an arbitrary implementation where we do not know whether it is a server or client connection (it could also be neither). This instead checks whether the package name contains `server` or `client` or neither. This is admittedly also brittle, but given the known implementations of `Connection` provided by the Jetty project, this pattern generally seems to hold, and it is at least more honest using `UNKNOWN` when our heuristic fails. It also avoids the `NoClassDefFoundError` when using `JettyConnectionMetrics` with Jetty 12. Resolves gh-4324 --- dependencies.gradle | 3 - gradle/libs.versions.toml | 11 +- micrometer-core/build.gradle | 6 +- .../binder/jetty/JettyConnectionMetrics.java | 11 +- micrometer-jetty11/build.gradle | 6 +- micrometer-test/build.gradle | 6 +- .../micrometer-samples-jetty12/build.gradle | 13 ++ .../JettyConnectionMetricsTest.java | 140 ++++++++++++++++++ settings.gradle | 2 +- 9 files changed, 177 insertions(+), 21 deletions(-) create mode 100644 samples/micrometer-samples-jetty12/build.gradle create mode 100644 samples/micrometer-samples-jetty12/src/test/java/io.micrometer.samples.jetty12/JettyConnectionMetricsTest.java diff --git a/dependencies.gradle b/dependencies.gradle index c0aea07e3e..fa7bc068d9 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -60,9 +60,6 @@ def VERSIONS = [ libs.aspectjweaver, libs.assertj, libs.awaitility, - libs.jettyClient, - libs.jettyServer, - libs.jettyServlet, libs.jersey2Server, libs.jersey2Hk2, libs.jersey2TestFrameworkInmemory, diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 7352c78b0f..a208608525 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -40,8 +40,9 @@ jackson-databind = "2.15.3" javax-cache = "1.1.1" javax-inject = "1" jaxb = "2.3.1" -jetty = "9.4.53.v20231009" +jetty9 = "9.4.53.v20231009" jetty11 = "11.0.16" +jetty12 = "12.0.3" jersey2 = "2.41" jersey3 = "3.0.11" jmh = "1.37" @@ -133,10 +134,12 @@ javax-cacheApi = { module = "javax.cache:cache-api", version.ref = "javax-cache" javax-inject = { module = "javax.inject:javax.inject", version.ref = "javax-inject" } javax-servletApi = { module = "javax.servlet:javax.servlet-api", version = "4.0.1" } jaxbApi = { module = "javax.xml.bind:jaxb-api", version.ref = "jaxb" } -jettyClient = { module = "org.eclipse.jetty:jetty-client", version.ref = "jetty" } -jettyServer = { module = "org.eclipse.jetty:jetty-server", version.ref = "jetty" } -jettyServlet = { module = "org.eclipse.jetty:jetty-servlet", version.ref = "jetty" } +jetty9Client = { module = "org.eclipse.jetty:jetty-client", version.ref = "jetty9" } +jetty9Server = { module = "org.eclipse.jetty:jetty-server", version.ref = "jetty9" } +jetty9Servlet = { module = "org.eclipse.jetty:jetty-servlet", version.ref = "jetty9" } jetty11Server = { module = "org.eclipse.jetty:jetty-server", version.ref = "jetty11" } +jetty12Server = { module = "org.eclipse.jetty:jetty-server", version.ref = "jetty12" } +jetty12Client = { module = "org.eclipse.jetty:jetty-client", version.ref = "jetty12" } jersey2Server = { module = "org.glassfish.jersey.core:jersey-server", version.ref = "jersey2" } jersey2Hk2 = { module = "org.glassfish.jersey.inject:jersey-hk2", version.ref = "jersey2" } jersey2TestFrameworkInmemory = { module = "org.glassfish.jersey.test-framework.providers:jersey-test-framework-provider-inmemory", version.ref = "jersey2" } diff --git a/micrometer-core/build.gradle b/micrometer-core/build.gradle index df8700de0e..726aecb324 100644 --- a/micrometer-core/build.gradle +++ b/micrometer-core/build.gradle @@ -92,10 +92,10 @@ dependencies { optionalApi 'org.hibernate:hibernate-entitymanager' // server runtime monitoring - optionalApi 'org.eclipse.jetty:jetty-server' + optionalApi libs.jetty9Server // jakarta servlet optionalApi 'jakarta.servlet:jakarta.servlet-api' - optionalApi 'org.eclipse.jetty:jetty-client' + optionalApi libs.jetty9Client optionalApi 'org.apache.tomcat.embed:tomcat-embed-core' optionalApi 'org.glassfish.jersey.core:jersey-server' optionalApi 'io.grpc:grpc-api' @@ -198,6 +198,8 @@ dependencies { testImplementation 'io.grpc:grpc-inprocess' testImplementation 'io.grpc:grpc-testing-proto' testImplementation 'com.squareup.retrofit2:retrofit' + + testImplementation libs.jetty9http2Server } task shenandoahTest(type: Test) { diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyConnectionMetrics.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyConnectionMetrics.java index 9478d5be57..85c559eacf 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyConnectionMetrics.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyConnectionMetrics.java @@ -21,7 +21,6 @@ import io.micrometer.core.instrument.distribution.TimeWindowMax; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.server.Connector; -import org.eclipse.jetty.server.HttpConnection; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.util.component.AbstractLifeCycle; @@ -162,10 +161,16 @@ public void onClosed(Connection connection) { } if (sample != null) { - String serverOrClient = connection instanceof HttpConnection ? "server" : "client"; + String type = "UNKNOWN"; + if (connection.getClass().getName().contains("server")) { + type = "server"; + } + else if (connection.getClass().getName().contains("client")) { + type = "client"; + } sample.stop(Timer.builder("jetty.connections.request") .description("Jetty client or server requests") - .tag("type", serverOrClient) + .tag("type", type) .tags(tags) .register(registry)); } diff --git a/micrometer-jetty11/build.gradle b/micrometer-jetty11/build.gradle index 92d8b275d3..7ac042710b 100644 --- a/micrometer-jetty11/build.gradle +++ b/micrometer-jetty11/build.gradle @@ -2,11 +2,7 @@ description 'Micrometer instrumentation for Jetty 11' dependencies { api project(":micrometer-core") - api(libs.jetty11Server) { - version { - strictly libs.jetty11Server.get().version - } - } + api libs.jetty11Server testImplementation 'org.junit.jupiter:junit-jupiter' testImplementation 'org.assertj:assertj-core' diff --git a/micrometer-test/build.gradle b/micrometer-test/build.gradle index a1f29a59dc..42221bf4df 100644 --- a/micrometer-test/build.gradle +++ b/micrometer-test/build.gradle @@ -54,9 +54,9 @@ dependencies { testImplementation 'org.apache.httpcomponents.client5:httpclient5' testImplementation 'org.apache.activemq:artemis-junit-5' testImplementation 'org.apache.activemq:artemis-jakarta-client' - testImplementation 'org.eclipse.jetty:jetty-client' - testImplementation 'org.eclipse.jetty:jetty-server' - testImplementation 'org.eclipse.jetty:jetty-servlet' + testImplementation libs.jetty9Client + testImplementation libs.jetty9Server + testImplementation libs.jetty9Servlet testImplementation 'org.glassfish.jersey.core:jersey-server' testImplementation libs.jersey2TestFrameworkJdkHttp // necessary for Jersey test framework diff --git a/samples/micrometer-samples-jetty12/build.gradle b/samples/micrometer-samples-jetty12/build.gradle new file mode 100644 index 0000000000..b8f7604f2a --- /dev/null +++ b/samples/micrometer-samples-jetty12/build.gradle @@ -0,0 +1,13 @@ +plugins { + id 'java' +} + +dependencies { + implementation project(":micrometer-core") + + testImplementation libs.jetty12Server + testImplementation libs.jetty12Client + testImplementation libs.httpcomponents.client + testImplementation libs.junitJupiter + testImplementation libs.assertj +} diff --git a/samples/micrometer-samples-jetty12/src/test/java/io.micrometer.samples.jetty12/JettyConnectionMetricsTest.java b/samples/micrometer-samples-jetty12/src/test/java/io.micrometer.samples.jetty12/JettyConnectionMetricsTest.java new file mode 100644 index 0000000000..f8acc1a0c4 --- /dev/null +++ b/samples/micrometer-samples-jetty12/src/test/java/io.micrometer.samples.jetty12/JettyConnectionMetricsTest.java @@ -0,0 +1,140 @@ +/* + * Copyright 2023 VMware, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.micrometer.samples.jetty12; + +import io.micrometer.core.instrument.MockClock; +import io.micrometer.core.instrument.binder.jetty.JettyConnectionMetrics; +import io.micrometer.core.instrument.simple.SimpleConfig; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClients; +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.Request; +import org.eclipse.jetty.client.StringRequestContent; +import org.eclipse.jetty.server.Connector; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; + +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.assertj.core.api.Assertions.assertThat; + +class JettyConnectionMetricsTest { + + private SimpleMeterRegistry registry = new SimpleMeterRegistry(SimpleConfig.DEFAULT, new MockClock()); + + private Server server = new Server(0); + + private ServerConnector connector = new ServerConnector(server); + + private CloseableHttpClient client = HttpClients.createDefault(); + + void setup() throws Exception { + connector.addBean(new JettyConnectionMetrics(registry)); + server.setConnectors(new Connector[] { connector }); + server.start(); + } + + @AfterEach + void teardown() throws Exception { + if (server.isRunning()) { + server.stop(); + } + } + + @Test + void contributesServerConnectorMetrics() throws Exception { + setup(); + HttpPost post = new HttpPost("http://localhost:" + connector.getLocalPort()); + post.setEntity(new StringEntity("123456")); + + try (CloseableHttpResponse ignored = client.execute(post)) { + try (CloseableHttpResponse ignored2 = client.execute(post)) { + assertThat(registry.get("jetty.connections.current").gauge().value()).isEqualTo(2.0); + assertThat(registry.get("jetty.connections.max").gauge().value()).isEqualTo(2.0); + } + } + + CountDownLatch latch = new CountDownLatch(1); + connector.addEventListener(new LifeCycle.Listener() { + @Override + public void lifeCycleStopped(LifeCycle event) { + latch.countDown(); + } + }); + // Convenient way to get Jetty to flush its connections, which is required to + // update the sent/received bytes metrics + server.stop(); + + assertThat(latch.await(10, SECONDS)).isTrue(); + assertThat(registry.get("jetty.connections.max").gauge().value()).isEqualTo(2.0); + assertThat(registry.get("jetty.connections.request").tag("type", "server").timer().count()).isEqualTo(2); + assertThat(registry.get("jetty.connections.bytes.in").summary().totalAmount()).isGreaterThan(1); + } + + @Test + void contributesClientConnectorMetrics() throws Exception { + setup(); + HttpClient httpClient = new HttpClient(); + httpClient.setFollowRedirects(false); + httpClient.addBean(new JettyConnectionMetrics(registry)); + + CountDownLatch latch = new CountDownLatch(1); + httpClient.addEventListener(new LifeCycle.Listener() { + @Override + public void lifeCycleStopped(LifeCycle event) { + latch.countDown(); + } + }); + + httpClient.start(); + + Request post = httpClient.POST("http://localhost:" + connector.getLocalPort()); + post.body(new StringRequestContent("123456")); + post.send(); + httpClient.stop(); + + assertThat(latch.await(10, SECONDS)).isTrue(); + assertThat(registry.get("jetty.connections.max").gauge().value()).isEqualTo(1.0); + assertThat(registry.get("jetty.connections.request").tag("type", "client").timer().count()).isEqualTo(1); + assertThat(registry.get("jetty.connections.bytes.out").summary().totalAmount()).isGreaterThan(1); + } + + @Test + void passingConnectorAddsConnectorNameTag() { + new JettyConnectionMetrics(registry, connector); + + assertThat(registry.get("jetty.connections.messages.in").counter().getId().getTag("connector.name")) + .isEqualTo("unnamed"); + } + + @Test + void namedConnectorsGetTaggedWithName() { + connector.setName("super-fast-connector"); + new JettyConnectionMetrics(registry, connector); + + assertThat(registry.get("jetty.connections.messages.in").counter().getId().getTag("connector.name")) + .isEqualTo("super-fast-connector"); + } + +} diff --git a/settings.gradle b/settings.gradle index cb2e2ca58b..92a9db2986 100644 --- a/settings.gradle +++ b/settings.gradle @@ -25,7 +25,7 @@ gradleEnterprise { include 'micrometer-commons', 'micrometer-core', 'micrometer-observation' -['core', 'boot2', 'boot2-reactive', 'spring-integration', 'hazelcast', 'hazelcast3', 'javalin', 'jersey3'].each { sample -> +['core', 'boot2', 'boot2-reactive', 'spring-integration', 'hazelcast', 'hazelcast3', 'javalin', 'jersey3', 'jetty12'].each { sample -> include "micrometer-samples-$sample" project(":micrometer-samples-$sample").projectDir = new File(rootProject.projectDir, "samples/micrometer-samples-$sample") }