Skip to content
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.

Sign up for GitHub

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

Jump to bottom

contrib/database/sql: trace when new connection is established #794

Closed
wants to merge 3 commits into from
Closed

contrib/database/sql: trace when new connection is established #794

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fc5290
contrib/database/sql: trace when calling Connect
Dec 18, 2020
caf0d63
Merge branch 'v1' into feature/contrib-sql-conn-trace
ajgajg1134 Jan 31, 2022
d296a64
contrib/database/sql: add connectCancelledCtx test
ajgajg1134 Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion contrib/database/sql/conn.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down
13 changes: 8 additions & 5 deletions contrib/database/sql/sql.go
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ import (
"errors"
"math"
"reflect"
"time"

"gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql/internal"
)
Expand Down Expand Up @@ -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,
Expand All @@ -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
}

Expand Down
20 changes: 20 additions & 0 deletions contrib/internal/sqltest/sqltest.go
View file
Open in desktop
Expand Up @@ -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,
Expand All @@ -60,6 +62,24 @@ func RunAll(t *testing.T, cfg *Config) {
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@CAFxX @knusbaum the test that you're asking for would look more or less like this:

Suggested change
func testConnectCancelledCtx(cfg *Config) func(*testing.T) {
return func(t *testing.T) {
cfg.mockTracer.Reset()
assert := assert.New(t)
ctx, cancel := context.WithCancel(context.Background())
cancel()
rs := reflect.ValueOf(cfg.DB).Elem()
rf := rs.FieldByName("connector")
rf = reflect.NewAt(rf.Type(), unsafe.Pointer(rf.UnsafeAddr())).Elem()
_, err := rf.Interface().(driver.Connector).Connect(ctx)
assert.ErrorIs(err, context.Canceled)
spans := cfg.mockTracer.FinishedSpans()
_ = spans
// assert.Len(spans, 2)
// ...
}
}

You can't use cfg.DB.PingContext(ctx) because sql.DB.conn checks the context at the start (https://github.com/golang/go/blob/7eb31d999cf2769deb0e7bdcafc30e18f52ceb48/src/database/sql/sql.go#L1205) and tracedConnector::Connect isn't even called.

This test won't pass because

dd-trace-go/contrib/database/sql/sql.go

Line 141 in 9892197

func (t dsnConnector) Connect(_ context.Context) (driver.Conn, error) {
ignores the context.

@CAFxX @knusbaum Taking the complexity of the test in mind and the fact that fixing dsnConnector.Connect isn't the goal of this PR I'd suggest to allow to merge this PR.

To run the integration test deploy postgres like docker run -it --rm --name dd-tests-postgres -e POSTGRES_PASSWORD=postgres -e POSTGRES_USER=postgres -p 127.0.0.1:5432:5432/tcp postgres and set env variable INTEGRATION=true.

Copy link
Contributor

@CAFxX CAFxX Jan 14, 2021 •
edited

Choose a reason for hiding this comment

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

What I had in mind was more along the lines of:

  • write hangingConnector, a driver.Connector that, when e.g. PingContext is called, hangs until the context is cancelled
  • construct a tracedConnector wrapping hangingConnector
  • call PingContext on the tracedConnector in a goroutine
  • wait until we know we are blocked in hangingConnector
  • cancel the context
  • wait until the PingContext call returns
  • check that the span was emitted

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()
Expand Down