feat: database/sql integration #893
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #893 +/- ##
==========================================
+ Coverage 86.04% 86.07% +0.03%
==========================================
Files 62 70 +8
Lines 6090 6485 +395
==========================================
+ Hits 5240 5582 +342
- Misses 635 675 +40
- Partials 215 228 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I really don't like golangci-lint, it's so pedantic :( |
|
Should be good by now. |
|
I'll test this locally myself this week and report back. We should also update the docs and main repository with this integration. |
|
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
sentrysql/conn.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| return &sentryTx{originalTx: tx, ctx: s.ctx, config: s.config}, nil |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
sentrysql/stmt.go
Outdated
| return s.Query(values) | ||
| } | ||
|
|
||
| parentSpan := sentry.SpanFromContext(s.ctx) |
There was a problem hiding this comment.
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)
giortzisg
left a comment
giortzisg
left a comment
There was a problem hiding this comment.
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.
sentrysql/conn.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| return &sentryTx{originalTx: tx, ctx: s.ctx, config: s.config}, nil |
There was a problem hiding this comment.
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.
sentrysql/conn.go
Outdated
| s.config.SetData(span, query) | ||
| defer span.Finish() | ||
|
|
||
| rows, err := queryerContext.QueryContext(ctx, query, args) |
There was a problem hiding this comment.
We should propagate the span context here.
| rows, err := queryerContext.QueryContext(ctx, query, args) | |
| rows, err := queryerContext.QueryContext(span.Context(), query, args) |
sentrysql/conn.go
Outdated
| s.config.SetData(span, query) | ||
| defer span.Finish() | ||
|
|
||
| rows, err := execerContext.ExecContext(ctx, query, args) |
There was a problem hiding this comment.
| rows, err := execerContext.ExecContext(ctx, query, args) | |
| rows, err := execerContext.ExecContext(span.Context(), query, args) |
sentrysql/stmt.go
Outdated
| s.config.SetData(span, s.query) | ||
| defer span.Finish() | ||
|
|
||
| result, err := stmtExecContext.ExecContext(ctx, args) |
There was a problem hiding this comment.
| result, err := stmtExecContext.ExecContext(ctx, args) | |
| result, err := stmtExecContext.ExecContext(span.Context(), args) |
| s.ctx = ctx | ||
| return s.Query(values) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| return nil, err | ||
| } | ||
|
|
||
| return s.Query(values) |
There was a problem hiding this comment.
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.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| if tt.WantSpan == nil { | ||
| t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans) | ||
| continue | ||
| } |
There was a problem hiding this comment.
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.
| users = append(users, user) | ||
| } | ||
|
|
||
| return users, nil |
There was a problem hiding this comment.
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.
| if tt.WantSpan == nil { | ||
| t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans) | ||
| continue | ||
| } |
There was a problem hiding this comment.
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).
| if tt.WantSpan == nil { | ||
| t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans) | ||
| continue | ||
| } |
There was a problem hiding this comment.
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)
| users = append(users, user) | ||
| } | ||
|
|
||
| return users, nil |
There was a problem hiding this comment.
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.
|
For those wondering why this is taking too long: We are lacking two missing features for this integration:
If we ship this integration with or without those 2 missing features, we're a bit concerned about the performance penalty (or performance bottleneck) for users. Another option would be just ship the integration and mark it as "experimental and might be removed in very near future", gather feedback about the performance & quality. If it's good enough, we might continue implement those missing features. |
|
Quick update: I will work on this later this weekend. I'll implement client side scrubbing & parameterized span attributes. Meaning we'll bring in one external dependency to do all this. |
| if tt.WantSpan == nil { | ||
| t.Errorf("Expecting no spans, but got %d spans: %v", len(gotSpans), gotSpans) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Test validation check doesn't verify span count
Low Severity
The comment states "if WantSpan is nil, yet we got some spans, it should be an error" but the code only checks if tt.WantSpan == nil without also checking len(gotSpans) > 0. This means if a test expects no spans (WantSpan: nil) and correctly receives no spans, the test would still report an error saying "Expecting no spans, but got 0 spans". The condition needs to verify that spans were actually received before reporting a mismatch. This pattern is repeated in multiple test functions.
Additional Locations (1)
| return sqllexer.DBMSSQLServer | ||
| case MSSQL: | ||
| return sqllexer.DBMSMySQL | ||
| case SQLite: |
There was a problem hiding this comment.
Bug: The toDbmsType function incorrectly returns sqllexer.DBMSSQLServer for MySQL and sqllexer.DBMSMySQL for MSSQL, causing queries to be parsed with the wrong lexer.
Severity: CRITICAL
🔍 Detailed Analysis
In sentrysql/sentrysql.go, the toDbmsType function has swapped the return values for the MySQL and MSSQL database systems. The MySQL case incorrectly returns sqllexer.DBMSSQLServer ("mssql"), and the MSSQL case returns sqllexer.DBMSMySQL ("mysql"). This will cause any user configuring the integration with sentrysql.MySQL or sentrysql.MSSQL to have their SQL queries parsed and normalized using the wrong database's rules. This leads to incorrect span data, such as table and operation names, being sent to Sentry, impacting the accuracy of the Queries Insights feature.
💡 Suggested Fix
In the toDbmsType function, swap the return values for the MySQL and MSSQL cases. The MySQL case should return sqllexer.DBMSMySQL, and the MSSQL case should return sqllexer.DBMSSQLServer.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentrysql/sentrysql.go#L34-L37
Potential issue: In `sentrysql/sentrysql.go`, the `toDbmsType` function has swapped the
return values for the `MySQL` and `MSSQL` database systems. The `MySQL` case incorrectly
returns `sqllexer.DBMSSQLServer` ("mssql"), and the `MSSQL` case returns
`sqllexer.DBMSMySQL` ("mysql"). This will cause any user configuring the integration
with `sentrysql.MySQL` or `sentrysql.MSSQL` to have their SQL queries parsed and
normalized using the wrong database's rules. This leads to incorrect span data, such as
table and operation names, being sent to Sentry, impacting the accuracy of the Queries
Insights feature.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8402173
| case MySQL: | ||
| return sqllexer.DBMSSQLServer | ||
| case MSSQL: | ||
| return sqllexer.DBMSMySQL |
There was a problem hiding this comment.
MySQL and MSSQL database type mappings are swapped
High Severity
The toDbmsType() function has the MySQL and MSSQL database system mappings swapped. MySQL returns sqllexer.DBMSSQLServer and MSSQL returns sqllexer.DBMSMySQL, which is backwards. This causes incorrect SQL obfuscation and normalization for all MySQL and Microsoft SQL Server users, leading to wrong query grouping in Sentry Queries Insights and potentially incorrect parameter redaction.
| } | ||
| if len(statementMetadata.Procedures) > 0 { | ||
| span.SetData("db.operation.name", statementMetadata.Procedures[0]) | ||
| } |
There was a problem hiding this comment.
Wrong metadata field used for SQL operation name
Medium Severity
The code uses statementMetadata.Procedures to set db.operation.name, but it should use statementMetadata.Commands. The normalizer is configured with WithCollectCommands(true) to collect SQL commands (SELECT, INSERT, UPDATE, DELETE), which populate the Commands field. For standard SQL queries without stored procedure calls, Procedures will be empty, causing db.operation.name to not be set even when normalization succeeds. The fallback code on lines 78-81 correctly uses SQL commands, indicating Commands is the intended source.
| users = append(users, user) | ||
| } | ||
|
|
||
| return users, nil |
There was a problem hiding this comment.
Missing rows.Err() check after iteration loop
Low Severity
The GetAllUsers function iterates through rows.Next() but does not check rows.Err() after the loop before returning. When rows.Next() returns false due to an error (e.g., network failure, context cancellation), the error is only accessible via rows.Err(). Without this check, the function silently returns partial data as if successful. Since this is example code that users will reference, this pattern may be copied into production code.
Additional Locations (1)
6 participants
(From this comment):
For those wondering why this is taking too long:
We are lacking two missing features for this integration:
If we ship this integration with or without those 2 missing features, we're a bit concerned about the performance penalty (or performance bottleneck) for users.
Another option would be just ship the integration and mark it as "experimental and might be removed in very near future", gather feedback about the performance & quality. If it's good enough, we might continue implement those missing features.
Queries insights tracing for Go.
closes #1128
closes GO-95