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.

Sign up for GitHub

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

Influxdb client: don't fill db.statement for create/drop database and write operations #11557

Merged
merged 2 commits into from
Jun 23, 2024
Merged

Influxdb client: don't fill db.statement for create/drop database and write operations #11557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2dd66b4
Influxdb client: don't fill db.statement for create/drop database and…
laurit Jun 12, 2024
1d13eb2
Merge branch 'main' into influxdb-operation
laurit Jun 12, 2024
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
8 changes: 8 additions & 0 deletions instrumentation/influxdb-2.4/javaagent/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ dependencies {
compileOnly("com.google.auto.value:auto-value-annotations")
annotationProcessor("com.google.auto.value:auto-value")

testInstrumentation(project(":instrumentation:okhttp:okhttp-3.0:javaagent"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe is it better to describe the context leak problem by adding necessary comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already described further down where the instrumentation is disabled.

Copy link
Contributor

@steverao steverao Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is already described further down where the instrumentation is disabled.

Yes, I noticed it before, but I was worried that someone might not have noticed the bottom comments. There are about 30 lines between them.😅


// we use methods that weren't present before 2.14 in tests
testLibrary("org.influxdb:influxdb-java:2.14")
}
Expand Down Expand Up @@ -44,3 +46,9 @@ tasks {
}
}
}

tasks.withType<Test>().configureEach {
// we disable the okhttp instrumentation, so we don't need to assert on the okhttp spans
// from the okhttp instrumentation we need OkHttp3IgnoredTypesConfigurer to fix context leaks
jvmArgs("-Dotel.instrumentation.okhttp.enabled=false")
}
7 changes: 3 additions & 4 deletions ...va/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbAttributesGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@ public String getStatement(InfluxDbRequest request) {
@Nullable
@Override
public String getOperation(InfluxDbRequest request) {
if (request.getSqlStatementInfo() != null) {
String operation = request.getSqlStatementInfo().getOperation();
return operation == null ? request.getSql() : operation;
if (request.getOperation() != null) {
return request.getOperation();
}
return null;
return request.getSqlStatementInfo().getOperation();
}

@Override
Expand Down
18 changes: 0 additions & 18 deletions ...main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbConstants.java
View file
Open in desktop

This file was deleted.

20 changes: 8 additions & 12 deletions ...io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4;

import static io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge.currentContext;
import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_NEW;
import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_OLD;
import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.DELETE_DATABASE_STATEMENT;
import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbSingletons.instrumenter;
import static net.bytebuddy.matcher.ElementMatchers.isEnum;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
Expand Down Expand Up @@ -94,7 +91,7 @@ public static void onEnter(
HttpUrl httpUrl = retrofit.baseUrl();
influxDbRequest =
InfluxDbRequest.create(
httpUrl.host(), httpUrl.port(), query.getDatabase(), query.getCommand());
httpUrl.host(), httpUrl.port(), query.getDatabase(), null, query.getCommand());

if (!instrumenter().shouldStart(parentContext, influxDbRequest)) {
return;
Expand Down Expand Up @@ -142,7 +139,6 @@ public static class InfluxDbModifyAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void onEnter(
@Advice.This InfluxDBImpl influxDbImpl,
@Advice.Origin("#m") String methodName,
@Advice.Argument(0) Object arg0,
@Advice.FieldValue(value = "retrofit") Retrofit retrofit,
Expand All @@ -168,17 +164,17 @@ public static void onEnter(
// write data by UDP protocol, in this way, can't get database name.
: arg0 instanceof Integer ? "" : String.valueOf(arg0);

String sql = methodName;
String operation;
if ("createDatabase".equals(methodName)) {
sql =
influxDbImpl.version().startsWith("0.")
? String.format(CREATE_DATABASE_STATEMENT_OLD, database)
: String.format(CREATE_DATABASE_STATEMENT_NEW, database);
operation = "CREATE DATABASE";
} else if ("deleteDatabase".equals(methodName)) {
sql = String.format(DELETE_DATABASE_STATEMENT, database);
operation = "DROP DATABASE";
} else {
operation = "WRITE";
}

influxDbRequest = InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, sql);
influxDbRequest =
InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, operation, null);

if (!instrumenter().shouldStart(parentContext, influxDbRequest)) {
return;
Expand Down
9 changes: 6 additions & 3 deletions ...c/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@
import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementInfo;
import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementSanitizer;
import io.opentelemetry.javaagent.bootstrap.internal.CommonConfig;
import javax.annotation.Nullable;

@AutoValue
public abstract class InfluxDbRequest {

private static final SqlStatementSanitizer sanitizer =
SqlStatementSanitizer.create(CommonConfig.get().isStatementSanitizationEnabled());

public static InfluxDbRequest create(String host, Integer port, String dbName, String sql) {
return new AutoValue_InfluxDbRequest(host, port, dbName, sql, sanitizer.sanitize(sql));
public static InfluxDbRequest create(
String host, Integer port, String dbName, String operation, String sql) {
return new AutoValue_InfluxDbRequest(host, port, dbName, operation, sanitizer.sanitize(sql));
}

public abstract String getHost();
Expand All @@ -26,7 +28,8 @@ public static InfluxDbRequest create(String host, Integer port, String dbName, S

public abstract String getDbName();

public abstract String getSql();
@Nullable
public abstract String getOperation();

public abstract SqlStatementInfo getSqlStatementInfo();
}
47 changes: 22 additions & 25 deletions ...est/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4;

import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_NEW;
import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.DELETE_DATABASE_STATEMENT;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
import static java.util.Arrays.asList;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -108,16 +106,13 @@ void testQueryAndModifyWithOneArgument() {
span.hasName("CREATE DATABASE " + dbName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(
attributeAssertions(
String.format(CREATE_DATABASE_STATEMENT_NEW, dbName),
"CREATE DATABASE",
dbName))),
attributeAssertions(null, "CREATE DATABASE", dbName))),
trace ->
trace.hasSpansSatisfyingExactly(
span ->
span.hasName("write " + dbName)
span.hasName("WRITE " + dbName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(attributeAssertions("write", "write", dbName))),
.hasAttributesSatisfying(attributeAssertions(null, "WRITE", dbName))),
trace ->
trace.hasSpansSatisfyingExactly(
span ->
Expand All @@ -131,10 +126,7 @@ void testQueryAndModifyWithOneArgument() {
span.hasName("DROP DATABASE " + dbName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(
attributeAssertions(
String.format(DELETE_DATABASE_STATEMENT, dbName),
"DROP DATABASE",
dbName))));
attributeAssertions(null, "DROP DATABASE", dbName))));
}

@Test
Expand Down Expand Up @@ -279,10 +271,10 @@ void testWriteWithFourArguments() {
trace ->
trace.hasSpansSatisfyingExactly(
span ->
span.hasName("write " + databaseName)
span.hasName("WRITE " + databaseName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(
attributeAssertions("write", "write", databaseName))));
attributeAssertions(null, "WRITE", databaseName))));
}

@Test
Expand All @@ -297,10 +289,10 @@ void testWriteWithFiveArguments() {
trace ->
trace.hasSpansSatisfyingExactly(
span ->
span.hasName("write " + databaseName)
span.hasName("WRITE " + databaseName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(
attributeAssertions("write", "write", databaseName))));
attributeAssertions(null, "WRITE", databaseName))));
}

@Test
Expand All @@ -316,19 +308,24 @@ void testWriteWithUdp() {
trace ->
trace.hasSpansSatisfyingExactly(
span ->
span.hasName("write")
span.hasName("WRITE")
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(attributeAssertions("write", "write", null))));
.hasAttributesSatisfying(attributeAssertions(null, "WRITE", null))));
}

private static List<AttributeAssertion> attributeAssertions(
String statement, String operation, String databaseName) {
return asList(
equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
equalTo(ServerAttributes.SERVER_ADDRESS, host),
equalTo(ServerAttributes.SERVER_PORT, port),
equalTo(DbIncubatingAttributes.DB_STATEMENT, statement),
equalTo(DbIncubatingAttributes.DB_OPERATION, operation));
List<AttributeAssertion> result = new ArrayList<>();
result.addAll(
asList(
equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
equalTo(ServerAttributes.SERVER_ADDRESS, host),
equalTo(ServerAttributes.SERVER_PORT, port),
equalTo(DbIncubatingAttributes.DB_OPERATION, operation)));
if (statement != null) {
result.add(equalTo(DbIncubatingAttributes.DB_STATEMENT, statement));
}
return result;
}
}
36 changes: 17 additions & 19 deletions ...4/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClient24Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4;

import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_NEW;
import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.DELETE_DATABASE_STATEMENT;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
import static java.util.Arrays.asList;
import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -17,6 +15,7 @@
import io.opentelemetry.sdk.testing.assertj.AttributeAssertion;
import io.opentelemetry.semconv.ServerAttributes;
import io.opentelemetry.semconv.incubating.DbIncubatingAttributes;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;
import org.influxdb.InfluxDB;
Expand Down Expand Up @@ -101,16 +100,13 @@ void testQueryAndModifyWithOneArgument() {
span.hasName("CREATE DATABASE " + dbName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(
attributeAssertions(
String.format(CREATE_DATABASE_STATEMENT_NEW, dbName),
"CREATE DATABASE",
dbName))),
attributeAssertions(null, "CREATE DATABASE", dbName))),
trace ->
trace.hasSpansSatisfyingExactly(
span ->
span.hasName("write " + dbName)
span.hasName("WRITE " + dbName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(attributeAssertions("write", "write", dbName))),
.hasAttributesSatisfying(attributeAssertions(null, "WRITE", dbName))),
trace ->
trace.hasSpansSatisfyingExactly(
span ->
Expand All @@ -124,10 +120,7 @@ void testQueryAndModifyWithOneArgument() {
span.hasName("DROP DATABASE " + dbName)
.hasKind(SpanKind.CLIENT)
.hasAttributesSatisfying(
attributeAssertions(
String.format(DELETE_DATABASE_STATEMENT, dbName),
"DROP DATABASE",
dbName))));
attributeAssertions(null, "DROP DATABASE", dbName))));
}

@Test
Expand All @@ -150,12 +143,17 @@ void testQueryWithTwoArguments() {

private static List<AttributeAssertion> attributeAssertions(
String statement, String operation, String databaseName) {
return asList(
equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
equalTo(ServerAttributes.SERVER_ADDRESS, host),
equalTo(ServerAttributes.SERVER_PORT, port),
equalTo(DbIncubatingAttributes.DB_STATEMENT, statement),
equalTo(DbIncubatingAttributes.DB_OPERATION, operation));
List<AttributeAssertion> result = new ArrayList<>();
result.addAll(
asList(
equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
equalTo(ServerAttributes.SERVER_ADDRESS, host),
equalTo(ServerAttributes.SERVER_PORT, port),
equalTo(DbIncubatingAttributes.DB_OPERATION, operation)));
if (statement != null) {
result.add(equalTo(DbIncubatingAttributes.DB_STATEMENT, statement));
}
return result;
}
}
Loading