feat(cloudflare): Add withSentry method
#13025
Conversation
AbhiPrasad
requested review from
lforst and
andreiborza
and removed request for
a team
size-limit report 📦
|
packages/cloudflare/README.md
Outdated
| npm install @sentry/cloudflare | ||
| ``` | ||
|
|
||
| Then set either the `nodejs_compat` or `nodejs_als` compatibility flags in your `wrangler.toml`: |
There was a problem hiding this comment.
l: Could we add a sentence of why this is needed? I saw Because we require tracing support as top-of-mind priority, AsyncLocalStorage support is a requirement of the SDK. in the issue you made.
| // Set user information, as well as tags and further extras | ||
| Sentry.setExtra('battery', 0.7); | ||
| Sentry.setTag('user_mode', 'admin'); | ||
| Sentry.setUser({ id: '4711' }); | ||
|
|
||
| // Add a breadcrumb for future events | ||
| Sentry.addBreadcrumb({ | ||
| message: 'My Breadcrumb', | ||
| // ... | ||
| }); | ||
|
|
||
| // Capture exceptions, messages or manual events | ||
| Sentry.captureMessage('Hello, world!'); | ||
| Sentry.captureException(new Error('Good bye')); | ||
| Sentry.captureEvent({ | ||
| message: 'Manual', | ||
| stacktrace: [ | ||
| // ... | ||
| ], | ||
| }); |
There was a problem hiding this comment.
q: From your issue:
This needs to be done because workers can change their bindings (env) without re-deploying the entire worker, so a top-level Sentry.init call would become stale.
Wouldn't these suffer from the same issue of going stale?
There was a problem hiding this comment.
Nope, because under the hood they call getClient() which should always have the current client in the handler execution context, therefore will stay up-to-date.
packages/cloudflare/src/handler.ts
Outdated
| setHttpStatus(span, res.status); | ||
| return res; | ||
| } catch (e) { | ||
| captureException(e, { mechanism: { handled: false } }); |
There was a problem hiding this comment.
q: I've seen some sdks add additional keys to mechanism, e.g. type: 'bun', do we want that here too? why or why not?
There was a problem hiding this comment.
As per the develop docs
Required unique identifier of this mechanism determining rendering and processing of the mechanism data.
In the Python SDK this is merely the name of the framework integration that produced the exception, while for native it is e.g. "minidump" or "applecrashreport".
Changed with 309c05a
| @@ -0,0 +1,300 @@ | |||
| // Note: These tests run the handler in Node.js, which is has some differences to the cloudflare workers runtime. | |||
There was a problem hiding this comment.
| // Note: These tests run the handler in Node.js, which is has some differences to the cloudflare workers runtime. | |
| // Note: These tests run the handler in Node.js, which has some differences to the cloudflare workers runtime. |
| const handler = { | ||
| async fetch(_request, _env, _context) { | ||
| expect(SentryCore.getClient() instanceof CloudflareClient).toBe(true); | ||
| return new Response('test'); | ||
| }, | ||
| } satisfies ExportedHandler; | ||
|
|
||
| const context = createMockExecutionContext(); | ||
| const wrappedHandler = withSentry(() => ({}), handler); | ||
| await wrappedHandler.fetch(new Request('https://example.com'), MOCK_ENV, context); | ||
|
|
||
| expect.assertions(1); |
There was a problem hiding this comment.
m: I think a robuster way to write this is to create a vi.fn that gets called with SentryCore.getClient() and assert that it has been called with that.
There was a problem hiding this comment.
Good call!
Changed with 61ad09f
| const handler = { | ||
| async fetch(_request, _env, _context) { | ||
| SentryCore.captureMessage('test'); | ||
| return new Response('test'); | ||
| }, | ||
| } satisfies ExportedHandler; | ||
|
|
||
| let sentryEvent: Event = {}; | ||
| const wrappedHandler = withSentry( | ||
| (env: any) => ({ | ||
| dsn: env.MOCK_DSN, | ||
| beforeSend(event) { | ||
| sentryEvent = event; | ||
| return null; | ||
| }, | ||
| }), | ||
| handler, | ||
| ); |
There was a problem hiding this comment.
l: can we extract this into a helper just call that in the next few tests?
There was a problem hiding this comment.
I'll come back to take a look at this after this PR merges, good suggestion.
| import * as SentryCore from '@sentry/core'; | ||
| import { init } from '../src/sdk'; | ||
|
|
||
| describe('init', () => { |
There was a problem hiding this comment.
m: could we add a test for init returning a client?
| export default withSentry( | ||
| (env) => ({ | ||
| dsn: env.SENTRY_DSN, | ||
| tracesSampleRate: 1.0, |
There was a problem hiding this comment.
Maybe add a comment explaining this value?
AbhiPrasad
force-pushed
the
abhi-cloudflare-with-sentry
branch
from
87b4fa1 to
e02e283
Compare
ref #12620
before reviewing this PR, I recommend reading through a writeup I did: #13007
This PR adds
withSentry, a method that wraps your cloudflare worker handler to add Sentry instrumentation. The writeup above explains why we need to do this over just a regularSentry.initcall.The implementation of
withSentryis fairly straightforward, wrapping the fetch handler in the cloudflare worker with:withIsolationScopeto isolate it from other concurrent requestscontinueTraceto continue distributed tracingstartSpanto track spansUsage looks something like so:
Next step here is to add more robust e2e tests, and then release an initial version!