From 13281aad24b2f44c4760d82880a5b9654568aae7 Mon Sep 17 00:00:00 2001 From: Eliott Bouhana Date: Wed, 20 Nov 2024 17:53:07 +0100 Subject: [PATCH 1/4] fix(injector: builtin: sql): driver is not registered from non-instrumented sql calls Signed-off-by: Eliott Bouhana --- _integration-tests/tests/sql/sql.go | 51 +++++++++++-------- internal/injector/builtin/generated.go | 11 +++- .../builtin/yaml/stdlib/database-sql.yml | 10 +++- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/_integration-tests/tests/sql/sql.go b/_integration-tests/tests/sql/sql.go index b65e5224..b2e8fcaf 100644 --- a/_integration-tests/tests/sql/sql.go +++ b/_integration-tests/tests/sql/sql.go @@ -23,32 +23,39 @@ type TestCase struct { } func (tc *TestCase) Setup(t *testing.T) { - var err error - tc.DB, err = sql.Open("sqlite3", "file::memory:") + //orchestrion:ignore + prepareConn, err := sql.Open("sqlite3", "file::memory:?cache=shared") require.NoError(t, err) - t.Cleanup(func() { - assert.NoError(t, tc.DB.Close()) - }) - _, err = tc.DB.ExecContext(context.Background(), - `CREATE TABLE IF NOT EXISTS notes ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - userid INTEGER, - content STRING, - created STRING -)`) - require.NoError(t, err) + func() { + defer prepareConn.Close() + + _, err = prepareConn.ExecContext(context.Background(), + `CREATE TABLE IF NOT EXISTS notes ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + userid INTEGER, + content STRING, + created STRING + )`) + require.NoError(t, err) - _, err = tc.DB.ExecContext(context.Background(), - `INSERT OR REPLACE INTO notes(userid, content, created) VALUES - (1, 'Hello, John. This is John. You are leaving a note for yourself. You are welcome and thank you.', datetime('now')), - (1, 'Hey, remember to mow the lawn.', datetime('now')), - (2, 'Reminder to submit that report by Thursday.', datetime('now')), - (2, 'Opportunities don''t happen, you create them.', datetime('now')), - (3, 'Pick up cabbage from the store on the way home.', datetime('now')), - (3, 'Review PR #1138', datetime('now') - );`) + _, err = prepareConn.ExecContext(context.Background(), + `INSERT OR REPLACE INTO notes(userid, content, created) VALUES + (1, 'Hello, John. This is John. You are leaving a note for yourself. You are welcome and thank you.', datetime('now')), + (1, 'Hey, remember to mow the lawn.', datetime('now')), + (2, 'Reminder to submit that report by Thursday.', datetime('now')), + (2, 'Opportunities don''t happen, you create them.', datetime('now')), + (3, 'Pick up cabbage from the store on the way home.', datetime('now')), + (3, 'Review PR #1138', datetime('now') + )`) + require.NoError(t, err) + }() + + tc.DB, err = sql.Open("sqlite3", "file::memory:?cache=shared") require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, tc.DB.Close()) + }) } func (tc *TestCase) Run(t *testing.T) { diff --git a/internal/injector/builtin/generated.go b/internal/injector/builtin/generated.go index 7519844f..fd528c65 100644 --- a/internal/injector/builtin/generated.go +++ b/internal/injector/builtin/generated.go @@ -1564,7 +1564,14 @@ var Aspects = [...]aspect.Aspect{ { JoinPoint: join.FunctionCall("database/sql", "Register"), Advice: []advice.Advice{ - advice.ReplaceFunction("gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql", "Register"), + advice.WrapExpression(code.MustTemplate( + "func(driverName string, driver driver.Driver) {\n sql.Register(driverName, driver)\n sqltrace.Register(driverName, driver)\n}({{ index .AST.Args 0 }}, {{ index .AST.Args 1 }})", + map[string]string{ + "sql": "database/sql", + "sqltrace": "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql", + }, + context.GoLangVersion{}, + )), }, }, { @@ -1874,4 +1881,4 @@ var InjectedPaths = [...]string{ } // Checksum is a checksum of the built-in configuration which can be used to invalidate caches. -const Checksum = "sha512:xmZzyYLOd/Rd6xiw6DaourRfLxpq0vKm9vnwvz8ENJhfN/wlYdnLA0ninNmp6UO8i8pbjrPV/l/mc2ZoZaYXpQ==" +const Checksum = "sha512:3RSiSosWn/St/Ene1Cb9HLEnIv8cxPX+xLanzGNQUX7nzp+R96xR/nruKKqodoLhNgkSWk2oi3htkwMkw5WPSA==" diff --git a/internal/injector/builtin/yaml/stdlib/database-sql.yml b/internal/injector/builtin/yaml/stdlib/database-sql.yml index 27ecf5cb..86ce2402 100644 --- a/internal/injector/builtin/yaml/stdlib/database-sql.yml +++ b/internal/injector/builtin/yaml/stdlib/database-sql.yml @@ -14,7 +14,15 @@ aspects: join-point: function-call: database/sql.Register advice: - - replace-function: gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql.Register + - wrap-expression: + imports: + sqltrace: gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql + sql: database/sql + template: |- + func(driverName string, driver driver.Driver) { + sql.Register(driverName, driver) + sqltrace.Register(driverName, driver) + }({{ index .AST.Args 0 }}, {{ index .AST.Args 1 }}) - id: Open join-point: From 89f2034e1c5de32d03d30adb8bd691d04e01d227 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Fri, 22 Nov 2024 10:37:33 +0100 Subject: [PATCH 2/4] Update test snapshot --- .../injector/builtin/testdata/server/database.go.snap | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/injector/builtin/testdata/server/database.go.snap b/internal/injector/builtin/testdata/server/database.go.snap index 5ec346ed..c9fae181 100644 --- a/internal/injector/builtin/testdata/server/database.go.snap +++ b/internal/injector/builtin/testdata/server/database.go.snap @@ -18,7 +18,13 @@ import ( //line samples/server/database.go:16 func init() { - __orchestrion_sql.Register("test", &testDriver{}) +//line + func(driverName string, driver driver.Driver) { + sql.Register(driverName, driver) + __orchestrion_sql.Register(driverName, driver) + }( +//line samples/server/database.go:17 + "test", &testDriver{}) } type testDriver struct{} From 362dce371b4f64f75e25435d33ea402f4ce5fead Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Fri, 22 Nov 2024 11:30:56 +0100 Subject: [PATCH 3/4] Retain the setup connection to ensure shared cache does not get busted --- _integration-tests/tests/sql/sql.go | 33 +++++++++++++++++++---------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/_integration-tests/tests/sql/sql.go b/_integration-tests/tests/sql/sql.go index b2e8fcaf..de4c40d7 100644 --- a/_integration-tests/tests/sql/sql.go +++ b/_integration-tests/tests/sql/sql.go @@ -20,27 +20,39 @@ import ( type TestCase struct { *sql.DB + // untraced is the un-traced database connection, used to set up the test case + // without producing unnecessary spans. It is retained as a [TestCase] field + // and cleaned up using [t.Cleanup] to ensure the shared cache is not lost, as + // it stops existing once the last DB connection using it is closed. + untraced *sql.DB } func (tc *TestCase) Setup(t *testing.T) { + const ( + dn = "sqlite3" + dsn = "file::memory:?cache=shared" + ) + + var err error + //orchestrion:ignore - prepareConn, err := sql.Open("sqlite3", "file::memory:?cache=shared") + tc.untraced, err = sql.Open(dn, dsn) require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, tc.untraced.Close()) }) - func() { - defer prepareConn.Close() + ctx := context.Background() - _, err = prepareConn.ExecContext(context.Background(), - `CREATE TABLE IF NOT EXISTS notes ( + _, err = tc.untraced.ExecContext(ctx, + `CREATE TABLE IF NOT EXISTS notes ( id INTEGER PRIMARY KEY AUTOINCREMENT, userid INTEGER, content STRING, created STRING )`) - require.NoError(t, err) + require.NoError(t, err) - _, err = prepareConn.ExecContext(context.Background(), - `INSERT OR REPLACE INTO notes(userid, content, created) VALUES + _, err = tc.untraced.ExecContext(ctx, + `INSERT OR REPLACE INTO notes(userid, content, created) VALUES (1, 'Hello, John. This is John. You are leaving a note for yourself. You are welcome and thank you.', datetime('now')), (1, 'Hey, remember to mow the lawn.', datetime('now')), (2, 'Reminder to submit that report by Thursday.', datetime('now')), @@ -48,10 +60,9 @@ func (tc *TestCase) Setup(t *testing.T) { (3, 'Pick up cabbage from the store on the way home.', datetime('now')), (3, 'Review PR #1138', datetime('now') )`) - require.NoError(t, err) - }() + require.NoError(t, err) - tc.DB, err = sql.Open("sqlite3", "file::memory:?cache=shared") + tc.DB, err = sql.Open(dn, dsn) require.NoError(t, err) t.Cleanup(func() { assert.NoError(t, tc.DB.Close()) From 43af6e85a80dcf4ec8abd80a812dbc31894f8c92 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Fri, 22 Nov 2024 11:32:16 +0100 Subject: [PATCH 4/4] Revert unintentional indent changes --- _integration-tests/tests/sql/sql.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/_integration-tests/tests/sql/sql.go b/_integration-tests/tests/sql/sql.go index de4c40d7..ca044691 100644 --- a/_integration-tests/tests/sql/sql.go +++ b/_integration-tests/tests/sql/sql.go @@ -44,22 +44,22 @@ func (tc *TestCase) Setup(t *testing.T) { _, err = tc.untraced.ExecContext(ctx, `CREATE TABLE IF NOT EXISTS notes ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - userid INTEGER, - content STRING, - created STRING - )`) + id INTEGER PRIMARY KEY AUTOINCREMENT, + userid INTEGER, + content STRING, + created STRING + )`) require.NoError(t, err) _, err = tc.untraced.ExecContext(ctx, `INSERT OR REPLACE INTO notes(userid, content, created) VALUES - (1, 'Hello, John. This is John. You are leaving a note for yourself. You are welcome and thank you.', datetime('now')), - (1, 'Hey, remember to mow the lawn.', datetime('now')), - (2, 'Reminder to submit that report by Thursday.', datetime('now')), - (2, 'Opportunities don''t happen, you create them.', datetime('now')), - (3, 'Pick up cabbage from the store on the way home.', datetime('now')), - (3, 'Review PR #1138', datetime('now') - )`) + (1, 'Hello, John. This is John. You are leaving a note for yourself. You are welcome and thank you.', datetime('now')), + (1, 'Hey, remember to mow the lawn.', datetime('now')), + (2, 'Reminder to submit that report by Thursday.', datetime('now')), + (2, 'Opportunities don''t happen, you create them.', datetime('now')), + (3, 'Pick up cabbage from the store on the way home.', datetime('now')), + (3, 'Review PR #1138', datetime('now') + )`) require.NoError(t, err) tc.DB, err = sql.Open(dn, dsn)