module/apmsql: don't report driver.ErrBadConn

database/sql drivers may return driver.ErrBadConn
to indicate that the connection is bad, and should
not be reused. We should not report these errors,
as they are expected by database/sql, and used for
connection pooling/reuse.

CHANGELOG.md

@@ -9,6 +9,7 @@
  - Added distributed trace context propagation to apmgrpc (#335)
  - Introduce `Span.Subtype`, `Span.Action` (#332)
  - apm.StartSpanOptions fixed to stop ignoring options (#326)
+ - module/apmsql: don't report driver.ErrBadConn (#346)
 
 ## [v1.0.0](https://github.com/elastic/apm-agent-go/releases/tag/v1.0.0)
 

module/apmsql/apmsql_test.go

@@ -3,8 +3,10 @@ package apmsql_test
 import (
 	"context"
 	"database/sql"
+	"database/sql/driver"
 	"testing"
 
+	sqlite3 "github.com/mattn/go-sqlite3"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -14,6 +16,10 @@ import (
 	_ "go.elastic.co/apm/module/apmsql/sqlite3"
 )
 
+func init() {
+	apmsql.Register("sqlite3_badconn", &sqlite3BadConnDriver{})
+}
+
 func TestPingContext(t *testing.T) {
 	db, err := apmsql.Open("sqlite3", ":memory:")
 	require.NoError(t, err)
@@ -56,12 +62,13 @@ func TestQueryContext(t *testing.T) {
 	_, err = db.Exec("CREATE TABLE foo (bar INT)")
 	require.NoError(t, err)
 
-	_, spans, _ := apmtest.WithTransaction(func(ctx context.Context) {
+	_, spans, errors := apmtest.WithTransaction(func(ctx context.Context) {
 		rows, err := db.QueryContext(ctx, "SELECT * FROM foo")
 		require.NoError(t, err)
 		rows.Close()
 	})
 	require.Len(t, spans, 1)
+	assert.Empty(t, errors)
 
 	assert.NotNil(t, spans[0].ID)
 	assert.Equal(t, "SELECT FROM foo", spans[0].Name)
@@ -191,3 +198,48 @@ func TestCaptureErrors(t *testing.T) {
 	assert.Equal(t, "query", spans[0].Action)
 	assert.Equal(t, "no such table: thin_air", errors[0].Exception.Message)
 }
+
+func TestBadConn(t *testing.T) {
+	db, err := apmsql.Open("sqlite3_badconn", ":memory:")
+	require.NoError(t, err)
+	defer db.Close()
+
+	db.Ping() // connect
+	_, spans, errors := apmtest.WithTransaction(func(ctx context.Context) {
+		_, err := db.QueryContext(ctx, "SELECT * FROM foo")
+		require.Error(t, err)
+	})
+
+	var attempts int
+	for _, span := range spans {
+		if span.Name == "SELECT FROM foo" {
+			attempts++
+		}
+	}
+	// Two attempts with cached-or-new, followed
+	// by one attempt with a new connection.
+	assert.Condition(t, func() bool { return attempts == 3 })
+	assert.Len(t, errors, 0) // no "bad connection" errors reported
+}
+
+type sqlite3BadConnDriver struct {
+	sqlite3.SQLiteDriver
+}
+
+func (d *sqlite3BadConnDriver) Open(name string) (driver.Conn, error) {
+	conn, err := d.SQLiteDriver.Open(name)
+	if err != nil {
+		return conn, err
+	}
+	return &sqlite3BadConn{
+		SQLiteConn: conn.(*sqlite3.SQLiteConn),
+	}, nil
+}
+
+type sqlite3BadConn struct {
+	*sqlite3.SQLiteConn
+}
+
+func (d *sqlite3BadConn) QueryContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Rows, error) {
+	return nil, driver.ErrBadConn
+}

module/apmsql/conn.go

@@ -68,8 +68,15 @@ func (c *conn) finishSpan(ctx context.Context, span *apm.Span, resultError *erro
 		// in check.
 		return
 	}
-	if e := apm.CaptureError(ctx, *resultError); e != nil {
-		e.Send()
+	switch *resultError {
+	case nil, driver.ErrBadConn:
+		// ErrBadConn is used by the connection pooling
+		// logic in database/sql, and so is expected and
+		// should not be reported.
+	default:
+		if e := apm.CaptureError(ctx, *resultError); e != nil {
+			e.Send()
+		}
 	}
 	span.End()
 }