feat: database/sql integration #893
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #893 +/- ##
==========================================
- Coverage 86.04% 85.38% -0.66%
==========================================
Files 62 70 +8
Lines 6090 6570 +480
==========================================
+ Hits 5240 5610 +370
- Misses 635 724 +89
- Partials 215 236 +21 ☔ 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}
| 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/in/12637?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)
sentrysql/sentrysql.go
Outdated
| return sqllexer.DBMSSQLServer | ||
| case MSSQL: | ||
| return sqllexer.DBMSMySQL | ||
| case SQLite: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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.
| 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)
| users = append(users, user) | ||
| } | ||
|
|
||
| return users, nil |
There was a problem hiding this comment.
Missing rows.Err() check after iterating database rows
Medium Severity
In GetAllUsers, after iterating with rows.Next(), the code returns without checking rows.Err(). In Go's database/sql, when rows.Next() returns false due to an error (like a network issue mid-stream), the error is only available through rows.Err(). Without this check, the function may silently return partial results with a nil error, causing data loss that goes unnoticed. This is an example file that users are likely to copy, making this pattern particularly concerning.
| users = append(users, user) | ||
| } | ||
|
|
||
| return users, nil |
There was a problem hiding this comment.
Missing rows.Err() check after iterating database rows
Medium Severity
In GetAllUsers, after the for rows.Next() loop completes, there's no call to rows.Err() before returning. According to Go's database/sql documentation, rows.Next() returns false both when there are no more rows AND when an error occurs during iteration. Without checking rows.Err(), errors that happen mid-iteration (other than Scan errors) will be silently ignored, potentially returning incomplete data. Since this is example code that users will reference, it sets an incorrect pattern.
| conn, err := fdriver.Open(c.name) | ||
| conn.(*fakeConn).waiter = c.waiter | ||
| return conn, err | ||
| } |
There was a problem hiding this comment.
Missing error check causes nil pointer dereference
Medium Severity
In fakeConnector.Connect, the return value from fdriver.Open(c.name) is used without checking if an error occurred first. The code immediately accesses conn.(*fakeConn).waiter before verifying that conn is not nil. If Open fails and returns (nil, error), this will cause a nil pointer dereference panic, crashing the tests.
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