-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Add Supabase Queues support #15921
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: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
bbadd60 to
e7b3370
Compare
1bc2897 to
d387514
Compare
e7b3370 to
56a2d84
Compare
56a2d84 to
13d60e0
Compare
423397d to
556703c
Compare
13d60e0 to
74869e4
Compare
e63915a to
719c8b6
Compare
| }, | ||
| }, | ||
| async span => { | ||
| return (Reflect.apply(target, thisArg, argumentsList) as Promise<unknown>).then((res: unknown) => { |
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 should probably also end the span when it throws/rejects? We can also set the status of the span then.
| name: 'supabase.db.rpc', | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.db.supabase', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op, |
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 think we can add the messaging.system attribute to be 'supabase' as described in https://develop.sentry.dev/sdk/telemetry/traces/modules/queues/
| const isProducerSpan = argumentsList[0] === 'enqueue'; | ||
| const isConsumerSpan = argumentsList[0] === 'dequeue'; |
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 don't know if this recently changed, but here they show send and pop as rpc args: https://supabase.com/docs/guides/queues/quickstart#enqueueing-and-dequeueing-messages 🤔
fb8eeb1 to
a8da234
Compare
ff2f804 to
5ce5ee9
Compare
|
@sentry review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request introduces instrumentation for Supabase queue operations using pgmq, enabling Sentry to capture spans and breadcrumbs for queue publishing and processing. This provides visibility into asynchronous task execution within Supabase applications. Click to see moreKey Technical ChangesThe key technical changes include: 1) Instrumenting the Architecture DecisionsThe primary architectural decision involves using Proxy objects to intercept calls to the Supabase client's Dependencies and InteractionsThis integration depends on the Risk ConsiderationsPotential risks include: 1) Performance overhead due to the instrumentation, although proxies are generally performant. 2) Incorrectly identifying queue operations, leading to spurious spans. 3) Failure to propagate trace context if consumers are not properly instrumented. 4) Security implications of modifying message bodies, although the injected data is limited to Sentry trace context. 5) The modification of the arguments list in place could lead to unexpected side effects. Notable Implementation DetailsNotable implementation details include: 1) The use of |
4f19854 to
1c518a2
Compare
|
anybody got eyes on this anymore? 😅 |
1c518a2 to
327b57a
Compare
327b57a to
1ed3684
Compare
1ed3684 to
2aef286
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
2aef286 to
c4f100a
Compare
c4f100a to
9accd4b
Compare
| op: 'queue.publish', | ||
| attributes: expect.objectContaining({ | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.queue.supabase.producer', | ||
| 'messaging.system': 'supabase', |
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: Mismatched Sentry Origin for Supabase Queue spans
The unit tests for Supabase queue instrumentation expect incorrect sentry.origin values for both producer and consumer spans. The tests assert 'auto.queue.supabase.producer' and 'auto.queue.supabase.consumer', but the implementation actually sets 'auto.db.supabase.queue.producer' and 'auto.db.supabase.queue.consumer'. This mismatch causes test failures, while E2E tests correctly reflect the implementation.
Additional Locations (1)
| await supabaseClient.rpc('send', { | ||
| queue_name: 'todos', | ||
| msg: { title: 'Test Todo' }, | ||
| }); |
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: Inconsistent RPC Payloads Break Sentry Tracing
The Supabase RPC calls for queue operations use msg for the message payload. Sentry's queue instrumentation expects message, which prevents proper trace context injection.
Additional Locations (1)
| 'sentry.origin': 'auto.db.supabase', | ||
| 'messaging.destination.name': 'todos', | ||
| 'messaging.message.id': '0', | ||
| }), |
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: Origin value mismatch in browser test
The browser integration test expects sentry.origin to be 'auto.db.supabase', but the actual implementation sets it to 'auto.db.supabase.queue.producer' (line 288 in supabase.ts). This mismatch means the test is checking for the wrong value and may pass incorrectly or fail unexpectedly. The same issue exists for the consumer span on lines 77-79, where the test expects 'auto.db.supabase' but the implementation uses 'auto.db.supabase.queue.consumer'.
| 'sentry.origin': 'auto.db.supabase', | ||
| 'messaging.destination.name': 'todos', | ||
| 'messaging.message.id': '0', | ||
| }), |
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: Origin string mismatch in browser tests
The browser integration test expects sentry.origin to be 'auto.db.supabase', but the actual implementation sets it to 'auto.db.supabase.queue.producer' (line 288 in supabase.ts). This mismatch means the test is checking for the wrong value and may pass incorrectly or fail unexpectedly. The same issue exists for the consumer span on lines 77-79, where the test expects 'auto.db.supabase' but the implementation uses 'auto.db.supabase.queue.consumer'.
| await supabaseClient.schema('pgmq_public').rpc('send', { | ||
| queue_name: 'todos', | ||
| msg: { title: 'Test Todo' }, | ||
| }); |
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: Incorrect RPC Parameter Name in Browser Test
The browser integration test uses incorrect parameter name msg for the send RPC call, but according to the SQL migration file (line 35 of enable-queues.sql), the correct parameter name is message. This will cause the test to fail when calling the actual Supabase queue function. All e2e tests correctly use message: as the parameter name.
Resolves: #14611
Summary:
Sample Queue Event: Link
supabaseClient.rpc('<queue_op>', ...)supabaseClient.schema(...).rpc('queue_op', ...)producerops:send,send_batchconsumerop:pop