feat: database/sql integration #893

aldy505 · 2024-10-18T10:21:52Z

Queries insights tracing for Go.

closes #1128
closes GO-95

sentrysql/conn.go

sentrysql/stmt.go

codecov · 2024-10-19T02:29:18Z

Codecov Report

❌ Patch coverage is 83.75000% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.92%. Comparing base (c243873) to head (3695056).

Files with missing lines	Patch %	Lines
sentrysql/conn.go	82.66%	20 Missing and 6 partials ⚠️
sentrysql/stmt.go	85.86%	8 Missing and 5 partials ⚠️
sentrysql/driver.go	65.62%	10 Missing and 1 partial ⚠️
sentrysql/sentrysql.go	92.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
- Coverage   86.07%   85.92%   -0.15%     
==========================================
  Files          62       69       +7     
  Lines        6090     6410     +320     
==========================================
+ Hits         5242     5508     +266     
- Misses        634      674      +40     
- Partials      214      228      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aldy505 · 2024-10-19T02:29:52Z

I really don't like golangci-lint, it's so pedantic :(

aldy505 · 2024-10-19T05:20:24Z

Should be good by now.

sentrysql/operation.go

sentrysql/operation_test.go

ribice · 2024-10-22T10:55:42Z

I'll test this locally myself this week and report back. We should also update the docs and main repository with this integration.

ribice · 2024-10-25T11:48:33Z

You're missing examples in _examples. These should also include example on how to set the DSN.

aldy505 · 2024-10-25T12:25:30Z

You're missing examples in _examples. These should also include example on how to set the DSN.

@ribice I've been occupied so much with work this week. Will find the time to work on this later.

giortzisg · 2025-05-14T07:46:32Z

sentrysql/sentrysql_connector_test.go

+			// if WantSpan is nil, yet we got some spans, it should be an error
+			if tt.WantSpan == nil {
+				t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans)
+				continue
+			}
+
+			// if WantSpan is not nil, we should have at least one span
+			if len(gotSpans) == 0 {
+				t.Errorf("Expecting at least one span, but got %d spans: %v", len(gotSpans), gotSpans)
+				continue
+			}
+


Can we not have tt.WantSpans as a []Span so that we can compare immediately with gotSpans? I'd say that it's not very readable.

Did some rounds with this, I can't get []Span comparison working (problems with google/go-cmp package). Is it okay if we backlog this somehow?

giortzisg · 2025-05-14T07:48:12Z

sentrysql/sentrysql_test.go

+func TestMain(m *testing.M) {
+	sql.Register("sentrysql-sqlite", sentrysql.NewSentrySQL(&sqlite.Driver{}, sentrysql.WithDatabaseName("memory"), sentrysql.WithDatabaseSystem(sentrysql.DatabaseSystem("sqlite")), sentrysql.WithServerAddress("localhost", "5432")))
+	// sentrysql-legacy is used by `sentrysql_legacy_test.go`
+	sql.Register("sentrysql-legacy", sentrysql.NewSentrySQL(ldriver, sentrysql.WithDatabaseSystem(sentrysql.DatabaseSystem("legacydb")), sentrysql.WithDatabaseName("fake")))
+
+	os.Exit(m.Run())
+}


This is obfuscated from the other tests. Should probably be an init statement on each file that registers whatever we need only for the tests on the file.

From the docs: If Register is called twice with the same name or if driver is nil, it panics.

I believe setting this here is the "right" way to do so, since it'll be run after any library's init invocation.

giortzisg · 2025-05-14T07:56:11Z

sentrysql/sentrysql_test.go

+			var foundMatch = false
+			gotSpans := got[i]
+
+			var diffs []string
+			for _, gotSpan := range gotSpans {
+				if diff := cmp.Diff(tt.WantSpan, gotSpan, optstrans); diff != "" {
+					diffs = append(diffs, diff)
+				} else {
+					foundMatch = true
+					break
+				}
+			}
+
+			if !foundMatch {
+				t.Errorf("Span mismatch (-want +got):\n%s", strings.Join(diffs, "\n"))
+			}


Same thing applies here. We should probably use a []Span for WantSpans

Copilot

Pull Request Overview

Adds Sentry tracing integration to Go’s database/sql by wrapping drivers, connectors, connections, statements, and transactions to automatically start spans for SQL operations.

Introduce sentrySQLConfig and Option functions to attach metadata (db system, name, host, port).
Wrap driver.Driver, driver.Connector, driver.Conn, and driver.Stmt to emit spans on Exec/Query (including context versions).
Add parseDatabaseOperation helper to extract SQL operation names and tests for it.

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
sentrysql/tx.go	Added `sentryTx` wrapper for `driver.Tx` (currently no spans).
sentrysql/stmt.go	Implemented `sentryStmt` with Exec/Query and ExecContext/QueryContext tracing.
sentrysql/sentrysql.go	Defined `DatabaseSystem`, config struct, and constructors (`NewSentrySQL`, `NewSentrySQLConnector`).
sentrysql/options.go	Added `Option` functions to set database metadata.
sentrysql/operation.go	Added `parseDatabaseOperation` to derive operation names.
sentrysql/operation_test.go	Added tests for `parseDatabaseOperation`.
sentrysql/driver.go	Wrapped `driver.Driver` and `driver.Connector` for injection.
sentrysql/conn.go	Implemented `sentryConn` with span creation for Exec/Query/Ping/Tx.
sentrysql/go.mod	Initialized module and dependencies.
sentrysql/example_test.go	Added examples for `NewSentrySQL` and `NewSentrySQLConnector`.
_examples/sql/main.go	End-to-end example demonstrating tracing of queries and transactions.

Comments suppressed due to low confidence (1)

sentrysql/stmt.go:75

There are no unit tests covering ExecContext and QueryContext in sentryStmt. Adding tests would verify span creation and fallback behavior.

func (s *sentryStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (driver.Result, error)

Copilot · 2025-06-17T11:06:28Z

sentrysql/stmt.go

+		return s.Exec(values)
+	}
+
+	parentSpan := sentry.SpanFromContext(s.ctx)


In ExecContext, you’re calling SpanFromContext on s.ctx instead of the passed-in ctx. This can attach spans to a stale context. Use sentry.SpanFromContext(ctx) instead.

Suggested change

parentSpan := sentry.SpanFromContext(s.ctx)

parentSpan := sentry.SpanFromContext(ctx)

This is wrong. ctx provided from the function argument does not have sentry span context. SInce what matters is the context in which the statement was started.

Copilot · 2025-06-17T11:06:28Z

sentrysql/stmt.go

+		return s.Query(values)
+	}
+
+	parentSpan := sentry.SpanFromContext(s.ctx)


In QueryContext, you’re using s.ctx for the parent span lookup instead of the method’s ctx parameter. Update to sentry.SpanFromContext(ctx) to ensure the correct context is used.

Suggested change

parentSpan := sentry.SpanFromContext(s.ctx)

parentSpan := sentry.SpanFromContext(ctx)

This is wrong. ctx provided from the function argument does not have sentry span context. SInce what matters is the context in which the statement was started.

Copilot · 2025-06-17T11:06:29Z

sentrysql/tx.go

+type sentryTx struct {
+	originalTx driver.Tx
+	ctx        context.Context
+	config     *sentrySQLConfig


[nitpick] The config field in sentryTx is never used. Consider removing it or adding tracing for Commit/Rollback to leverage the config metadata.

Tracing on Commit / Rollback would be useless.

aldy505 · 2025-09-25T07:34:30Z

@cleptric @giortzisg any updates?

giortzisg · 2025-09-25T07:38:14Z

We're currently doing a big refactoring on the transport layer, will have a look after that.

cursor · 2025-10-23T01:50:16Z

sentrysql/conn.go

+		return nil, err
+	}
+
+	return &sentryTx{originalTx: tx, ctx: s.ctx, config: s.config}, nil


Bug: Context Propagation Issue in Sentry Wrappers

In sentryConn.BeginTx and sentryStmt's ExecContext and QueryContext methods, the wrapper incorrectly uses the stored s.ctx field instead of the method's ctx parameter when the underlying driver supports context. This can lead to stale context propagation and incorrect Sentry span parenting.

Additional Locations (2)

sentrysql/stmt.go#L88-L89

sentrysql/stmt.go#L122-L123

the comment is actually correct here, we should derive the ctx from the caller and if that is nil then fallback to the parent ctx.

cursor · 2025-10-23T01:50:16Z

sentrysql/stmt.go

+		return s.Query(values)
+	}
+
+	parentSpan := sentry.SpanFromContext(s.ctx)


Bug: Incorrect Span Parentage in Context-Aware Drivers

In both ExecContext and QueryContext, when the underlying driver supports context-aware interfaces, sentry.SpanFromContext uses s.ctx instead of the ctx parameter. Since s.ctx is intended for fallback scenarios, using it here can result in incorrect span parentage or missing spans in the tracing hierarchy.

Additional Locations (1)

sentrysql/stmt.go#L88-L89

giortzisg

We can keep the tests as is for now. Added a few comments, let's amend them and bump the go version and we should be good to merge this.

giortzisg · 2025-11-17T09:38:34Z

sentrysql/conn.go

+		return nil, err
+	}
+
+	return &sentryTx{originalTx: tx, ctx: s.ctx, config: s.config}, nil


the comment is actually correct here, we should derive the ctx from the caller and if that is nil then fallback to the parent ctx.

giortzisg · 2025-11-17T10:46:00Z

sentrysql/conn.go

+	s.config.SetData(span, query)
+	defer span.Finish()
+
+	rows, err := queryerContext.QueryContext(ctx, query, args)


We should propagate the span context here.

Suggested change

rows, err := queryerContext.QueryContext(ctx, query, args)

rows, err := queryerContext.QueryContext(span.Context(), query, args)

giortzisg · 2025-11-17T10:46:33Z

sentrysql/conn.go

+	s.config.SetData(span, query)
+	defer span.Finish()
+
+	rows, err := execerContext.ExecContext(ctx, query, args)


Suggested change

rows, err := execerContext.ExecContext(ctx, query, args)

rows, err := execerContext.ExecContext(span.Context(), query, args)

giortzisg · 2025-11-17T10:47:33Z

sentrysql/stmt.go

+	s.config.SetData(span, s.query)
+	defer span.Finish()
+
+	result, err := stmtExecContext.ExecContext(ctx, args)


Suggested change

result, err := stmtExecContext.ExecContext(ctx, args)

result, err := stmtExecContext.ExecContext(span.Context(), args)

sentrysql/stmt.go

sentrysql/conn.go

sentrysql/stmt.go

sentrysql/conn.go

sentrysql/stmt.go

cursor · 2025-11-19T13:26:03Z

sentrysql/stmt.go

+		s.ctx = ctx
+		return s.Query(values)
+	}
+


Bug: Wrong context used for span in statement query

In QueryContext, when the original statement implements StmtQueryContext, the code retrieves the parent span from s.ctx instead of the ctx parameter. This causes span tracking to use a stale context rather than the current one provided by the caller, potentially breaking the span hierarchy and context propagation for prepared statement queries.

sentrysql/stmt.go

sentrysql/conn.go

cursor · 2025-11-20T00:31:06Z

sentrysql/stmt.go

+			return nil, err
+		}
+
+		return s.Query(values)


Bug: Missing context assignment in QueryContext fallback path

In sentryStmt.QueryContext, when the underlying driver doesn't implement StmtQueryContext and falls back to the non-context Query method, the code fails to assign the context to s.ctx before calling s.Query(values). This causes the context and any associated span information to be lost. The ExecContext method correctly does s.ctx = ctx before calling s.Exec(values) at line 85, but QueryContext is missing this assignment at line 123, resulting in inconsistent behavior where query tracing won't work for legacy drivers.

sentrysql/conn.go

sentrysql/operation.go

sentrysql/stmt.go

_examples/sql/main.go

cursor · 2025-12-07T01:46:30Z

sentrysql/sentrysql_connector_test.go

+			if tt.WantSpan == nil {
+				t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans)
+				continue
+			}


Bug: Test falsely errors when WantSpan is nil

The test logic incorrectly reports an error when tt.WantSpan is nil. The comment states the intent is to error only when WantSpan is nil AND spans were received, but the condition only checks tt.WantSpan == nil without verifying len(gotSpans) > 0. This causes the test to always fail when no spans are expected, even if correctly zero spans were received.

cursor · 2025-12-07T01:46:30Z

_examples/sql/main.go

+		users = append(users, user)
+	}
+
+	return users, nil


Bug: Missing rows.Err() check after iteration loop

In the GetAllUsers function, after iterating over rows.Next(), there's no call to rows.Err() to check for errors that occurred during iteration. The rows.Next() method returns false both when iteration completes successfully and when an error occurs. Without checking rows.Err(), iteration errors could be silently ignored and the function would return incomplete results.

sentrysql/example_test.go

cursor · 2025-12-07T01:46:30Z

sentrysql/sentrysql_legacy_test.go

+			if tt.WantSpan == nil {
+				t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans)
+				continue
+			}


Bug: Same test bug pattern in legacy test file

The test logic at line 136 has the same bug as in sentrysql_connector_test.go. It always errors when tt.WantSpan == nil without checking if len(gotSpans) > 0. This causes false test failures when no spans are correctly expected and received. The same pattern appears at multiple locations in this file (lines 254, 439, 568).

See https://getsentry.github.io/sentry-conventions/generated/attributes/db.html

cursor · 2025-12-07T01:56:02Z

sentrysql/sentrysql_connector_test.go

+			if tt.WantSpan == nil {
+				t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans)
+				continue
+			}


Bug: Test incorrectly errors when expecting and receiving zero spans

The test validation logic is incorrect. The comment states "if WantSpan is nil, yet we got some spans, it should be an error", but the condition only checks if tt.WantSpan == nil without verifying that spans were actually received. When WantSpan is nil and correctly zero spans are received, the test incorrectly reports an error. The condition needs to be if tt.WantSpan == nil && len(gotSpans) > 0 to match the intended behavior described in the comment.

Additional Locations (1)

sentrysql/sentrysql_legacy_test.go#L135-L139

cursor · 2025-12-07T01:56:02Z

_examples/sql/main.go

+		users = append(users, user)
+	}
+
+	return users, nil


Bug: Example code missing rows.Err() check after iteration

The GetAllUsers function is missing a rows.Err() check after the iteration loop. Per Go's database/sql best practices, rows.Err() must be checked after the loop to catch any errors that may have occurred during iteration. As written, users copying this example code could silently miss database errors that occur while fetching rows.

aldy505 added 5 commits October 18, 2024 17:21

feat: initial sentrysql implementation

e544314

chore: resolve a few lint issues

92e3a6b

chore: another attempt at resolving lint issues

aa4acb2

chore: wrong method name

a6b5de8

chore: another attempt of fixing lint issues

c6ebbda

ribice reviewed Oct 18, 2024

View reviewed changes

sentrysql/conn.go Outdated Show resolved Hide resolved

ribice reviewed Oct 18, 2024

View reviewed changes

sentrysql/stmt.go Outdated Show resolved Hide resolved

ribice reviewed Oct 18, 2024

View reviewed changes

sentrysql/stmt.go Outdated Show resolved Hide resolved

ribice reviewed Oct 18, 2024

View reviewed changes

sentrysql/stmt.go Outdated Show resolved Hide resolved

aldy505 added 3 commits October 19, 2024 08:51

feat: implement missing bits from driver interfaces

63413f3

chore: missing a period on comment

b77e392

test: replace ramsql with in-memory sqlite

9b59328

aldy505 added 8 commits October 19, 2024 10:22

test: common queries

7ecd868

chore: avoid cyclo errors

19c97df

test: no parent span

d9073aa

test: db.Driver

05d699b

chore: trailing newline

0fc642e

test: using mysql server for connector

7b5fbb5

test: remove mysql server, stay on go1.18

93198bd

chore: another go1.18 rollback

ab1d3bf

aldy505 marked this pull request as ready for review October 19, 2024 05:20

aldy505 requested a review from ribice October 19, 2024 05:20

ribice reviewed Oct 20, 2024

View reviewed changes

sentrysql/operation.go Outdated Show resolved Hide resolved

ribice reviewed Oct 20, 2024

View reviewed changes

sentrysql/operation_test.go Show resolved Hide resolved

aldy505 requested a review from giortzisg May 7, 2025 03:40

giortzisg reviewed May 14, 2025

View reviewed changes

cleptric requested review from cleptric and Copilot June 17, 2025 11:03

Copilot AI reviewed Jun 17, 2025

View reviewed changes

Merge branch 'master' into feat/sentrysql

b399672

cursor bot reviewed Oct 23, 2025

View reviewed changes

aldy505 mentioned this pull request Nov 10, 2025

Database/SQL Integration #1128

Open

giortzisg reviewed Nov 17, 2025

View reviewed changes

Merge branch 'master' into feat/sentrysql

6e6e5f8

cursor bot reviewed Nov 19, 2025

View reviewed changes

sentrysql/conn.go Outdated Show resolved Hide resolved

sentrysql/stmt.go Outdated Show resolved Hide resolved

sentrysql/stmt.go Show resolved Hide resolved

fix(sentrysql): use context provided by the newly created span

b5458bc

cursor bot reviewed Nov 19, 2025

View reviewed changes

fix(sentrysql): put context on the right place

e7ac573

cursor bot reviewed Nov 19, 2025

View reviewed changes

sentrysql/stmt.go Show resolved Hide resolved

fix: Context from ExecContext lost in legacy driver fallback

ed45c51

cursor bot reviewed Nov 20, 2025

View reviewed changes

aldy505 added 2 commits November 30, 2025 10:35

fix(sentrysql): resolve review from Cursor

f610dc9

Merge branch 'master' into feat/sentrysql

1a78d47

cursor bot reviewed Nov 30, 2025

View reviewed changes

sentrysql/conn.go Show resolved Hide resolved

sentrysql/operation.go Show resolved Hide resolved

aldy505 added 2 commits December 7, 2025 08:34

fix: comments from Cursor

b7c348f

fix: avoid nil mutex

d864e1b

sentry bot reviewed Dec 7, 2025

View reviewed changes

sentrysql/stmt.go Show resolved Hide resolved

cursor bot reviewed Dec 7, 2025

View reviewed changes

sentrysql/stmt.go Outdated Show resolved Hide resolved

_examples/sql/main.go Show resolved Hide resolved

fix: remove lazy mutex init

1618821

cursor bot reviewed Dec 7, 2025

View reviewed changes

feat: align with sentry-convention

3695056

See https://getsentry.github.io/sentry-conventions/generated/attributes/db.html

cursor bot reviewed Dec 7, 2025

View reviewed changes

	parentSpan := sentry.SpanFromContext(s.ctx)
	parentSpan := sentry.SpanFromContext(ctx)

	rows, err := queryerContext.QueryContext(ctx, query, args)
	rows, err := queryerContext.QueryContext(span.Context(), query, args)

	rows, err := execerContext.ExecContext(ctx, query, args)
	rows, err := execerContext.ExecContext(span.Context(), query, args)

	result, err := stmtExecContext.ExecContext(ctx, args)
	result, err := stmtExecContext.ExecContext(span.Context(), args)

feat: database/sql integration #893

Are you sure you want to change the base?

feat: database/sql integration #893

Uh oh!

Conversation

aldy505 commented Oct 18, 2024 • edited by dingsdax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aldy505 commented Oct 19, 2024

Uh oh!

aldy505 commented Oct 19, 2024

Uh oh!

Uh oh!

Uh oh!

ribice commented Oct 22, 2024

Uh oh!

ribice commented Oct 25, 2024

Uh oh!

aldy505 commented Oct 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldy505 commented Sep 25, 2025

Uh oh!

giortzisg commented Sep 25, 2025

Uh oh!

cursor bot Oct 23, 2025

Choose a reason for hiding this comment

Bug: Context Propagation Issue in Sentry Wrappers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Oct 23, 2025

Choose a reason for hiding this comment

Bug: Incorrect Span Parentage in Context-Aware Drivers

Uh oh!

giortzisg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aldy505 commented Oct 18, 2024 •

edited by dingsdax

Loading

codecov bot commented Oct 19, 2024 •

edited

Loading