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

Fix ClassCastException in JDBC instrumentation #6088

Merged
merged 4 commits into from
Jun 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions instrumentation/jdbc/bootstrap/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
plugins {
id("otel.javaagent-bootstrap")
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file would be a good place to document why DbInfo needs to be in the bootstrap class loader, maybe walking through an example of how the wrong DbInfo class can get picked up without this

/*
JDDC instrumentation uses VirtualField<Connection, DbInfo>. Add DbInfo, that is used as the value of
VirtualField, to boot loader. We do this because when JDBC instrumentation is started in multiple
class loaders in the same hierarchy, each would define their own version of DbInfo. It is possible
that the value read from virtual field would be from the wrong class loader and could produce a
ClassCastException. Having a single copy of DbInfo that is in boot loader avoids this issue.
*/

sourceSets {
main {
val shadedDep = project(":instrumentation:jdbc:library")
output.dir(
shadedDep.file("build/extracted/shadow-bootstrap"),
"builtBy" to ":instrumentation:jdbc:library:extractShadowJarBootstrap"
)
}
}
19 changes: 17 additions & 2 deletions instrumentation/jdbc/javaagent/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,13 @@ muzzle {
}

dependencies {
implementation(project(":instrumentation:jdbc:library"))

bootstrap(project(":instrumentation:jdbc:bootstrap"))
compileOnly(
project(
path = ":instrumentation:jdbc:library",
configuration = "shadow"
)
)
compileOnly("com.google.auto.value:auto-value-annotations")
annotationProcessor("com.google.auto.value:auto-value")

Expand All @@ -31,6 +36,16 @@ dependencies {
testImplementation(project(":instrumentation:jdbc:testing"))
}

sourceSets {
main {
val shadedDep = project(":instrumentation:jdbc:library")
output.dir(
shadedDep.file("build/extracted/shadow-javaagent"),
"builtBy" to ":instrumentation:jdbc:library:extractShadowJarJavaagent"
)
}
}

tasks.withType<Test>().configureEach {
jvmArgs("-Dotel.instrumentation.jdbc-datasource.enabled=true")
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
import static net.bytebuddy.matcher.ElementMatchers.returns;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

import io.opentelemetry.instrumentation.jdbc.internal.DbInfo;
import io.opentelemetry.instrumentation.jdbc.internal.JdbcConnectionUrlParser;
import io.opentelemetry.instrumentation.jdbc.internal.JdbcData;
import io.opentelemetry.javaagent.bootstrap.jdbc.DbInfo;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import java.sql.Connection;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import io.opentelemetry.api.trace.SpanKind
import io.opentelemetry.instrumentation.jdbc.TestConnection
import io.opentelemetry.instrumentation.jdbc.TestDriver
import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
import io.opentelemetry.javaagent.instrumentation.jdbc.test.ProxyStatementFactory
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
import org.apache.derby.jdbc.EmbeddedDataSource
import org.apache.derby.jdbc.EmbeddedDriver
Expand Down Expand Up @@ -820,4 +821,36 @@ class JdbcInstrumentationTest extends AgentInstrumentationSpecification {
return super.getMetaData()
}
}

// regression test for https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/6015
def "test proxy statement"() {
def connection = new Driver().connect(jdbcUrls.get("h2"), null)
Statement statement = connection.createStatement()
Statement proxyStatement = ProxyStatementFactory.proxyStatement(statement)
ResultSet resultSet = runWithSpan("parent") {
return proxyStatement.executeQuery("SELECT 3")
}

expect:
resultSet.next()
resultSet.getInt(1) == 3
assertTraces(1) {
trace(0, 2) {
span(0) {
name "parent"
kind SpanKind.INTERNAL
hasNoParent()
}
span(1) {
name "SELECT $dbNameLower"
kind CLIENT
childOf span(0)
}
}
}

cleanup:
statement.close()
connection.close()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.jdbc.test;

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Proxy;
import java.sql.Statement;

public class ProxyStatementFactory {

public static Statement proxyStatement(Statement statement) throws Exception {
TestClassLoader classLoader = new TestClassLoader(ProxyStatementFactory.class.getClassLoader());
Class<?> testInterface = classLoader.loadClass(TestInterface.class.getName());
if (testInterface.getClassLoader() != classLoader) {
throw new IllegalStateException("wrong class loader");
}
InvocationHandler invocationHandler = (proxy, method, args) -> method.invoke(statement, args);
Statement proxyStatement =
(Statement)
Proxy.newProxyInstance(
classLoader, new Class<?>[] {Statement.class, testInterface}, invocationHandler);
// adding package private interface TestInterface to jdk proxy forces defining the proxy class
// in the same package as the package private interface
if (!proxyStatement
.getClass()
.getName()
.startsWith("io.opentelemetry.javaagent.instrumentation.jdbc.test")) {
throw new IllegalStateException("proxy statement is in wrong package");
}

return proxyStatement;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.jdbc.test;

import java.net.URL;
import java.net.URLClassLoader;

public class TestClassLoader extends URLClassLoader {

public TestClassLoader(ClassLoader parent) {
super(
new URL[] {TestClassLoader.class.getProtectionDomain().getCodeSource().getLocation()},
parent);
}

@Override
protected synchronized Class<?> loadClass(String name, boolean resolve)
throws ClassNotFoundException {
Class<?> clazz = findLoadedClass(name);
if (clazz != null) {
return clazz;
}
if (name.startsWith("io.opentelemetry.javaagent.instrumentation.jdbc.test")) {
try {
return findClass(name);
} catch (ClassNotFoundException exception) {
// ignore
}
}
return super.loadClass(name, resolve);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.jdbc.test;

// Adding a package private interface to jdk proxy forces defining the proxy class in the package
// of the package private class. Usually proxy classes are defined in a package that we exclude from
// instrumentation. We use this class to force proxy into a different package so it would get
// instrumented.
interface TestInterface {}
29 changes: 29 additions & 0 deletions instrumentation/jdbc/library/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

plugins {
id("com.github.johnrengelman.shadow")
id("otel.library-instrumentation")
}

Expand All @@ -13,3 +14,31 @@ dependencies {

testImplementation(project(":instrumentation:jdbc:testing"))
}

tasks {
shadowJar {
dependencies {
// including only current module excludes its transitive dependencies
include(project(":instrumentation:jdbc:library"))
}
// rename classes that are included in :instrumentation:jdbc:bootstrap
relocate("io.opentelemetry.instrumentation.jdbc.internal.dbinfo", "io.opentelemetry.javaagent.bootstrap.jdbc")
}

// this will be included in javaagent module
val extractShadowJarJavaagent by registering(Copy::class) {
dependsOn(shadowJar)
from(zipTree(shadowJar.get().archiveFile))
into("build/extracted/shadow-javaagent")
exclude("META-INF/**")
exclude("io/opentelemetry/javaagent/bootstrap/**")
}

// this will be included in bootstrap module
val extractShadowJarBootstrap by registering(Copy::class) {
dependsOn(shadowJar)
from(zipTree(shadowJar.get().archiveFile))
into("build/extracted/shadow-bootstrap")
include("io/opentelemetry/javaagent/bootstrap/**")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@
import static io.opentelemetry.instrumentation.jdbc.internal.JdbcSingletons.INSTRUMENTATION_NAME;

import io.opentelemetry.instrumentation.api.internal.EmbeddedInstrumentationProperties;
import io.opentelemetry.instrumentation.jdbc.internal.DbInfo;
import io.opentelemetry.instrumentation.jdbc.internal.JdbcConnectionUrlParser;
import io.opentelemetry.instrumentation.jdbc.internal.OpenTelemetryConnection;
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.sql.Connection;
import java.sql.Driver;
import java.sql.DriverManager;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.jdbc.internal.DbInfo;
import io.opentelemetry.instrumentation.jdbc.internal.OpenTelemetryConnection;
import io.opentelemetry.instrumentation.jdbc.internal.ThrowingSupplier;
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.io.PrintWriter;
import java.sql.Connection;
import java.sql.SQLException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static io.opentelemetry.instrumentation.jdbc.internal.JdbcUtils.extractDbInfo;

import com.google.auto.value.AutoValue;
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.Statement;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.instrumentation.jdbc.internal;

import io.opentelemetry.instrumentation.api.instrumenter.db.SqlClientAttributesGetter;
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import javax.annotation.Nullable;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@

package io.opentelemetry.instrumentation.jdbc.internal;

import static io.opentelemetry.instrumentation.jdbc.internal.DbInfo.DEFAULT;
import static io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo.DEFAULT;
import static java.util.logging.Level.FINE;
import static java.util.regex.Pattern.CASE_INSENSITIVE;

import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes.DbSystemValues;
import java.io.UnsupportedEncodingException;
import java.net.URI;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.instrumentation.jdbc.internal;

import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.lang.ref.WeakReference;
import java.sql.Connection;
import java.sql.PreparedStatement;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import static java.util.logging.Level.FINE;

import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.lang.reflect.Field;
import java.sql.Connection;
import java.sql.DatabaseMetaData;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

package io.opentelemetry.instrumentation.jdbc.internal;

import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.io.InputStream;
import java.io.Reader;
import java.math.BigDecimal;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

package io.opentelemetry.instrumentation.jdbc.internal;

import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.sql.Array;
import java.sql.Blob;
import java.sql.CallableStatement;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

package io.opentelemetry.instrumentation.jdbc.internal;

import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.io.InputStream;
import java.io.Reader;
import java.math.BigDecimal;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo;
import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.jdbc.internal;
package io.opentelemetry.instrumentation.jdbc.internal.dbinfo;

import com.google.auto.value.AutoValue;
import javax.annotation.Nullable;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.jdbc

import io.opentelemetry.api.trace.SpanKind
import io.opentelemetry.instrumentation.jdbc.internal.DbInfo
import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo
import io.opentelemetry.instrumentation.jdbc.internal.OpenTelemetryCallableStatement
import io.opentelemetry.instrumentation.jdbc.internal.OpenTelemetryConnection
import io.opentelemetry.instrumentation.jdbc.internal.OpenTelemetryPreparedStatement
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.instrumentation.jdbc.internal

import io.opentelemetry.instrumentation.jdbc.internal.dbinfo.DbInfo
import spock.lang.Shared
import spock.lang.Specification

Expand Down
1 change: 1 addition & 0 deletions settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ include(":instrumentation:jaxws:jaxws-common:library")
include(":instrumentation:jaxws:jaxws-jws-api-1.1:javaagent")
include(":instrumentation:jboss-logmanager:jboss-logmanager-appender-1.1:javaagent")
include(":instrumentation:jboss-logmanager:jboss-logmanager-mdc-1.1:javaagent")
include(":instrumentation:jdbc:bootstrap")
include(":instrumentation:jdbc:javaagent")
include(":instrumentation:jdbc:library")
include(":instrumentation:jdbc:testing")
Expand Down