-
-
Notifications
You must be signed in to change notification settings - Fork 267
Sentry Supabase Integration #2913
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2913 +/- ##
==========================================
- Coverage 87.88% 87.76% -0.13%
==========================================
Files 287 287
Lines 9780 9780
==========================================
- Hits 8595 8583 -12
- Misses 1185 1197 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Android Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
79f6b41 | 469.66 ms | 525.90 ms | 56.24 ms |
2d34233 | 470.54 ms | 558.90 ms | 88.36 ms |
6f47800 | 451.04 ms | 509.64 ms | 58.60 ms |
dbd526b | 504.88 ms | 569.02 ms | 64.15 ms |
4481076 | 484.08 ms | 505.70 ms | 21.61 ms |
73a3c38 | 478.18 ms | 526.62 ms | 48.44 ms |
6ba4675 | 499.80 ms | 632.43 ms | 132.63 ms |
aeb02f2 | 373.84 ms | 437.00 ms | 63.16 ms |
73dca78 | 476.53 ms | 522.21 ms | 45.68 ms |
640ad0c | 466.00 ms | 552.67 ms | 86.67 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
79f6b41 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
2d34233 | 6.54 MiB | 7.55 MiB | 1.01 MiB |
6f47800 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
dbd526b | 6.54 MiB | 7.69 MiB | 1.15 MiB |
4481076 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
73a3c38 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
6ba4675 | 6.54 MiB | 7.53 MiB | 1015.26 KiB |
aeb02f2 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
73dca78 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
640ad0c | 6.54 MiB | 7.69 MiB | 1.15 MiB |
Previous results on branch: feat/supabase
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
34b0923 | 509.55 ms | 590.18 ms | 80.62 ms |
d2f4fd9 | 453.31 ms | 507.02 ms | 53.71 ms |
e4d1bf6 | 499.64 ms | 528.47 ms | 28.82 ms |
6a8651f | 476.65 ms | 532.62 ms | 55.96 ms |
0ab2edd | 459.85 ms | 506.63 ms | 46.77 ms |
0a08454 | 515.10 ms | 634.67 ms | 119.57 ms |
30c089b | 471.71 ms | 522.65 ms | 50.94 ms |
8bd9497 | 470.21 ms | 524.26 ms | 54.05 ms |
5e3ca91 | 451.24 ms | 496.74 ms | 45.50 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
34b0923 | 6.44 MiB | 7.43 MiB | 1013.10 KiB |
d2f4fd9 | 6.54 MiB | 7.53 MiB | 1017.45 KiB |
e4d1bf6 | 6.44 MiB | 7.44 MiB | 1017.53 KiB |
6a8651f | 6.54 MiB | 7.53 MiB | 1015.94 KiB |
0ab2edd | 6.54 MiB | 7.53 MiB | 1016.33 KiB |
0a08454 | 6.44 MiB | 7.43 MiB | 1013.22 KiB |
30c089b | 6.44 MiB | 7.43 MiB | 1010.46 KiB |
8bd9497 | 6.44 MiB | 7.43 MiB | 1013.21 KiB |
5e3ca91 | 6.44 MiB | 7.44 MiB | 1017.53 KiB |
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b6c8720 | 1252.65 ms | 1266.61 ms | 13.96 ms |
6f47800 | 1247.52 ms | 1259.37 ms | 11.85 ms |
640ad0c | 1241.04 ms | 1253.96 ms | 12.92 ms |
73a3c38 | 1263.37 ms | 1277.90 ms | 14.53 ms |
73dca78 | 1246.65 ms | 1265.42 ms | 18.76 ms |
6ba4675 | 1223.12 ms | 1238.17 ms | 15.04 ms |
93b7728 | 1247.23 ms | 1264.87 ms | 17.64 ms |
2d34233 | 1258.19 ms | 1268.92 ms | 10.73 ms |
ec78888 | 1251.37 ms | 1269.40 ms | 18.04 ms |
dbd526b | 1244.78 ms | 1259.02 ms | 14.24 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b6c8720 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
6f47800 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
640ad0c | 7.86 MiB | 9.44 MiB | 1.58 MiB |
73a3c38 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
73dca78 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
6ba4675 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
93b7728 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
2d34233 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
ec78888 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
dbd526b | 7.86 MiB | 9.44 MiB | 1.58 MiB |
Previous results on branch: feat/supabase
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8bd9497 | 1265.35 ms | 1273.98 ms | 8.63 ms |
d2f4fd9 | 1252.60 ms | 1271.13 ms | 18.52 ms |
6a8651f | 1254.15 ms | 1266.88 ms | 12.73 ms |
30c089b | 1257.23 ms | 1269.16 ms | 11.93 ms |
5e3ca91 | 1263.90 ms | 1277.43 ms | 13.53 ms |
0ab2edd | 1254.71 ms | 1264.28 ms | 9.56 ms |
e4d1bf6 | 1249.16 ms | 1267.12 ms | 17.96 ms |
0a08454 | 1265.15 ms | 1283.18 ms | 18.04 ms |
34b0923 | 1269.29 ms | 1275.88 ms | 6.59 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8bd9497 | 8.43 MiB | 10.01 MiB | 1.58 MiB |
d2f4fd9 | 7.86 MiB | 9.45 MiB | 1.59 MiB |
6a8651f | 7.85 MiB | 9.42 MiB | 1.57 MiB |
30c089b | 8.43 MiB | 10.01 MiB | 1.58 MiB |
5e3ca91 | 8.43 MiB | 10.00 MiB | 1.56 MiB |
0ab2edd | 7.85 MiB | 9.45 MiB | 1.59 MiB |
e4d1bf6 | 8.43 MiB | 10.00 MiB | 1.56 MiB |
0a08454 | 8.43 MiB | 10.01 MiB | 1.58 MiB |
34b0923 | 8.43 MiB | 10.01 MiB | 1.58 MiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick pass, I'll have a closer look later
static Map<String, dynamic>? _readBody(String table, BaseRequest request) { | ||
final bodyString = | ||
request is Request && request.body.isNotEmpty ? request.body : null; | ||
final body = bodyString != null ? jsonDecode(bodyString) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we introduce some limit? or does the supabase SDK already handle big request bodies?
description, | ||
'db.${supabaseRequest.operation.value}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can do span op: db.sql.query
and description to be the full supabase query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure i understand. Could you give me an example for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like:
description: 'SELECT * FROM TABLE'
op: 'db.sql.query'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following the conventions: https://develop.sentry.dev/sdk/telemetry/traces/span-operations/#database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for this, including most common operations/clauses.
class SentrySupabaseTracingClient extends BaseClient { | ||
final Client _innerClient; | ||
final Hub _hub; | ||
|
||
SentrySupabaseTracingClient(this._innerClient, this._hub); | ||
|
||
@override | ||
Future<StreamedResponse> send(BaseRequest request) async { | ||
final supabaseRequest = SentrySupabaseRequest.fromRequest(request); | ||
|
||
final span = _createSpan(supabaseRequest); | ||
|
||
StreamedResponse? response; | ||
|
||
try { | ||
response = await _innerClient.send(request); | ||
|
||
span?.setData('http.response.status_code', response.statusCode); | ||
span?.setData('http.response_content_length', response.contentLength); | ||
span?.status = SpanStatus.fromHttpStatusCode(response.statusCode); | ||
} catch (e) { | ||
span?.throwable = e; | ||
span?.status = SpanStatus.internalError(); | ||
rethrow; | ||
} finally { | ||
await span?.finish(); | ||
} | ||
|
||
return response; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is also triggered for auth requests? we gotta make sure this only handles database operations
auth tracing should be separate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now only handling the restful requests, ignoring everything else (including auth).
# Conflicts: # .craft.yml
Will make another review pass soon |
@denrase pls take a look at cursor's comment I think the concern is valid since we'd be doing this for every request |
@buenaflor 🤦♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Sentry JSON Parsing Error Handling
The _readBody
method in SentrySupabaseRequest
calls jsonDecode
without error handling. If the request body contains malformed JSON, an unhandled FormatException
is thrown. This exception propagates, causing the entire HTTP request to fail and potentially crash the application. This violates the non-invasive nature of Sentry integration, as it prevents requests from reaching Supabase that might otherwise be processed. The jsonDecode
call should be wrapped in a try-catch
block to gracefully return null
on FormatException
.
supabase/lib/src/sentry_supabase_request.dart#L75-L86
sentry-dart/supabase/lib/src/sentry_supabase_request.dart
Lines 75 to 86 in 27911aa
static Map<String, dynamic>? _readBody(String table, BaseRequest request) { | |
final bodyString = | |
request is Request && request.body.isNotEmpty ? request.body : null; | |
final body = bodyString != null ? jsonDecode(bodyString) : null; | |
if (body is Map<String, dynamic>) { | |
return body; | |
} else { | |
return null; | |
} | |
} |
Bug: SentrySupabaseRequest Path Validation Flaw
The path validation in SentrySupabaseRequest.fromRequest
is insufficient. While it checks for the /rest/v1
prefix, it incorrectly assumes url.pathSegments.last
will always be a valid table name. For URLs like /rest/v1
or /rest/v1/
, this results in 'v1' or an empty string being extracted as the table name, leading to incorrect parsing. The code should validate that there are at least three path segments and that the last segment represents a non-empty table name.
supabase/lib/src/sentry_supabase_request.dart#L22-L31
sentry-dart/supabase/lib/src/sentry_supabase_request.dart
Lines 22 to 31 in 27911aa
static SentrySupabaseRequest? fromRequest(BaseRequest request) { | |
final url = request.url; | |
// Ignoring URLS like https://example.com/auth/v1/token?grant_type=password | |
// Only consider requests to the REST API. | |
if (!url.path.startsWith('/rest/v1')) { | |
return null; | |
} | |
final table = url.pathSegments.last; | |
final operation = _extractOperation(request.method, request.headers); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
📜 Description
Supabase instrumentation for database operations (no auth instrumentation)
💡 Motivation and Context
Closes #2727
💚 How did you test it?
Unit tests.
📝 Checklist
sendDefaultPii
is enabled