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

Add full APM/DBM mode for Oracle #8090

Merged
merged 13 commits into from
Dec 20, 2024
Merged

Add full APM/DBM mode for Oracle #8090

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fe6ca2f
Full APM/DBM mode for Oracle
nenadnoveljic Dec 13, 2024
6a0e5ae
enabled injection in tests
nenadnoveljic Dec 13, 2024
ef32413
tests
nenadnoveljic Dec 13, 2024
b07cca4
oracle container
nenadnoveljic Dec 16, 2024
4d0fab8
fixing oracle image
nenadnoveljic Dec 16, 2024
20c710b
fixed containers
nenadnoveljic Dec 16, 2024
5bd91c4
fixed first tests
nenadnoveljic Dec 16, 2024
860bb75
more oracle tests
nenadnoveljic Dec 16, 2024
f7636a4
finished tests
nenadnoveljic Dec 16, 2024
907ad6b
fixed tests
nenadnoveljic Dec 17, 2024
082bc46
Update dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace…
nenadnoveljic Dec 20, 2024
612ae6d
DD_INSTRUMENTATION_PREFIX
nenadnoveljic Dec 20, 2024
f8907b1
compute isOracle
nenadnoveljic Dec 20, 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
3 changes: 0 additions & 3 deletions ...nt/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/jdbc/DBInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ public static class Builder {

public Builder type(String type) {
this.type = type;
// Those DBs use the full text of the query including the comments as a cache key,
// so we disable full propagation support for them to avoid destroying the cache.
if (type.equals("oracle")) this.fullPropagationSupport = false;
return this;
}

Expand Down
2 changes: 2 additions & 0 deletions dd-java-agent/instrumentation/jdbc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,12 @@ dependencies {
testImplementation group: 'mysql', name: 'mysql-connector-java', version: '8.0.23'
testImplementation group: 'org.postgresql', name: 'postgresql', version: '[9.4,42.2.18]'
testImplementation group: 'com.microsoft.sqlserver', name: 'mssql-jdbc', version: '10.2.0.jre8'
testImplementation group: 'com.oracle.database.jdbc', name: 'ojdbc8', version: '19.19.0.0'

testImplementation group: 'org.testcontainers', name:'mysql', version: libs.versions.testcontainers.get()
testImplementation group: 'org.testcontainers', name:'postgresql', version: libs.versions.testcontainers.get()
testImplementation group: 'org.testcontainers', name:'mssqlserver', version: libs.versions.testcontainers.get()
testImplementation group: 'org.testcontainers', name:'oracle-xe', version: '1.20.4'

testRuntimeOnly project(':dd-java-agent:instrumentation:iast-instrumenter')

Expand Down
3 changes: 3 additions & 0 deletions ...ain/java/datadog/trace/instrumentation/jdbc/AbstractPreparedStatementInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ public static AgentScope onEnter(@Advice.This final Statement statement) {
} else if (DECORATE.isPostgres(dbInfo) && DBM_TRACE_PREPARED_STATEMENTS) {
span = startSpan(DATABASE_QUERY);
DECORATE.setApplicationName(span, connection);
} else if (DECORATE.isOracle(dbInfo)) {
span = startSpan(DATABASE_QUERY);
DECORATE.setAction(span, connection);
} else {
span = startSpan(DATABASE_QUERY);
}
Expand Down
36 changes: 36 additions & 0 deletions .../instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/JDBCDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ public String traceParent(AgentSpan span, int samplingPriority) {
return sb.toString();
}

public boolean isOracle(final DBInfo dbInfo) {
return "oracle".equals(dbInfo.getType());
}

public boolean isPostgres(final DBInfo dbInfo) {
return dbInfo.getType().startsWith("postgres");
}
Expand All @@ -260,6 +264,38 @@ public boolean isSqlServer(final DBInfo dbInfo) {
return "sqlserver".equals(dbInfo.getType());
}

/**
* Executes `connection.setClientInfo("OCSID.ACTION", traceContext)` statement on the Oracle DB to
* set the trace parent in `v$session.action`. This is used because it isn't possible to propagate
* trace parent with the comment.
*
* @param span The span of the instrumented statement
* @param connection The same connection as the one that will be used for the actual statement
*/
public void setAction(AgentSpan span, Connection connection) {
try {

Integer priority = span.forceSamplingDecision();
if (priority == null) {
return;
}
final String traceContext = "_DD_" + DECORATE.traceParent(span, priority);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe extract "_DD_" to a constant, like OracleActionPrefix or something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed


connection.setClientInfo("OCSID.ACTION", traceContext);

} catch (Throwable e) {
log.debug(
"Failed to set extra DBM data in application_name for trace {}. "
+ "To disable this behavior, set trace_prepared_statements to 'false'. "
+ "See https://docs.datadoghq.com/database_monitoring/connect_dbm_and_apm/ for more info.{}",
nenadnoveljic marked this conversation as resolved.
Show resolved Hide resolved
span.getTraceId().toHexString(),
e);
DECORATE.onError(span, e);
} finally {
span.setTag("_dd.dbm_trace_injected", true);
}
}

/**
* Executes a `SET CONTEXT_INFO` statement on the DB with the active trace ID and the given span
* ID. This context will be "attached" to future queries on the same connection. See <a
Expand Down
17 changes: 12 additions & 5 deletions ...ation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,18 @@ public static AgentScope onEnter(
final AgentSpan span;
final boolean isSqlServer = DECORATE.isSqlServer(dbInfo);

if (isSqlServer && INJECT_COMMENT && injectTraceContext) {
// The span ID is pre-determined so that we can reference it when setting the context
final long spanID = DECORATE.setContextInfo(connection, dbInfo);
// we then force that pre-determined span ID for the span covering the actual query
span = AgentTracer.get().buildSpan(DATABASE_QUERY).withSpanId(spanID).start();
if (INJECT_COMMENT && injectTraceContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: What about having only one startSpan() as default case, and grouping custom span decoration together like:

if (INJECT_COMMENT && injectTraceContext && (isSqlServer || isOracle)) {
  if (isSqlServer) { 
    // DECORATE + build/start
  } else if (isOracle) {
    // start + DECORATE
  }
 } else {
   startSpan()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Is it OK to merge the PR as it is, and do the change when we remove the instrumentation span for SQL Server? The line 96 (span = AgentTracer.get().buildSpan(DATABASE_QUERY).withSpanId(spanID).start();) will then become span = startSpan(DATABASE_QUERY);, so we'll be able to remove span = startSpan(DATABASE_QUERY); from all branches.

if (isSqlServer) {
// The span ID is pre-determined so that we can reference it when setting the context
final long spanID = DECORATE.setContextInfo(connection, dbInfo);
// we then force that pre-determined span ID for the span covering the actual query
span = AgentTracer.get().buildSpan(DATABASE_QUERY).withSpanId(spanID).start();
} else if (DECORATE.isOracle(dbInfo)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's weird that we have a decorate method for isOracle while isSqlServer is computed here. I feel like this should be unified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed now

span = startSpan(DATABASE_QUERY);
DECORATE.setAction(span, connection);
} else {
span = startSpan(DATABASE_QUERY);
}
} else {
span = startSpan(DATABASE_QUERY);
}
Expand Down
2 changes: 1 addition & 1 deletion dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCDecoratorTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ abstract class JDBCDecoratorTest extends AgentTestRunner {

where:
dbType | expectedByType
"oracle" | false
"oracle" | true
"sqlserver" | true
"mysql" | true
"postgresql" | true
Expand Down
Loading
Loading