Skip to content

Update SQL span name for procedures #7557

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

Merged
merged 1 commit into from
Jan 19, 2023
Merged
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
Original file line number Diff line number Diff line change
@@ -6,19 +6,14 @@
package io.opentelemetry.instrumentation.api.db;

import com.google.auto.value.AutoValue;
import java.util.function.Function;
import javax.annotation.Nullable;

@AutoValue
public abstract class SqlStatementInfo {

public static SqlStatementInfo create(
@Nullable String fullStatement, @Nullable String operation, @Nullable String table) {
return new AutoValue_SqlStatementInfo(fullStatement, operation, table);
}

public SqlStatementInfo mapTable(Function<String, String> mapper) {
return SqlStatementInfo.create(getFullStatement(), getOperation(), mapper.apply(getTable()));
@Nullable String fullStatement, @Nullable String operation, @Nullable String identifier) {
return new AutoValue_SqlStatementInfo(fullStatement, operation, identifier);
}

@Nullable
@@ -28,5 +23,5 @@ public SqlStatementInfo mapTable(Function<String, String> mapper) {
public abstract String getOperation();

@Nullable
public abstract String getTable();
public abstract String getMainIdentifier();
}
Original file line number Diff line number Diff line change
@@ -25,11 +25,12 @@ public static <REQUEST> SpanNameExtractor<REQUEST> create(

/**
* Returns a {@link SpanNameExtractor} that constructs the span name according to DB semantic
* conventions: {@code <db.operation> <db.name>.<table>}.
* conventions: {@code <db.operation> <db.name>.<identifier>}.
*
* @see SqlStatementInfo#getOperation() used to extract {@code <db.operation>}.
* @see DbClientAttributesGetter#name(Object) used to extract {@code <db.name>}.
* @see SqlStatementInfo#getTable() used to extract {@code <db.table>}.
* @see SqlStatementInfo#getMainIdentifier() used to extract {@code <db.table>} or stored
* procedure name.
*/
public static <REQUEST> SpanNameExtractor<REQUEST> create(
SqlClientAttributesGetter<REQUEST> getter) {
@@ -40,24 +41,24 @@ public static <REQUEST> SpanNameExtractor<REQUEST> create(

private DbClientSpanNameExtractor() {}

protected String computeSpanName(String dbName, String operation, String table) {
protected String computeSpanName(String dbName, String operation, String mainIdentifier) {
if (operation == null) {
return dbName == null ? DEFAULT_SPAN_NAME : dbName;
}

StringBuilder name = new StringBuilder(operation);
if (dbName != null || table != null) {
if (dbName != null || mainIdentifier != null) {
name.append(' ');
}
// skip db name if table already has a db name prefixed to it
if (dbName != null && (table == null || table.indexOf('.') == -1)) {
// skip db name if identifier already has a db name prefixed to it
if (dbName != null && (mainIdentifier == null || mainIdentifier.indexOf('.') == -1)) {
name.append(dbName);
if (table != null) {
if (mainIdentifier != null) {
name.append('.');
}
}
if (table != null) {
name.append(table);
if (mainIdentifier != null) {
name.append(mainIdentifier);
}
return name.toString();
}
@@ -82,7 +83,7 @@ public String extract(REQUEST request) {
private static final class SqlClientSpanNameExtractor<REQUEST>
extends DbClientSpanNameExtractor<REQUEST> {

// a dedicated sanitizer just for extracting the operation and table name
// a dedicated sanitizer just for extracting the operation and identifier name
private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true);

private final SqlClientAttributesGetter<REQUEST> getter;
@@ -96,7 +97,7 @@ public String extract(REQUEST request) {
String dbName = getter.name(request);
SqlStatementInfo sanitizedStatement = sanitizer.sanitize(getter.rawStatement(request));
return computeSpanName(
dbName, sanitizedStatement.getOperation(), sanitizedStatement.getTable());
dbName, sanitizedStatement.getOperation(), sanitizedStatement.getMainIdentifier());
}
}
}
Original file line number Diff line number Diff line change
@@ -43,6 +43,8 @@ public static <REQUEST, RESPONSE> SqlClientAttributesExtractorBuilder<REQUEST, R
return new SqlClientAttributesExtractorBuilder<>(getter);
}

private static final String SQL_CALL = "CALL";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: just a blank line to separate constants from fields:

Suggested change
private static final String SQL_CALL = "CALL";
private static final String SQL_CALL = "CALL";


private final AttributeKey<String> dbTableAttribute;
private final SqlStatementSanitizer sanitizer;

@@ -60,8 +62,11 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
super.onStart(attributes, parentContext, request);

SqlStatementInfo sanitizedStatement = sanitizer.sanitize(getter.rawStatement(request));
String operation = sanitizedStatement.getOperation();
internalSet(attributes, SemanticAttributes.DB_STATEMENT, sanitizedStatement.getFullStatement());
internalSet(attributes, SemanticAttributes.DB_OPERATION, sanitizedStatement.getOperation());
internalSet(attributes, dbTableAttribute, sanitizedStatement.getTable());
internalSet(attributes, SemanticAttributes.DB_OPERATION, operation);
if (!SQL_CALL.equals(operation)) {
internalSet(attributes, dbTableAttribute, sanitizedStatement.getMainIdentifier());
}
}
}
70 changes: 50 additions & 20 deletions instrumentation-api-semconv/src/main/jflex/SqlSanitizer.jflex
Original file line number Diff line number Diff line change
@@ -63,13 +63,13 @@ WHITESPACE = [ \t\r\n]+
}

/** @return text matched by current token without enclosing double quotes or backticks */
private String readTableName() {
String tableName = yytext();
if (tableName != null && ((tableName.startsWith("\"") && tableName.endsWith("\""))
|| (tableName.startsWith("`") && tableName.endsWith("`")))) {
tableName = tableName.substring(1, tableName.length() - 1);
private String readIdentifierName() {
String identifierName = yytext();
if (identifierName != null && ((identifierName.startsWith("\"") && identifierName.endsWith("\""))
|| (identifierName.startsWith("`") && identifierName.endsWith("`")))) {
identifierName = identifierName.substring(1, identifierName.length() - 1);
}
return tableName;
return identifierName;
}

// you can reference a table in the FROM clause in one of the following ways:
@@ -92,7 +92,7 @@ WHITESPACE = [ \t\r\n]+
}

private static abstract class Operation {
String mainTable = null;
String mainIdentifier = null;

/** @return true if all statement info is gathered */
boolean handleFrom() {
@@ -119,8 +119,13 @@ WHITESPACE = [ \t\r\n]+
return false;
}

/** @return true if all statement info is gathered */
boolean handleNext() {
return false;
}

SqlStatementInfo getResult(String fullStatement) {
return SqlStatementInfo.create(fullStatement, getClass().getSimpleName().toUpperCase(java.util.Locale.ROOT), mainTable);
return SqlStatementInfo.create(fullStatement, getClass().getSimpleName().toUpperCase(java.util.Locale.ROOT), mainIdentifier);
}
}

@@ -152,13 +157,13 @@ WHITESPACE = [ \t\r\n]+
}

// subquery in WITH or SELECT clause, before main FROM clause; skipping
mainTable = null;
mainIdentifier = null;
return true;
}

boolean handleJoin() {
// for SELECT statements with joined tables there's no main table
mainTable = null;
mainIdentifier = null;
return true;
}

@@ -173,17 +178,17 @@ WHITESPACE = [ \t\r\n]+

// SELECT FROM (subquery) case
if (parenLevel != 0) {
mainTable = null;
mainIdentifier = null;
return true;
}

// whenever >1 table is used there is no main table (e.g. unions)
if (mainTableSetAlready) {
mainTable = null;
mainIdentifier = null;
return true;
}

mainTable = readTableName();
mainIdentifier = readIdentifierName();
mainTableSetAlready = true;
expectingTableName = false;
// start counting identifiers after encountering main from clause
@@ -199,7 +204,7 @@ WHITESPACE = [ \t\r\n]+
// any other list that can appear later needs at least 4 idents)
if (identifiersAfterMainFromClause > 0
&& identifiersAfterMainFromClause <= FROM_TABLE_REF_MAX_IDENTIFIERS) {
mainTable = null;
mainIdentifier = null;
return true;
}
return false;
@@ -219,7 +224,7 @@ WHITESPACE = [ \t\r\n]+
return false;
}

mainTable = readTableName();
mainIdentifier = readIdentifierName();
return true;
}
}
@@ -237,21 +242,33 @@ WHITESPACE = [ \t\r\n]+
return false;
}

mainTable = readTableName();
mainIdentifier = readIdentifierName();
return true;
}
}

private class Update extends Operation {
boolean handleIdentifier() {
mainTable = readTableName();
mainIdentifier = readIdentifierName();
return true;
}
}

private class Call extends Operation {
boolean handleIdentifier() {
mainIdentifier = readIdentifierName();
return true;
}

boolean handleNext() {
mainIdentifier = null;
return true;
}
}

private class Merge extends Operation {
boolean handleIdentifier() {
mainTable = readTableName();
mainIdentifier = readIdentifierName();
return true;
}
}
@@ -298,14 +315,20 @@ WHITESPACE = [ \t\r\n]+
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
"CALL" {
if (!insideComment) {
setOperation(new Call());
}
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
"MERGE" {
if (!insideComment) {
setOperation(new Merge());
}
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}

"FROM" {
if (!insideComment && !extractionDone) {
if (operation == NoOp.INSTANCE) {
@@ -332,6 +355,13 @@ WHITESPACE = [ \t\r\n]+
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
"NEXT" {
if (!insideComment && !extractionDone) {
extractionDone = operation.handleNext();
}
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
{COMMA} {
if (!insideComment && !extractionDone) {
extractionDone = operation.handleComma();
@@ -407,4 +437,4 @@ WHITESPACE = [ \t\r\n]+
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
}
}
Original file line number Diff line number Diff line change
@@ -232,12 +232,13 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th

static class SimplifyArgs implements ArgumentsProvider {

static Function<String, SqlStatementInfo> expect(String operation, String table) {
return sql -> SqlStatementInfo.create(sql, operation, table);
static Function<String, SqlStatementInfo> expect(String operation, String identifier) {
return sql -> SqlStatementInfo.create(sql, operation, identifier);
}

static Function<String, SqlStatementInfo> expect(String sql, String operation, String table) {
return ignored -> SqlStatementInfo.create(sql, operation, table);
static Function<String, SqlStatementInfo> expect(
String sql, String operation, String identifier) {
return ignored -> SqlStatementInfo.create(sql, operation, identifier);
}

@Override
@@ -275,6 +276,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
Arguments.of("/* update comment */ select * from table1", expect("SELECT", "table1")),
Arguments.of("select /*((*/abc from table", expect("SELECT", "table")),
Arguments.of("SeLeCT * FrOm TAblE", expect("SELECT", "table")),
Arguments.of("select next value in hibernate_sequence", expect("SELECT", null)),

// hibernate/jpa
Arguments.of("FROM schema.table", expect("SELECT", "schema.table")),
@@ -308,6 +310,12 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
expect("update \"my table\" set answer=?", "UPDATE", "my table")),
Arguments.of("update /*table", expect("UPDATE", null)),

// Call
Arguments.of("call test_proc()", expect("CALL", "test_proc")),
Arguments.of("call test_proc", expect("CALL", "test_proc")),
Arguments.of("call next value in hibernate_sequence", expect("CALL", null)),
Copy link
Member

Choose a reason for hiding this comment

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

(not sure what happens, but may be a good test also)

Suggested change
Arguments.of("call next value in hibernate_sequence", expect("CALL", null)),
Arguments.of("call next value in hibernate_sequence", expect("CALL", null)),
Arguments.of("select next value in hibernate_sequence", expect("SELECT", null)),

Copy link
Member

Choose a reason for hiding this comment

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

@akats7 sorry it looks like this comment got resolved accidentally, does it make sense to add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trask Yep I did add it, its in the //Select section of the test cases

Arguments.of("call db.test_proc", expect("CALL", "db.test_proc")),

// Merge
Arguments.of("merge into table", expect("MERGE", "table")),
Arguments.of("merge into `my table`", expect("MERGE", "my table")),
Original file line number Diff line number Diff line change
@@ -112,7 +112,7 @@ class SpringJpaTest extends AgentInstrumentationSpecification {
}
if (!isHibernate4) {
span(2) {
name "test"
name "CALL test"
kind CLIENT
childOf span(1)
attributes {
@@ -121,6 +121,7 @@ class SpringJpaTest extends AgentInstrumentationSpecification {
"$SemanticAttributes.DB_USER" "sa"
"$SemanticAttributes.DB_STATEMENT" "call next value for hibernate_sequence"
"$SemanticAttributes.DB_CONNECTION_STRING" "hsqldb:mem:"
"$SemanticAttributes.DB_OPERATION" "CALL"
}
}
span(3) {
Original file line number Diff line number Diff line change
@@ -22,8 +22,8 @@ public static String getOperationNameForQuery(String query) {
SqlStatementInfo info = sanitizer.sanitize(query);
if (info.getOperation() != null) {
operation = info.getOperation();
if (info.getTable() != null) {
operation += " " + info.getTable();
if (info.getMainIdentifier() != null) {
operation += " " + info.getMainIdentifier();
}
}
return operation;
Original file line number Diff line number Diff line change
@@ -104,7 +104,7 @@ class ProcedureCallTest extends AgentInstrumentationSpecification {
}
}
span(2) {
name "test"
name "CALL test.TEST_PROC"
kind CLIENT
childOf span(1)
attributes {
@@ -113,6 +113,7 @@ class ProcedureCallTest extends AgentInstrumentationSpecification {
"$SemanticAttributes.DB_USER" "sa"
"$SemanticAttributes.DB_STATEMENT" "{call TEST_PROC()}"
"$SemanticAttributes.DB_CONNECTION_STRING" "hsqldb:mem:"
"$SemanticAttributes.DB_OPERATION" "CALL"
}
}
span(3) {
Original file line number Diff line number Diff line change
@@ -111,7 +111,7 @@ abstract class AbstractSpringJpaTest<ENTITY, REPOSITORY extends JpaRepository<EN
if (!isHibernate4) {
offset = 1
span(1) {
name "test"
name "CALL test"
kind CLIENT
childOf span(0)
attributes {
@@ -120,6 +120,7 @@ abstract class AbstractSpringJpaTest<ENTITY, REPOSITORY extends JpaRepository<EN
"$SemanticAttributes.DB_USER" "sa"
"$SemanticAttributes.DB_CONNECTION_STRING" "hsqldb:mem:"
"$SemanticAttributes.DB_STATEMENT" ~/^call next value for /
"$SemanticAttributes.DB_OPERATION" "CALL"
}
}
}