Support SELECT/INSERT/UPDATE/DELETE db operation + span names #1253

damemi · 2024-11-07T19:37:00Z

Database spans MUST follow the overall [guidelines for span names](https://github.com/open-telemetry/opentelemetry-specification/tree/v1.37.0/specification/trace/api.md#span).

The span name SHOULD be {db.query.summary} if a summary is available.

If no summary is available, the span name SHOULD be {db.operation.name} {target} provided that a (low-cardinality) db.operation.name is available (see below for the exact definition of the [{target}](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md#target-placeholder) placeholder).

If a (low-cardinality) db.operation.name is not available, database span names SHOULD default to the [{target}](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md#target-placeholder).

If neither {db.operation.name} nor {target} are available, span name SHOULD be {db.system}.

Semantic conventions for individual database systems MAY specify different span name format.

The {target} SHOULD describe the entity that the operation is performed against and SHOULD adhere to one of the following values, provided they are accessible:

db.collection.name SHOULD be used for data manipulation operations or operations on a database collection.
db.namespace SHOULD be used for operations on a specific database namespace.
server.address:server.port SHOULD be used for other operations not targeting any specific database(s) or collection(s)
If a corresponding {target} value is not available for a specific operation, the instrumentation SHOULD omit the {target}. For example, for an operation describing SQL query on an anonymous table like SELECT * FROM (SELECT * FROM table) t, span name should be SELECT.

We don't have collection (table), namespace, or server.address readily available (right now), but we can at least parse certain operations. We can also support more over time. So this gets us at least a couple useful span names besides DB (fallback for unsupported operations)

This means we can also set db.operation.name with that same value when it is available.

damemi · 2024-11-07T19:59:42Z

internal/pkg/instrumentation/bpf/database/sql/probe.go

@@ -8,6 +8,8 @@ import (
 	"os"
 	"strconv"

+	sql "github.com/xwb1989/sqlparser"


I looked through a couple sql parsing libraries, this isn't the most maintained... https://github.com/krasun/gosqlparser seems good but for some reason I don't think it supports * in queries (??). For what we're doing now I think it's fine but this could use another look some day

internal/pkg/instrumentation/bpf/database/sql/probe.go

internal/test/e2e/databasesql/main.go

internal/pkg/instrumentation/bpf/database/sql/probe.go

MrAlias · 2024-11-20T17:44:40Z

internal/pkg/instrumentation/bpf/database/sql/probe.go

+
+				if operation != "" {
+					span.Attributes().PutStr(string(semconv.DBOperationNameKey), operation)
+					span.SetName(operation)


Should we try to build the {db.query.summary} or {db.operation.name} {target} here given we have already parsed the query?

I was originally doing db.query.summary, but we don't have any of the values for {target} (db.collection, db.namespace, or server address). So, it recommends just using the operation:

If a corresponding {target} value is not available for a specific operation, the instrumentation SHOULD omit the {target}. For example, for an operation describing SQL query on an anonymous table like SELECT * FROM (SELECT * FROM table) t, span name should be SELECT.

The table might be defined in the query statement though, right? The query parsing library should be able to get this, right?

Not that I could find, unfortunately :\ The first library I tried (https://github.com/krasun/gosqlparser) does have that, which is how I was building it at first. But that library breaks when it tries to parse a * character (like SELECT * FROM). Which makes no sense to me, but that's my understanding from trying to debug it and read that library's code. I'm open to add a TODO here for this though because maybe I'm just missing something

I think this should work, right?

package main import ( "fmt" "github.com/xwb1989/sqlparser" ) // Parse takes a SQL query string and returns the parsed query statement type // and table name, or an error if parsing failed. func Parse(query string) (string, string, error) { stmt, err := sqlparser.Parse(query) if err != nil { return "", "", fmt.Errorf("failed to parse query: %w", err) } switch stmt := stmt.(type) { case *sqlparser.Select: return "SELECT", getTableName(stmt.From), nil case *sqlparser.Update: return "UPDATE", getTableName(stmt.TableExprs), nil case *sqlparser.Insert: return "INSERT", stmt.Table.Name.String(), nil case *sqlparser.Delete: return "DELETE", getTableName(stmt.TableExprs), nil default: return "UNKNOWN", "", fmt.Errorf("unsupported statement type") } } // getTableName extracts the table name from a SQL node. func getTableName(node sqlparser.SQLNode) string { switch tableExpr := node.(type) { case sqlparser.TableName: return tableExpr.Name.String() case sqlparser.TableExprs: for _, expr := range tableExpr { if tableName, ok := expr.(*sqlparser.AliasedTableExpr); ok { if name, ok := tableName.Expr.(sqlparser.TableName); ok { return name.Name.String() } } } } return "" } func main() { queries := []string{ "SELECT * FROM users WHERE id = 1", "SELECT id, name FROM users WHERE id = 1", "INSERT INTO orders (id, amount) VALUES (1, 100)", "UPDATE products SET price = 19.99 WHERE id = 10", "DELETE FROM sessions WHERE expired = true", "CREATE TABLE logs (id INT, message TEXT)", } for _, query := range queries { fmt.Println("Query: ", query) statement, table, err := Parse(query) if err != nil { fmt.Println("Error:", err) continue } fmt.Printf("Statement: %s, Table: %s\n", statement, table) } }

$ go run . Query: SELECT * FROM users WHERE id = 1 Statement: SELECT, Table: users Query: SELECT id, name FROM users WHERE id = 1 Statement: SELECT, Table: users Query: INSERT INTO orders (id, amount) VALUES (1, 100) Statement: INSERT, Table: orders Query: UPDATE products SET price = 19.99 WHERE id = 10 Statement: UPDATE, Table: products Query: DELETE FROM sessions WHERE expired = true Statement: DELETE, Table: sessions Query: CREATE TABLE logs (id INT, message TEXT) Error: unsupported statement type

FWIW, looking at vitess, it seems like it should be able to support more statement types in the table lookup (i.e. table create).

Using "vitess.io/vitess/go/vt/sqlparser":

package main import ( "fmt" "vitess.io/vitess/go/vt/sqlparser" ) // Parse takes a SQL query string and returns the parsed query statement type // and table name(s), or an error if parsing failed. func Parse(query string) (string, []string, error) { p, err := sqlparser.New(sqlparser.Options{}) if err != nil { return "", nil, fmt.Errorf("failed to create parser: %w", err) } stmt, err := p.Parse(query) if err != nil { return "", nil, fmt.Errorf("failed to parse query: %w", err) } var statementType string var tables []string switch stmt := stmt.(type) { case *sqlparser.Select: statementType = "SELECT" tables = extractTables(stmt.From) case *sqlparser.Update: statementType = "UPDATE" tables = extractTables(stmt.TableExprs) case *sqlparser.Insert: statementType = "INSERT" tables = []string{stmt.Table.TableNameString()} case *sqlparser.Delete: statementType = "DELETE" tables = extractTables(stmt.TableExprs) case *sqlparser.CreateTable: statementType = "CREATE TABLE" tables = []string{stmt.Table.Name.String()} case *sqlparser.AlterTable: statementType = "ALTER TABLE" tables = []string{stmt.Table.Name.String()} case *sqlparser.DropTable: statementType = "DROP TABLE" for _, table := range stmt.FromTables { tables = append(tables, table.Name.String()) } case *sqlparser.CreateDatabase: statementType = "CREATE DATABASE" tables = []string{stmt.DBName.String()} case *sqlparser.DropDatabase: statementType = "DROP DATABASE" tables = []string{stmt.DBName.String()} case *sqlparser.TruncateTable: statementType = "TRUNCATE TABLE" tables = []string{stmt.Table.Name.String()} default: return "UNKNOWN", nil, fmt.Errorf("unsupported statement type") } return statementType, tables, nil } // extractTables extracts table names from a list of SQL nodes. func extractTables(exprs sqlparser.TableExprs) []string { var tables []string for _, expr := range exprs { switch tableExpr := expr.(type) { case *sqlparser.AliasedTableExpr: if name, ok := tableExpr.Expr.(sqlparser.TableName); ok { tables = append(tables, name.Name.String()) } } } return tables } func main() { queries := []string{ "SELECT * FROM users WHERE id = 1", "SELECT id, name FROM users WHERE id = 1", "INSERT INTO users (id, name) VALUES (1, 'Alice')", "UPDATE users SET name = 'Bob' WHERE id = 1", "DELETE FROM users WHERE id = 1", "CREATE TABLE users (id INT, name VARCHAR(100))", "ALTER TABLE users ADD COLUMN age INT", "DROP TABLE users", "CREATE DATABASE test_db", "DROP DATABASE test_db", "TRUNCATE TABLE users", } for _, query := range queries { fmt.Println("Query: ", query) statement, tables, err := Parse(query) if err != nil { fmt.Printf("Error parsing query: %s\nQuery: %s\n\n", err, query) continue } fmt.Printf("Statement: %s, Tables/DBs: %v\n", statement, tables) } }

$ go run . Query: SELECT * FROM users WHERE id = 1 Statement: SELECT, Tables/DBs: [users] Query: SELECT id, name FROM users WHERE id = 1 Statement: SELECT, Tables/DBs: [users] Query: INSERT INTO users (id, name) VALUES (1, 'Alice') Statement: INSERT, Tables/DBs: [users] Query: UPDATE users SET name = 'Bob' WHERE id = 1 Statement: UPDATE, Tables/DBs: [users] Query: DELETE FROM users WHERE id = 1 Statement: DELETE, Tables/DBs: [users] Query: CREATE TABLE users (id INT, name VARCHAR(100)) Statement: CREATE TABLE, Tables/DBs: [users] Query: ALTER TABLE users ADD COLUMN age INT Statement: ALTER TABLE, Tables/DBs: [users] Query: DROP TABLE users Statement: DROP TABLE, Tables/DBs: [users] Query: CREATE DATABASE test_db Statement: CREATE DATABASE, Tables/DBs: [test_db] Query: DROP DATABASE test_db Statement: DROP DATABASE, Tables/DBs: [test_db] Query: TRUNCATE TABLE users Statement: TRUNCATE TABLE, Tables/DBs: [users]

Updated to use vitess with your suggestion, looks good to me, ptal!

internal/pkg/instrumentation/bpf/database/sql/probe.go

MrAlias · 2024-11-20T17:48:43Z

go.mod

@@ -22,6 +22,7 @@ require (
 	github.com/hashicorp/go-version v1.7.0
 	github.com/pkg/errors v0.9.1
 	github.com/stretchr/testify v1.9.0
+	github.com/xwb1989/sqlparser v0.0.0-20180606152119-120387863bf2


Could we instead use the code this was based on: https://github.com/vitessio/vitess/tree/main/go/vt/sqlparser

It seems like this was a fork of that repository¹, but it hasn't been maintained or synced since that fork.

Would it be too large of a dependency to just rely on vitess directly?

Footnotes

https://github.com/xwb1989/sqlparser?tab=readme-ov-file#notice ↩

I did think about using vitess directly, but it seemed like a lot more than what we needed. Since this is just an implementation detail, we could refactor it later if we want but I think this is the best option for right now

I'm concerned when looking through all the changes that have been applied to the upstream code over the past 6 years, there's a considerable amount of changes. Notably:

There are many additional statement types that can be parsed: https://pkg.go.dev/vitess.io/vitess/go/vt/sqlparser#StatementType

The statement type itself can print its name meaning we will not need to add an additional switch statement: https://pkg.go.dev/vitess.io/vitess/go/vt/sqlparser#StatementType.String

The code is not owned by Google, it is licensed by a CNCF project

Make Parser Memory Usage More Efficient vitessio/vitess#4174

Misc fixes vitessio/vitess#2395 which addressed:

Support double quoted alias names vitessio/vitess#1953

sqlparse not support field alias chinese vitessio/vitess#2184

[parser] use table_alias for ENGINE option in CREATE TABLE stmt vitessio/vitess#8307

Should we make our own fork of this pacakge?

Can we maybe get a better understanding of dependency size by looking at what the binary size is based on the possible dependencies?

I'll admit it looks like you've dug into it deeper than I did, hah. These are some great points so maybe that is the better approach. I'll try switching it to that

The binary size went from 30MB->37MB with vitess included, that's not small but I'm not personally concerned about it and it could be something we look to refactor later. wdyt?

damemi · 2024-12-02T21:16:24Z

internal/test/e2e/databasesql/traces.json

+                {
+                  "key": "db.query.text",
+                  "value": {
+                    "stringValue": "syntax error"


will follow up to parse error response codes from databases separately, and mark them as error spans appropriately. Don't want to grow this PR too much and this is equivalent to what we do now

MrAlias

This looks like an exciting change. Mainly small comments outside of the compatibility policy issue.

Do we need to support older versions of Go? Would that break any downstream users of go.opentelemetry.io/auto?

MrAlias · 2024-12-02T21:59:07Z

docs/configuration.md

@@ -27,6 +27,7 @@ Alternatively, you can add support for additional or different configurations by
 | Environment variable                | Description                                            | Default value |
 |-------------------------------------|--------------------------------------------------------|---------------|
 | `OTEL_GO_AUTO_INCLUDE_DB_STATEMENT` | Sets whether to include SQL queries in the trace data. |               |
+| `OTEL_GO_AUTO_INCLUDE_DB_OPERATION` | Sets whether to include SQL operation in the trace data. |               |


It is not valid to have OTEL_GO_AUTO_INCLUDE_DB_OPERATION without OTEL_GO_AUTO_INCLUDE_DB_STATEMENT, right? If so we should probably point that out.
Also, should we state the exact attributes we'll add? i.e db.collection.name db.operation.name etc`

MrAlias · 2024-12-02T22:01:23Z

go.mod

@@ -1,6 +1,6 @@
 module go.opentelemetry.io/auto

-go 1.22.7
+go 1.23.2


Hmm, this might be problematic. This would violate our compatibility policy: https://github.com/open-telemetry/opentelemetry-go-instrumentation?tab=readme-ov-file#compatibility

I agree. this looks quite aggressive.
For reference, when I wanted to update the dependency of this repo I had to bump the go.mod to 1.22.7 which is what we require today (odigos-io/odigos@fb167f8), and we have quite a lot of other dependencies in Odigos and none of them required this relatively new version.

MrAlias · 2024-12-02T22:03:27Z

internal/pkg/instrumentation/bpf/database/sql/probe_test.go

+	err := os.Setenv(IncludeDBOperationEnvVar, "true")
+	assert.NoError(t, err)


Suggested change

err := os.Setenv(IncludeDBOperationEnvVar, "true")

assert.NoError(t, err)

t.Setenv(IncludeDBOperationEnvVar, "true")

MrAlias · 2024-12-02T22:06:01Z

internal/test/e2e/databasesql/main.go

 	}

-	logger.Info("queryDb called")
+	logger.Info(fmt.Sprintf("Query: %s", query))


nit: use the structured elements of the logger to handle this.

Suggested change

logger.Info(fmt.Sprintf("Query: %s", query))

logger.Info("queryDB called", "query", query))

RonFed · 2024-12-03T07:26:55Z

LICENSES/libbpf/bpf.h

What's the reason for these changes? can you pleas add it to the PR description

this all came in when I ran make update-licenses (CI license check failed saying it needed updates). I agree it's weird that libbpf was also updated

It looks like the libbpf script wasn't run.

damemi requested a review from a team as a code owner November 7, 2024 19:37

damemi force-pushed the sql-semconv branch 3 times, most recently from 8f0e456 to 5340675 Compare November 7, 2024 19:53

damemi commented Nov 7, 2024

View reviewed changes

damemi force-pushed the sql-semconv branch 2 times, most recently from 89e64c7 to 7116a06 Compare November 8, 2024 19:49

RonFed reviewed Nov 8, 2024

View reviewed changes

internal/pkg/instrumentation/bpf/database/sql/probe.go Outdated Show resolved Hide resolved

internal/test/e2e/databasesql/main.go Outdated Show resolved Hide resolved

damemi force-pushed the sql-semconv branch 3 times, most recently from c809878 to 97ccfaf Compare November 20, 2024 16:23

MrAlias reviewed Nov 20, 2024

View reviewed changes

internal/pkg/instrumentation/bpf/database/sql/probe.go Outdated Show resolved Hide resolved

damemi force-pushed the sql-semconv branch from 97ccfaf to ef789ba Compare November 20, 2024 17:50

MrAlias reviewed Nov 20, 2024

View reviewed changes

damemi added 6 commits December 2, 2024 15:16

Support SELECT/INSERT/UPDATE/DELETE db operation + span names

6b3ad09

Add benchmark test for sql.Parse

3e2a0cf

Update logs in tests

b3633cd

Put DB Operation behind flag

dcb4e30

Set new env var in test

9559347

Add env var to docs

13639c3

damemi force-pushed the sql-semconv branch 2 times, most recently from d005d0f to cccbf66 Compare December 2, 2024 21:12

Switch to vitess sql and add more tests

264081d

damemi force-pushed the sql-semconv branch from cccbf66 to 264081d Compare December 2, 2024 21:15

damemi commented Dec 2, 2024

View reviewed changes

damemi added 2 commits December 2, 2024 16:17

make update-licenses

6635f3a

lint

55e9aa7

damemi force-pushed the sql-semconv branch from ffa764a to 55e9aa7 Compare December 2, 2024 21:35

MrAlias reviewed Dec 2, 2024

View reviewed changes

RonFed reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SELECT/INSERT/UPDATE/DELETE db operation + span names #1253

Support SELECT/INSERT/UPDATE/DELETE db operation + span names #1253

damemi commented Nov 7, 2024

damemi Nov 7, 2024

MrAlias Nov 20, 2024

damemi Nov 21, 2024

MrAlias Nov 21, 2024

damemi Nov 21, 2024

MrAlias Nov 22, 2024 •

edited

Loading

MrAlias Nov 22, 2024

damemi Dec 2, 2024

MrAlias Nov 20, 2024

damemi Nov 21, 2024

MrAlias Nov 21, 2024

MrAlias Nov 21, 2024

damemi Nov 21, 2024

damemi Dec 2, 2024

damemi Dec 2, 2024

MrAlias left a comment

MrAlias Dec 2, 2024

RonFed Dec 3, 2024

MrAlias Dec 2, 2024

RonFed Dec 3, 2024 •

edited

Loading

MrAlias Dec 2, 2024

MrAlias Dec 2, 2024

RonFed Dec 3, 2024

damemi Dec 3, 2024

MrAlias Dec 3, 2024

	\| `OTEL_GO_AUTO_INCLUDE_DB_OPERATION` \| Sets whether to include SQL operation in the trace data. \| \|
	\| `OTEL_GO_AUTO_PARSE_DB_STATEMENT` \| Sets whether to parse the SQL statement for trace data. \| \|

		err := os.Setenv(IncludeDBOperationEnvVar, "true")
		assert.NoError(t, err)

	err := os.Setenv(IncludeDBOperationEnvVar, "true")
	assert.NoError(t, err)
	t.Setenv(IncludeDBOperationEnvVar, "true")

	logger.Info(fmt.Sprintf("Query: %s", query))
	logger.Info("queryDB called", "query", query))

Support SELECT/INSERT/UPDATE/DELETE db operation + span names #1253

Are you sure you want to change the base?

Support SELECT/INSERT/UPDATE/DELETE db operation + span names #1253

Conversation

damemi commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonFed Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias Nov 22, 2024 •

edited

Loading

RonFed Dec 3, 2024 •

edited

Loading