From f1a746dca5af8cf245f66458b155a9819e75449b Mon Sep 17 00:00:00 2001 From: jason plumb <75337021+breedx-splk@users.noreply.github.com> Date: Thu, 2 Jun 2022 16:18:07 -0700 Subject: [PATCH] Add tomcat-jdbc connection pool metrics instrumentation (#6102) * add tomcat-jdbc connection pool metrics instrumentation * use duration * code review comments * remove unnecessary awaits * udpate supported-libraries.md * add comment about weakmap * add sleeps for safety --- docs/supported-libraries.md | 1 + .../tomcat/tomcat-jdbc/build.gradle.kts | 17 ++++ .../jdbc/DataSourceProxyInstrumentation.java | 53 ++++++++++ .../jdbc/TomcatConnectionPoolMetrics.java | 58 +++++++++++ .../jdbc/TomcatJdbcInstrumentationModule.java | 25 +++++ .../jdbc/TomcatJdbcInstrumentationTest.java | 99 +++++++++++++++++++ settings.gradle.kts | 1 + 7 files changed, 254 insertions(+) create mode 100644 instrumentation/tomcat/tomcat-jdbc/build.gradle.kts create mode 100644 instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/DataSourceProxyInstrumentation.java create mode 100644 instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatConnectionPoolMetrics.java create mode 100644 instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationModule.java create mode 100644 instrumentation/tomcat/tomcat-jdbc/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationTest.java diff --git a/docs/supported-libraries.md b/docs/supported-libraries.md index 8b92cfd52e67..b6adc75094f8 100644 --- a/docs/supported-libraries.md +++ b/docs/supported-libraries.md @@ -110,6 +110,7 @@ These are the supported libraries and frameworks: | [Spring Web Services](https://spring.io/projects/spring-ws) | 2.0+ | | [Spring WebFlux](https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/web/reactive/package-summary.html) | 5.0+ | | [Spymemcached](https://github.com/couchbase/spymemcached) | 2.12+ | +| [Tomcat JDBC Pool](https://tomcat.apache.org/tomcat-7.0-doc/jdbc-pool.html) | 8.5.0+ | | [Twilio](https://github.com/twilio/twilio-java) | 6.6+ (not including 8.x yet) | | [Undertow](https://undertow.io/) | 1.4+ | | [Vaadin](https://vaadin.com/) | 14.2+ | diff --git a/instrumentation/tomcat/tomcat-jdbc/build.gradle.kts b/instrumentation/tomcat/tomcat-jdbc/build.gradle.kts new file mode 100644 index 000000000000..ab8684db2fd1 --- /dev/null +++ b/instrumentation/tomcat/tomcat-jdbc/build.gradle.kts @@ -0,0 +1,17 @@ +plugins { + id("otel.javaagent-instrumentation") +} + +muzzle { + pass { + group.set("org.apache.tomcat") + module.set("tomcat-jdbc") + versions.set("[8.5.0,)") + // no assertInverse because tomcat-jdbc < 8.5 doesn't have methods that we hook into + } +} + +dependencies { + compileOnly("org.apache.tomcat:tomcat-jdbc:8.5.0") + testImplementation("org.apache.tomcat:tomcat-jdbc:8.5.0") +} diff --git a/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/DataSourceProxyInstrumentation.java b/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/DataSourceProxyInstrumentation.java new file mode 100644 index 000000000000..07d79f4ce801 --- /dev/null +++ b/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/DataSourceProxyInstrumentation.java @@ -0,0 +1,53 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.jdbc; + +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; +import static net.bytebuddy.matcher.ElementMatchers.takesNoArguments; + +import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.apache.tomcat.jdbc.pool.DataSourceProxy; + +class DataSourceProxyInstrumentation implements TypeInstrumentation { + + @Override + public ElementMatcher typeMatcher() { + return named("org.apache.tomcat.jdbc.pool.DataSourceProxy"); + } + + @Override + public void transform(TypeTransformer transformer) { + transformer.applyAdviceToMethod( + isPublic().and(named("createPool")).and(takesNoArguments()), + this.getClass().getName() + "$CreatePoolAdvice"); + + transformer.applyAdviceToMethod( + isPublic().and(named("close")).and(takesArguments(1)), + this.getClass().getName() + "$CloseAdvice"); + } + + @SuppressWarnings("unused") + public static class CreatePoolAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void onExit(@Advice.This DataSourceProxy dataSource) { + TomcatConnectionPoolMetrics.registerMetrics(dataSource); + } + } + + @SuppressWarnings("unused") + public static class CloseAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void onExit(@Advice.This DataSourceProxy dataSource) { + TomcatConnectionPoolMetrics.unregisterMetrics(dataSource); + } + } +} diff --git a/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatConnectionPoolMetrics.java b/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatConnectionPoolMetrics.java new file mode 100644 index 000000000000..79734df8e8b8 --- /dev/null +++ b/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatConnectionPoolMetrics.java @@ -0,0 +1,58 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.jdbc; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.OpenTelemetry; +import io.opentelemetry.api.metrics.ObservableLongUpDownCounter; +import io.opentelemetry.instrumentation.api.metrics.db.DbConnectionPoolMetrics; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.tomcat.jdbc.pool.DataSourceProxy; + +public final class TomcatConnectionPoolMetrics { + + private static final OpenTelemetry openTelemetry = GlobalOpenTelemetry.get(); + private static final String INSTRUMENTATION_NAME = "io.opentelemetry.tomcat-jdbc"; + + // a weak map does not make sense here because each Meter holds a reference to the dataSource + // DataSourceProxy does not implement equals()/hashCode(), so it's safe to keep them in a plain + // ConcurrentHashMap + private static final Map> dataSourceMetrics = + new ConcurrentHashMap<>(); + + public static void registerMetrics(DataSourceProxy dataSource) { + dataSourceMetrics.computeIfAbsent(dataSource, TomcatConnectionPoolMetrics::createCounters); + } + + private static List createCounters(DataSourceProxy dataSource) { + + DbConnectionPoolMetrics metrics = + DbConnectionPoolMetrics.create( + openTelemetry, INSTRUMENTATION_NAME, dataSource.getPoolName()); + + return Arrays.asList( + metrics.usedConnections(dataSource::getActive), + metrics.idleConnections(dataSource::getIdle), + metrics.minIdleConnections(dataSource::getMinIdle), + metrics.maxIdleConnections(dataSource::getMaxIdle), + metrics.maxConnections(dataSource::getMaxActive), + metrics.pendingRequestsForConnection(dataSource::getWaitCount)); + } + + public static void unregisterMetrics(DataSourceProxy dataSource) { + List counters = dataSourceMetrics.remove(dataSource); + if (counters != null) { + for (ObservableLongUpDownCounter meter : counters) { + meter.close(); + } + } + } + + private TomcatConnectionPoolMetrics() {} +} diff --git a/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationModule.java b/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationModule.java new file mode 100644 index 000000000000..177f9bdd3773 --- /dev/null +++ b/instrumentation/tomcat/tomcat-jdbc/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationModule.java @@ -0,0 +1,25 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.jdbc; + +import static java.util.Collections.singletonList; + +import com.google.auto.service.AutoService; +import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import java.util.List; + +@AutoService(InstrumentationModule.class) +public class TomcatJdbcInstrumentationModule extends InstrumentationModule { + public TomcatJdbcInstrumentationModule() { + super("tomcat-jdbc"); + } + + @Override + public List typeInstrumentations() { + return singletonList(new DataSourceProxyInstrumentation()); + } +} diff --git a/instrumentation/tomcat/tomcat-jdbc/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationTest.java b/instrumentation/tomcat/tomcat-jdbc/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationTest.java new file mode 100644 index 000000000000..c2902c228fe8 --- /dev/null +++ b/instrumentation/tomcat/tomcat-jdbc/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/jdbc/TomcatJdbcInstrumentationTest.java @@ -0,0 +1,99 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.jdbc; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; + +import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension; +import io.opentelemetry.instrumentation.testing.junit.db.DbConnectionPoolMetricsAssertions; +import java.sql.Connection; +import org.apache.tomcat.jdbc.pool.DataSource; +import org.assertj.core.api.AbstractIterableAssert; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class TomcatJdbcInstrumentationTest { + + @RegisterExtension + static final AgentInstrumentationExtension testing = AgentInstrumentationExtension.create(); + + @Mock javax.sql.DataSource dataSourceMock; + @Mock Connection connectionMock; + + @Test + void shouldReportMetrics() throws Exception { + // given + given(dataSourceMock.getConnection()).willReturn(connectionMock); + + DataSource tomcatDataSource = new DataSource(); + tomcatDataSource.setDataSource(dataSourceMock); + + // there shouldn't be any problems if this methods gets called more than once + tomcatDataSource.createPool(); + tomcatDataSource.createPool(); + + // when + Connection connection = tomcatDataSource.getConnection(); + Thread.sleep(100); + connection.close(); + + // then + assertConnectionPoolMetrics(tomcatDataSource.getPoolName()); + + // when + // this one too shouldn't cause any problems when called more than once + tomcatDataSource.close(); + tomcatDataSource.close(); + Thread.sleep(100); + testing.clearData(); + Thread.sleep(100); + + // then + assertNoConnectionPoolMetrics(); + } + + private static void assertConnectionPoolMetrics(String poolName) { + assertThat(poolName) + .as("tomcat-jdbc generates a unique pool name if it's not explicitly provided") + .isNotEmpty(); + + DbConnectionPoolMetricsAssertions.create(testing, "io.opentelemetry.tomcat-jdbc", poolName) + // no timeouts happen during this test + .disableConnectionTimeouts() + .disableCreateTime() + .disableWaitTime() + .disableUseTime() + .assertConnectionPoolEmitsMetrics(); + } + + private static void assertNoConnectionPoolMetrics() { + testing.waitAndAssertMetrics( + "io.opentelemetry.tomcat-jdbc", + "db.client.connections.usage", + AbstractIterableAssert::isEmpty); + testing.waitAndAssertMetrics( + "io.opentelemetry.tomcat-jdbc", + "db.client.connections.idle.min", + AbstractIterableAssert::isEmpty); + testing.waitAndAssertMetrics( + "io.opentelemetry.tomcat-jdbc", + "db.client.connections.idle.max", + AbstractIterableAssert::isEmpty); + testing.waitAndAssertMetrics( + "io.opentelemetry.tomcat-jdbc", + "db.client.connections.max", + AbstractIterableAssert::isEmpty); + testing.waitAndAssertMetrics( + "io.opentelemetry.tomcat-jdbc", + "db.client.connections.pending_requests", + AbstractIterableAssert::isEmpty); + } +} diff --git a/settings.gradle.kts b/settings.gradle.kts index a292d7b806c9..4808ea0fba4a 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -432,6 +432,7 @@ include(":instrumentation:tapestry-5.4:javaagent") include(":instrumentation:tomcat:tomcat-7.0:javaagent") include(":instrumentation:tomcat:tomcat-10.0:javaagent") include(":instrumentation:tomcat:tomcat-common:javaagent") +include(":instrumentation:tomcat:tomcat-jdbc") include(":instrumentation:twilio-6.6:javaagent") include(":instrumentation:undertow-1.4:bootstrap") include(":instrumentation:undertow-1.4:javaagent")