-
Notifications
You must be signed in to change notification settings - Fork 329
Fix Oracle DBM trace correlation #10829
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import datadog.trace.agent.test.InstrumentationSpecification | ||
| import datadog.trace.api.config.TraceInstrumentationConfig | ||
| import test.TestConnection | ||
| import test.TestDatabaseMetaData | ||
| import test.TestPreparedStatement | ||
| import test.TestStatement | ||
|
|
||
| /** | ||
| * Tests that Oracle DBM SQL comment injection produces the correct dddbs and dddb tags. | ||
| * | ||
| * Bug 1: dddbs was populated with the generic type string "oracle" instead of the SID/service name. | ||
| * Bug 2: dddb was never injected because the Oracle URL parser sets instance, not db. | ||
| */ | ||
| abstract class OracleInjectionTestBase extends InstrumentationSpecification { | ||
| @Override | ||
| void configurePreAgent() { | ||
| super.configurePreAgent() | ||
|
|
||
| injectSysConfig(TraceInstrumentationConfig.DB_DBM_PROPAGATION_MODE_MODE, "full") | ||
| injectSysConfig("service.name", "my_service_name") | ||
| } | ||
|
|
||
| static query = "SELECT 1" | ||
|
|
||
| // Note: the URL parser lowercases the full URL before extraction, so identifiers are lowercase. | ||
| static sidUrl = "jdbc:oracle:thin:@localhost:1521:BENEDB" | ||
| static serviceNameUrl = "jdbc:oracle:thin:@//localhost:1521/MYSERVICE" | ||
|
|
||
| static sidInjection = "ddps='my_service_name',dddbs='benedb',ddh='localhost',dddb='benedb'" | ||
| static serviceNameInjection = "ddps='my_service_name',dddbs='myservice',ddh='localhost',dddb='myservice'" | ||
|
|
||
| TestConnection createOracleConnection(String url) { | ||
| def connection = new TestConnection(false) | ||
| def metadata = new TestDatabaseMetaData() | ||
| metadata.setURL(url) | ||
| connection.setMetaData(metadata) | ||
| return connection | ||
| } | ||
| } | ||
|
|
||
| class OracleInjectionForkedTest extends OracleInjectionTestBase { | ||
|
|
||
| def "Oracle prepared statement injects instance name in dddbs and dddb"() { | ||
| setup: | ||
| def connection = createOracleConnection(url) | ||
|
|
||
| when: | ||
| def statement = connection.prepareStatement(query) as TestPreparedStatement | ||
| statement.execute() | ||
|
|
||
| then: | ||
| statement.sql == "/*${expected}*/ ${query}" | ||
|
|
||
| where: | ||
| url | expected | ||
| sidUrl | sidInjection | ||
| serviceNameUrl | serviceNameInjection | ||
| } | ||
|
|
||
| def "Oracle single statement injects instance name in dddbs and dddb"() { | ||
| setup: | ||
| def connection = createOracleConnection(url) | ||
|
|
||
| when: | ||
| def statement = connection.createStatement() as TestStatement | ||
nenadnoveljic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| statement.executeQuery(query) | ||
|
|
||
| then: | ||
| // Oracle uses v$session.action for trace context, so no traceparent in comment | ||
| statement.sql == "/*${expected}*/ ${query}" | ||
|
|
||
| where: | ||
| url | expected | ||
| sidUrl | sidInjection | ||
| serviceNameUrl | serviceNameInjection | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1146,6 +1146,43 @@ class RemoteDBMTraceInjectedForkedTest extends RemoteJDBCInstrumentationTest { | |
| final databaseNaming = new DatabaseNamingV1() | ||
| return databaseNaming.normalizedName(dbType) | ||
| } | ||
|
|
||
| def "Oracle DBM comment contains instance name in dddbs and dddb, not generic type string"() { | ||
| setup: | ||
| // Use a query text unlikely to already be in v$sql cursor cache | ||
| def markerQuery = "SELECT 1729 /* oracle-dbm-fix-verify */ FROM dual" | ||
| def conn = connectTo(ORACLE, peerConnectionProps(ORACLE)) | ||
|
|
||
| when: | ||
| def stmt = conn.createStatement() | ||
| runUnderTrace("parent") { | ||
| stmt.execute(markerQuery) | ||
| } | ||
| TEST_WRITER.waitForTraces(1) | ||
|
|
||
| then: | ||
| // Connect as system to read v$sql — system shares the same password as the test user | ||
| // in the gvenzl/oracle-free image (both are set via ORACLE_PASSWORD). | ||
| def adminUrl = "jdbc:oracle:thin:@//${oracle.getHost()}:${oracle.getMappedPort(1521)}/freepdb1" | ||
| def adminConn = java.sql.DriverManager.getConnection(adminUrl, "system", oracle.getPassword()) | ||
| def rs = adminConn.createStatement().executeQuery( | ||
| "SELECT sql_fulltext FROM v\$sql " + | ||
| "WHERE sql_fulltext LIKE '%1729%' AND sql_fulltext LIKE '%dddbs%' " + | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This lookup can return the verifier query itself instead of the instrumented Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated with adding like predicate on the comment |
||
| "AND sql_fulltext LIKE '%oracle-dbm-fix-verify%' " + | ||
| "AND ROWNUM = 1" | ||
| ) | ||
| assert rs.next() : "Instrumented Oracle query not found in v\$sql — DBM comment may be missing" | ||
| def sqlText = rs.getString(1) | ||
| // dddbs and dddb should both carry the PDB/service name, not the generic "oracle" type string | ||
| assert sqlText.contains("dddbs='freepdb1'") : "Expected dddbs='freepdb1' in SQL comment, got: ${sqlText}" | ||
| assert sqlText.contains("dddb='freepdb1'") : "Expected dddb='freepdb1' in SQL comment, got: ${sqlText}" | ||
| assert !sqlText.contains("dddbs='oracle'") : "dddbs must not be the generic type string 'oracle': ${sqlText}" | ||
|
|
||
| cleanup: | ||
| adminConn?.close() | ||
| stmt?.close() | ||
| conn?.close() | ||
| } | ||
| } | ||
|
|
||
| class RemoteDBMTraceInjectedForkedTestTracePreparedStatements extends RemoteJDBCInstrumentationTest { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it seems that all the info to return the correct data is contained in dbInfo, couldn't we put this logic in
dbInfo.getDb()rather than having to write it explicitely here ?