From 2fc529000e0ce642ce35291a3dfe962f28ce92ed Mon Sep 17 00:00:00 2001 From: stroem Date: Fri, 18 Dec 2020 08:08:03 +0100 Subject: [PATCH 1/6] contrib/database/sql: trace when calling Connect --- contrib/database/sql/conn.go | 3 ++- contrib/database/sql/sql.go | 13 ++++++++----- contrib/internal/sqltest/sqltest.go | 20 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/contrib/database/sql/conn.go b/contrib/database/sql/conn.go index 5198fb7903..dbcd537429 100644 --- a/contrib/database/sql/conn.go +++ b/contrib/database/sql/conn.go @@ -22,7 +22,8 @@ var _ driver.Conn = (*tracedConn)(nil) type queryType string const ( - queryTypeQuery queryType = "Query" + queryTypeConnect queryType = "Connect" + queryTypeQuery = "Query" queryTypePing = "Ping" queryTypePrepare = "Prepare" queryTypeExec = "Exec" diff --git a/contrib/database/sql/sql.go b/contrib/database/sql/sql.go index da54dae219..3e0e8298da 100644 --- a/contrib/database/sql/sql.go +++ b/contrib/database/sql/sql.go @@ -23,6 +23,7 @@ import ( "errors" "math" "reflect" + "time" "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql/internal" ) @@ -111,11 +112,7 @@ type tracedConnector struct { cfg *config } -func (t *tracedConnector) Connect(c context.Context) (driver.Conn, error) { - conn, err := t.connector.Connect(c) - if err != nil { - return nil, err - } +func (t *tracedConnector) Connect(ctx context.Context) (driver.Conn, error) { tp := &traceParams{ driverName: t.driverName, cfg: t.cfg, @@ -125,6 +122,12 @@ func (t *tracedConnector) Connect(c context.Context) (driver.Conn, error) { } else if t.cfg.dsn != "" { tp.meta, _ = internal.ParseDSN(t.driverName, t.cfg.dsn) } + start := time.Now() + conn, err := t.connector.Connect(ctx) + tp.tryTrace(ctx, queryTypeConnect, "", start, err) + if err != nil { + return nil, err + } return &tracedConn{conn, tp}, err } diff --git a/contrib/internal/sqltest/sqltest.go b/contrib/internal/sqltest/sqltest.go index 162ef149ca..d186cac8e1 100644 --- a/contrib/internal/sqltest/sqltest.go +++ b/contrib/internal/sqltest/sqltest.go @@ -49,6 +49,8 @@ func RunAll(t *testing.T, cfg *Config) { cfg.mockTracer = mocktracer.Start() defer cfg.mockTracer.Stop() + // Make sure testConnect runs first to ensure a connection is established + t.Run("Connect", testConnect(cfg)) for name, test := range map[string]func(*Config) func(*testing.T){ "Ping": testPing, "Query": testQuery, @@ -60,6 +62,24 @@ func RunAll(t *testing.T, cfg *Config) { } } +func testConnect(cfg *Config) func(*testing.T) { + return func(t *testing.T) { + cfg.mockTracer.Reset() + assert := assert.New(t) + err := cfg.DB.Ping() + assert.Nil(err) + spans := cfg.mockTracer.FinishedSpans() + assert.Len(spans, 2) + + span := spans[0] + assert.Equal(cfg.ExpectName, span.OperationName()) + cfg.ExpectTags["sql.query_type"] = "Connect" + for k, v := range cfg.ExpectTags { + assert.Equal(v, span.Tag(k), "Value mismatch on tag %s", k) + } + } +} + func testPing(cfg *Config) func(*testing.T) { return func(t *testing.T) { cfg.mockTracer.Reset() From d296a645a0d52ae77fe2de5c687410900f5dea8c Mon Sep 17 00:00:00 2001 From: "andrew.glaude" Date: Mon, 31 Jan 2022 15:33:43 -0500 Subject: [PATCH 2/6] contrib/database/sql: add connectCancelledCtx test --- contrib/database/sql/sql.go | 4 +-- contrib/database/sql/sql_test.go | 58 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/contrib/database/sql/sql.go b/contrib/database/sql/sql.go index 8a85fc7ba3..dac44a143e 100644 --- a/contrib/database/sql/sql.go +++ b/contrib/database/sql/sql.go @@ -151,7 +151,7 @@ func (t dsnConnector) Driver() driver.Driver { return t.driver } -// OpenDB returns connection to a DB using a the traced version of the given driver. In order for OpenDB +// OpenDB returns connection to a DB using the traced version of the given driver. In order for OpenDB // to work, the driver must first be registered using Register. If this did not occur, OpenDB will panic. func OpenDB(c driver.Connector, opts ...Option) *sql.DB { name, ok := registeredDrivers.name(c.Driver()) @@ -179,7 +179,7 @@ func OpenDB(c driver.Connector, opts ...Option) *sql.DB { return sql.OpenDB(tc) } -// Open returns connection to a DB using a the traced version of the given driver. In order for Open +// Open returns connection to a DB using the traced version of the given driver. In order for Open // to work, the driver must first be registered using Register. If this did not occur, Open will // return an error. func Open(driverName, dataSourceName string, opts ...Option) (*sql.DB, error) { diff --git a/contrib/database/sql/sql_test.go b/contrib/database/sql/sql_test.go index d19f92f7bc..2a3859b746 100644 --- a/contrib/database/sql/sql_test.go +++ b/contrib/database/sql/sql_test.go @@ -6,11 +6,16 @@ package sql import ( + "context" + "database/sql/driver" + "errors" "fmt" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "log" "math" "os" "testing" + "time" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/sqltest" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" @@ -190,3 +195,56 @@ func TestMySQLUint64(t *testing.T) { assert.NoError(rows.Err()) assert.NoError(rows.Close()) } + +//hangingConnector hangs on Connect until ctx is cancelled. +type hangingConnector struct{} + +func (h *hangingConnector) Connect(ctx context.Context) (driver.Conn, error) { + select { + case <-ctx.Done(): + return &panicConn{}, errors.New("context cancelled") + } +} + +func (h *hangingConnector) Driver() driver.Driver { + panic("hangingConnector: Driver() not implemented") +} + +type panicConn struct{} + +func (p *panicConn) Prepare(query string) (driver.Stmt, error) { + panic("panicConn: Prepare called") +} + +func (p *panicConn) Close() error { + panic("panicConn: Close called") +} + +func (p *panicConn) Begin() (driver.Tx, error) { + panic("panicConn: Begin called") +} + +func TestConnectCancelledCtx(t *testing.T) { + mockTracer := mocktracer.Start() + defer mockTracer.Stop() + assert := assert.New(t) + tc := tracedConnector{ + connector: &hangingConnector{}, + driverName: "hangingConnector", + cfg: new(config), + } + ctx, cancelFunc := context.WithCancel(context.Background()) + + go func() { + tc.Connect(ctx) + }() + time.Sleep(time.Millisecond * 100) + cancelFunc() + time.Sleep(time.Millisecond * 100) + + spans := mockTracer.FinishedSpans() + assert.Len(spans, 1) + s := spans[0] + assert.Equal("hangingConnector.query", s.OperationName()) + assert.Equal("Connect", s.Tag("sql.query_type")) +} From 5f5205c043b63f53d2df7a62bdf310a912974445 Mon Sep 17 00:00:00 2001 From: "andrew.glaude" Date: Tue, 1 Feb 2022 09:35:57 -0500 Subject: [PATCH 3/6] contrib/database/sql: prevent idle connections in tests to guarantee reconnection --- contrib/database/sql/conn_test.go | 4 ++-- contrib/database/sql/sql_test.go | 7 ++++++- contrib/gorm.io/gorm.v1/gorm_test.go | 2 ++ contrib/internal/sqltest/sqltest.go | 22 +++++++++++----------- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/contrib/database/sql/conn_test.go b/contrib/database/sql/conn_test.go index d08aa45d4e..92d2b4f772 100644 --- a/contrib/database/sql/conn_test.go +++ b/contrib/database/sql/conn_test.go @@ -92,9 +92,9 @@ func TestWithSpanTags(t *testing.T) { rows.Close() spans := mt.FinishedSpans() - assert.Len(t, spans, 1) + assert.Len(t, spans, 2) - span := spans[0] + span := spans[1] assert.Equal(t, tt.want.opName, span.OperationName()) for k, v := range tt.want.ctxTags { assert.Equal(t, v, span.Tag(k), "Value mismatch on tag %s", k) diff --git a/contrib/database/sql/sql_test.go b/contrib/database/sql/sql_test.go index 2a3859b746..c55faa2bfe 100644 --- a/contrib/database/sql/sql_test.go +++ b/contrib/database/sql/sql_test.go @@ -10,7 +10,6 @@ import ( "database/sql/driver" "errors" "fmt" - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "log" "math" "os" @@ -19,6 +18,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/sqltest" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "github.com/go-sql-driver/mysql" "github.com/lib/pq" @@ -45,6 +45,7 @@ func TestMySQL(t *testing.T) { log.Fatal(err) } defer db.Close() + db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -71,6 +72,7 @@ func TestPostgres(t *testing.T) { log.Fatal(err) } defer db.Close() + db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -102,6 +104,7 @@ func TestOpenOptions(t *testing.T) { log.Fatal(err) } defer db.Close() + db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -128,6 +131,7 @@ func TestOpenOptions(t *testing.T) { } db := OpenDB(c) defer db.Close() + db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -155,6 +159,7 @@ func TestOpenOptions(t *testing.T) { } db := OpenDB(c, WithDSN(dsn)) defer db.Close() + db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, diff --git a/contrib/gorm.io/gorm.v1/gorm_test.go b/contrib/gorm.io/gorm.v1/gorm_test.go index 9d4d1fec19..b489ef662c 100644 --- a/contrib/gorm.io/gorm.v1/gorm_test.go +++ b/contrib/gorm.io/gorm.v1/gorm_test.go @@ -61,6 +61,7 @@ func TestMySQL(t *testing.T) { if err != nil { log.Fatal(err) } + internalDB.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: internalDB, @@ -95,6 +96,7 @@ func TestPostgres(t *testing.T) { if err != nil { log.Fatal(err) } + internalDB.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: internalDB, diff --git a/contrib/internal/sqltest/sqltest.go b/contrib/internal/sqltest/sqltest.go index 4aa1758d0c..f5370b2b3c 100644 --- a/contrib/internal/sqltest/sqltest.go +++ b/contrib/internal/sqltest/sqltest.go @@ -87,9 +87,9 @@ func testPing(cfg *Config) func(*testing.T) { err := cfg.DB.Ping() assert.Nil(err) spans := cfg.mockTracer.FinishedSpans() - assert.Len(spans, 1) + assert.Len(spans, 2) - span := spans[0] + span := spans[1] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Ping" for k, v := range cfg.ExpectTags { @@ -108,9 +108,9 @@ func testQuery(cfg *Config) func(*testing.T) { assert.Nil(err) spans := cfg.mockTracer.FinishedSpans() - assert.Len(spans, 1) + assert.Len(spans, 2) - span := spans[0] + span := spans[1] cfg.ExpectTags["sql.query_type"] = "Query" assert.Equal(cfg.ExpectName, span.OperationName()) for k, v := range cfg.ExpectTags { @@ -134,9 +134,9 @@ func testStatement(cfg *Config) func(*testing.T) { assert.Equal(nil, err) spans := cfg.mockTracer.FinishedSpans() - assert.Len(spans, 1) + assert.Len(spans, 3) - span := spans[0] + span := spans[1] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Prepare" for k, v := range cfg.ExpectTags { @@ -148,8 +148,8 @@ func testStatement(cfg *Config) func(*testing.T) { assert.Equal(nil, err2) spans = cfg.mockTracer.FinishedSpans() - assert.Len(spans, 1) - span = spans[0] + assert.Len(spans, 4) + span = spans[2] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Exec" for k, v := range cfg.ExpectTags { @@ -167,9 +167,9 @@ func testBeginRollback(cfg *Config) func(*testing.T) { assert.Equal(nil, err) spans := cfg.mockTracer.FinishedSpans() - assert.Len(spans, 1) + assert.Len(spans, 2) - span := spans[0] + span := spans[1] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Begin" for k, v := range cfg.ExpectTags { @@ -212,7 +212,7 @@ func testExec(cfg *Config) func(*testing.T) { parent.Finish() // flush children spans := cfg.mockTracer.FinishedSpans() - assert.Len(spans, 4) + assert.Len(spans, 5) var span mocktracer.Span for _, s := range spans { From 7dc5cec3d929a2449d6e5318d0b067a29b226f31 Mon Sep 17 00:00:00 2001 From: "andrew.glaude" Date: Tue, 1 Feb 2022 10:13:31 -0500 Subject: [PATCH 4/6] contrib/database/sql: move idleConn setting to shared space to cover additional sql libs --- contrib/database/sql/sql_test.go | 23 ++--------------------- contrib/gorm.io/gorm.v1/gorm_test.go | 2 -- contrib/internal/sqltest/sqltest.go | 1 + 3 files changed, 3 insertions(+), 23 deletions(-) diff --git a/contrib/database/sql/sql_test.go b/contrib/database/sql/sql_test.go index c55faa2bfe..b953a9c04f 100644 --- a/contrib/database/sql/sql_test.go +++ b/contrib/database/sql/sql_test.go @@ -45,7 +45,6 @@ func TestMySQL(t *testing.T) { log.Fatal(err) } defer db.Close() - db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -72,7 +71,6 @@ func TestPostgres(t *testing.T) { log.Fatal(err) } defer db.Close() - db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -104,7 +102,6 @@ func TestOpenOptions(t *testing.T) { log.Fatal(err) } defer db.Close() - db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -131,7 +128,6 @@ func TestOpenOptions(t *testing.T) { } db := OpenDB(c) defer db.Close() - db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -159,7 +155,6 @@ func TestOpenOptions(t *testing.T) { } db := OpenDB(c, WithDSN(dsn)) defer db.Close() - db.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: db, @@ -201,13 +196,13 @@ func TestMySQLUint64(t *testing.T) { assert.NoError(rows.Close()) } -//hangingConnector hangs on Connect until ctx is cancelled. +// hangingConnector hangs on Connect until ctx is cancelled. type hangingConnector struct{} func (h *hangingConnector) Connect(ctx context.Context) (driver.Conn, error) { select { case <-ctx.Done(): - return &panicConn{}, errors.New("context cancelled") + return nil, errors.New("context cancelled") } } @@ -215,20 +210,6 @@ func (h *hangingConnector) Driver() driver.Driver { panic("hangingConnector: Driver() not implemented") } -type panicConn struct{} - -func (p *panicConn) Prepare(query string) (driver.Stmt, error) { - panic("panicConn: Prepare called") -} - -func (p *panicConn) Close() error { - panic("panicConn: Close called") -} - -func (p *panicConn) Begin() (driver.Tx, error) { - panic("panicConn: Begin called") -} - func TestConnectCancelledCtx(t *testing.T) { mockTracer := mocktracer.Start() defer mockTracer.Stop() diff --git a/contrib/gorm.io/gorm.v1/gorm_test.go b/contrib/gorm.io/gorm.v1/gorm_test.go index b489ef662c..9d4d1fec19 100644 --- a/contrib/gorm.io/gorm.v1/gorm_test.go +++ b/contrib/gorm.io/gorm.v1/gorm_test.go @@ -61,7 +61,6 @@ func TestMySQL(t *testing.T) { if err != nil { log.Fatal(err) } - internalDB.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: internalDB, @@ -96,7 +95,6 @@ func TestPostgres(t *testing.T) { if err != nil { log.Fatal(err) } - internalDB.SetMaxIdleConns(0) testConfig := &sqltest.Config{ DB: internalDB, diff --git a/contrib/internal/sqltest/sqltest.go b/contrib/internal/sqltest/sqltest.go index f5370b2b3c..d77753d014 100644 --- a/contrib/internal/sqltest/sqltest.go +++ b/contrib/internal/sqltest/sqltest.go @@ -48,6 +48,7 @@ func Prepare(tableName string) func() { func RunAll(t *testing.T, cfg *Config) { cfg.mockTracer = mocktracer.Start() defer cfg.mockTracer.Stop() + cfg.DB.SetMaxIdleConns(0) // Make sure testConnect runs first to ensure a connection is established t.Run("Connect", testConnect(cfg)) From f9ea5e8621859156096ebf76e347c12f4ea84bda Mon Sep 17 00:00:00 2001 From: "andrew.glaude" Date: Tue, 1 Feb 2022 10:15:59 -0500 Subject: [PATCH 5/6] contrib/database/sql: connect test now does not need to be run first --- contrib/internal/sqltest/sqltest.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contrib/internal/sqltest/sqltest.go b/contrib/internal/sqltest/sqltest.go index d77753d014..4c268500a2 100644 --- a/contrib/internal/sqltest/sqltest.go +++ b/contrib/internal/sqltest/sqltest.go @@ -50,9 +50,8 @@ func RunAll(t *testing.T, cfg *Config) { defer cfg.mockTracer.Stop() cfg.DB.SetMaxIdleConns(0) - // Make sure testConnect runs first to ensure a connection is established - t.Run("Connect", testConnect(cfg)) for name, test := range map[string]func(*Config) func(*testing.T){ + "Connect": testConnect, "Ping": testPing, "Query": testQuery, "Statement": testStatement, From 4edd78ec11249f6c4a98944692487d5e9689d275 Mon Sep 17 00:00:00 2001 From: "andrew.glaude" Date: Thu, 3 Feb 2022 13:30:42 -0500 Subject: [PATCH 6/6] contrib/database/sql: check for connect span in other sql tests --- contrib/database/sql/conn_test.go | 7 +++++++ contrib/internal/sqltest/sqltest.go | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/contrib/database/sql/conn_test.go b/contrib/database/sql/conn_test.go index 92d2b4f772..3203bf0e27 100644 --- a/contrib/database/sql/conn_test.go +++ b/contrib/database/sql/conn_test.go @@ -94,6 +94,13 @@ func TestWithSpanTags(t *testing.T) { spans := mt.FinishedSpans() assert.Len(t, spans, 2) + connectSpan := spans[0] + assert.Equal(t, tt.want.opName, connectSpan.OperationName()) + assert.Equal(t, "Connect", connectSpan.Tag("sql.query_type")) + for k, v := range tt.want.ctxTags { + assert.Equal(t, v, connectSpan.Tag(k), "Value mismatch on tag %s", k) + } + span := spans[1] assert.Equal(t, tt.want.opName, span.OperationName()) for k, v := range tt.want.ctxTags { diff --git a/contrib/internal/sqltest/sqltest.go b/contrib/internal/sqltest/sqltest.go index 4c268500a2..0931ab5308 100644 --- a/contrib/internal/sqltest/sqltest.go +++ b/contrib/internal/sqltest/sqltest.go @@ -89,6 +89,8 @@ func testPing(cfg *Config) func(*testing.T) { spans := cfg.mockTracer.FinishedSpans() assert.Len(spans, 2) + verifyConnectSpan(spans[0], assert, cfg) + span := spans[1] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Ping" @@ -110,6 +112,8 @@ func testQuery(cfg *Config) func(*testing.T) { spans := cfg.mockTracer.FinishedSpans() assert.Len(spans, 2) + verifyConnectSpan(spans[0], assert, cfg) + span := spans[1] cfg.ExpectTags["sql.query_type"] = "Query" assert.Equal(cfg.ExpectName, span.OperationName()) @@ -136,6 +140,8 @@ func testStatement(cfg *Config) func(*testing.T) { spans := cfg.mockTracer.FinishedSpans() assert.Len(spans, 3) + verifyConnectSpan(spans[0], assert, cfg) + span := spans[1] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Prepare" @@ -169,6 +175,8 @@ func testBeginRollback(cfg *Config) func(*testing.T) { spans := cfg.mockTracer.FinishedSpans() assert.Len(spans, 2) + verifyConnectSpan(spans[0], assert, cfg) + span := spans[1] assert.Equal(cfg.ExpectName, span.OperationName()) cfg.ExpectTags["sql.query_type"] = "Begin" @@ -238,6 +246,14 @@ func testExec(cfg *Config) func(*testing.T) { } } +func verifyConnectSpan(span mocktracer.Span, assert *assert.Assertions, cfg *Config) { + assert.Equal(cfg.ExpectName, span.OperationName()) + cfg.ExpectTags["sql.query_type"] = "Connect" + for k, v := range cfg.ExpectTags { + assert.Equal(v, span.Tag(k), "Value mismatch on tag %s", k) + } +} + // Config holds the test configuration. type Config struct { *sql.DB