Skip to content

Commit f306352

Browse files
JPeer264JPeer264
committed
fix(cloudflare): Ensure every request is instruments the functions
1 parent 119c06f commit f306352

27 files changed

Lines changed: 687 additions & 143 deletions

‎dev-packages/cloudflare-integration-tests/suites/double-instrumentation/index.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import * as Sentry from '@sentry/cloudflare';
2+
3+
interface Env {
4+
SENTRY_DSN: string;
5+
}
6+
7+
const handler = {
8+
async fetch(request: Request, _env: Env, _ctx: ExecutionContext) {
9+
if (request.url.includes('/error')) {
10+
throw new Error('Test error from double-instrumented worker');
11+
}
12+
return new Response('ok');
13+
},
14+
};
15+
16+
// Deliberately call withSentry twice on the same handler object.
17+
// This simulates scenarios where the module is re-evaluated or the handler
18+
// is wrapped multiple times. The SDK should handle this gracefully
19+
// without double-wrapping (which would cause duplicate error reports).
20+
const once = Sentry.withSentry((env: Env) => ({ dsn: env.SENTRY_DSN }), handler);
21+
export default Sentry.withSentry((env: Env) => ({ dsn: env.SENTRY_DSN }), once);

‎dev-packages/cloudflare-integration-tests/suites/double-instrumentation/test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import { expect, it } from 'vitest';
2+
import { eventEnvelope } from '../../expect';
3+
import { createRunner } from '../../runner';
4+
5+
it('Only sends one error event when withSentry is called twice', async ({ signal }) => {
6+
const runner = createRunner(__dirname)
7+
.expect(
8+
eventEnvelope({
9+
level: 'error',
10+
exception: {
11+
values: [
12+
{
13+
type: 'Error',
14+
value: 'Test error from double-instrumented worker',
15+
stacktrace: {
16+
frames: expect.any(Array),
17+
},
18+
mechanism: { type: 'auto.http.cloudflare', handled: false },
19+
},
20+
],
21+
},
22+
request: {
23+
headers: expect.any(Object),
24+
method: 'GET',
25+
url: expect.any(String),
26+
},
27+
}),
28+
)
29+
.start(signal);
30+
await runner.makeRequest('get', '/error', { expectError: true });
31+
await runner.completed();
32+
});
33+
34+
it('Successful response works when withSentry is called twice', async ({ signal }) => {
35+
const runner = createRunner(__dirname).start(signal);
36+
const response = await runner.makeRequest<string>('get', '/');
37+
expect(response).toBe('ok');
38+
});

‎dev-packages/cloudflare-integration-tests/suites/double-instrumentation/wrangler.jsonc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"name": "worker-name",
3+
"compatibility_date": "2025-06-17",
4+
"main": "index.ts",
5+
"compatibility_flags": ["nodejs_compat"],
6+
}

‎dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/index.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import * as Sentry from '@sentry/cloudflare';
2+
import { DurableObject } from 'cloudflare:workers';
3+
4+
interface Env {
5+
SENTRY_DSN: string;
6+
TEST_DURABLE_OBJECT: DurableObjectNamespace;
7+
}
8+
9+
class TestDurableObjectBase extends DurableObject<Env> {
10+
public constructor(ctx: DurableObjectState, env: Env) {
11+
super(ctx, env);
12+
}
13+
14+
// eslint-disable-next-line @typescript-eslint/explicit-member-accessibility
15+
async doWork(): Promise<string> {
16+
const results: string[] = [];
17+
18+
for (let i = 1; i <= 5; i++) {
19+
await Sentry.startSpan({ name: `task-${i}`, op: 'task' }, async () => {
20+
// Simulate async work
21+
await new Promise<void>(resolve => setTimeout(resolve, 1));
22+
results.push(`done-${i}`);
23+
});
24+
}
25+
26+
return results.join(',');
27+
}
28+
}
29+
30+
export const TestDurableObject = Sentry.instrumentDurableObjectWithSentry(
31+
(env: Env) => ({
32+
dsn: env.SENTRY_DSN,
33+
tracesSampleRate: 1.0,
34+
instrumentPrototypeMethods: true,
35+
}),
36+
TestDurableObjectBase,
37+
);
38+
39+
export default {
40+
async fetch(_request: Request, env: Env): Promise<Response> {
41+
const id: DurableObjectId = env.TEST_DURABLE_OBJECT.idFromName('test');
42+
const stub = env.TEST_DURABLE_OBJECT.get(id) as unknown as TestDurableObjectBase;
43+
const result = await stub.doWork();
44+
return new Response(result);
45+
},
46+
};

‎dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import { expect, it } from 'vitest';
2+
import { createRunner } from '../../../runner';
3+
4+
// Regression test for https://github.com/getsentry/sentry-javascript/issues/20030
5+
// When a Durable Object method calls Sentry.startSpan multiple times, those spans
6+
// must appear as children of the DO transaction. The first invocation always worked;
7+
// the second invocation on the same DO instance previously lost its child spans
8+
// because the client was disposed after the first call.
9+
it('sends child spans on repeated Durable Object calls', async ({ signal }) => {
10+
function assertDoWorkEnvelope(envelope: unknown): void {
11+
const transactionEvent = (envelope as any)[1]?.[0]?.[1];
12+
13+
expect(transactionEvent).toEqual(
14+
expect.objectContaining({
15+
transaction: 'doWork',
16+
contexts: expect.objectContaining({
17+
trace: expect.objectContaining({
18+
op: 'rpc',
19+
origin: 'auto.faas.cloudflare.durable_object',
20+
}),
21+
}),
22+
}),
23+
);
24+
25+
// All 5 child spans should be present
26+
expect(transactionEvent.spans).toHaveLength(5);
27+
expect(transactionEvent.spans).toEqual(
28+
expect.arrayContaining([
29+
expect.objectContaining({ description: 'task-1', op: 'task' }),
30+
expect.objectContaining({ description: 'task-2', op: 'task' }),
31+
expect.objectContaining({ description: 'task-3', op: 'task' }),
32+
expect.objectContaining({ description: 'task-4', op: 'task' }),
33+
expect.objectContaining({ description: 'task-5', op: 'task' }),
34+
]),
35+
);
36+
37+
// All child spans share the root trace_id
38+
const rootTraceId = transactionEvent.contexts?.trace?.trace_id;
39+
expect(rootTraceId).toBeDefined();
40+
for (const span of transactionEvent.spans) {
41+
expect(span.trace_id).toBe(rootTraceId);
42+
}
43+
}
44+
45+
// Expect 5 transaction envelopes — one per call.
46+
const runner = createRunner(__dirname)
47+
.expectN(5, assertDoWorkEnvelope)
48+
.start(signal);
49+
50+
await runner.makeRequest('get', '/');
51+
await runner.makeRequest('get', '/');
52+
await runner.makeRequest('get', '/');
53+
await runner.makeRequest('get', '/');
54+
await runner.makeRequest('get', '/');
55+
await runner.completed();
56+
});

‎dev-packages/cloudflare-integration-tests/suites/tracing/durableobject-spans/wrangler.jsonc‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
{
2+
"name": "worker-name",
3+
"main": "index.ts",
4+
"compatibility_date": "2025-06-17",
5+
"migrations": [
6+
{
7+
"new_sqlite_classes": ["TestDurableObject"],
8+
"tag": "v1",
9+
},
10+
],
11+
"durable_objects": {
12+
"bindings": [
13+
{
14+
"class_name": "TestDurableObject",
15+
"name": "TEST_DURABLE_OBJECT",
16+
},
17+
],
18+
},
19+
"compatibility_flags": ["nodejs_als"],
20+
}

‎dev-packages/cloudflare-integration-tests/suites/tracing/durableobject/wrangler.jsonc‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,4 @@
1717
],
1818
},
1919
"compatibility_flags": ["nodejs_als"],
20-
"vars": {
21-
"SENTRY_DSN": "https://932e620ee3921c3b4a61c72558ad88ce@o447951.ingest.us.sentry.io/4509553159831552",
22-
},
2320
}

‎dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler-sub-worker.jsonc‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,5 @@
22
"name": "cloudflare-service-binding-sub-worker",
33
"main": "index-sub-worker.ts",
44
"compatibility_date": "2025-06-17",
5-
"compatibility_flags": ["nodejs_als"],
6-
"vars": {
7-
"SENTRY_DSN": "https://932e620ee3921c3b4a61c72558ad88ce@o447951.ingest.us.sentry.io/4509553159831552",
8-
},
5+
"compatibility_flags": ["nodejs_als"]
96
}

‎dev-packages/cloudflare-integration-tests/suites/tracing/worker-service-binding/wrangler.jsonc‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
33
"main": "index.ts",
44
"compatibility_date": "2025-06-17",
55
"compatibility_flags": ["nodejs_als"],
6-
"vars": {
7-
"SENTRY_DSN": "https://932e620ee3921c3b4a61c72558ad88ce@o447951.ingest.us.sentry.io/4509553159831552",
8-
},
96
"services": [
107
{
118
"binding": "ANOTHER_WORKER",

‎packages/cloudflare/src/durableobject.ts‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { captureException } from '@sentry/core';
33
import type { DurableObject } from 'cloudflare:workers';
44
import { setAsyncLocalStorageAsyncContextStrategy } from './async';
55
import type { CloudflareOptions } from './client';
6-
import { isInstrumented, markAsInstrumented } from './instrument';
6+
import { ensureInstrumented, getInstrumented, markAsInstrumented } from './instrument';
77
import { getFinalOptions } from './options';
88
import { wrapRequestHandler } from './request';
99
import { instrumentContext } from './utils/instrumentContext';
@@ -67,16 +67,16 @@ export function instrumentDurableObjectWithSentry<
6767

6868
// Any other public methods on the Durable Object instance are RPC calls.
6969

70-
if (obj.fetch && typeof obj.fetch === 'function' && !isInstrumented(obj.fetch)) {
71-
obj.fetch = new Proxy(obj.fetch, {
72-
apply(target, thisArg, args) {
73-
return wrapRequestHandler({ options, request: args[0], context }, () =>
74-
Reflect.apply(target, thisArg, args),
75-
);
76-
},
77-
});
78-
79-
markAsInstrumented(obj.fetch);
70+
if (obj.fetch && typeof obj.fetch === 'function') {
71+
obj.fetch = ensureInstrumented(obj.fetch, original =>
72+
new Proxy(original, {
73+
apply(target, thisArg, args) {
74+
return wrapRequestHandler({ options, request: args[0], context }, () => {
75+
return Reflect.apply(target, thisArg, args);
76+
});
77+
},
78+
}),
79+
);
8080
}
8181

8282
if (obj.alarm && typeof obj.alarm === 'function') {
@@ -177,7 +177,18 @@ function instrumentPrototype<T extends NewableFunction>(target: T, methodsToInst
177177
methodNames.forEach(methodName => {
178178
const originalMethod = (proto as Record<string, unknown>)[methodName];
179179

180-
if (!originalMethod || isInstrumented(originalMethod)) {
180+
if (!originalMethod) {
181+
return;
182+
}
183+
184+
const existingInstrumented = getInstrumented(originalMethod);
185+
if (existingInstrumented) {
186+
Object.defineProperty(proto, methodName, {
187+
value: existingInstrumented,
188+
enumerable: false,
189+
writable: true,
190+
configurable: true,
191+
});
181192
return;
182193
}
183194

@@ -216,6 +227,11 @@ function instrumentPrototype<T extends NewableFunction>(target: T, methodsToInst
216227
return wrapper.apply(this, args);
217228
};
218229

230+
// Only mark wrappedMethod as instrumented (not originalMethod → wrappedMethod).
231+
// originalMethod must stay unmapped because wrappedMethod calls
232+
// wrapMethodWithSentry(options, originalMethod) on each invocation to create
233+
// a per-instance proxy. If originalMethod mapped to wrappedMethod, that call
234+
// would return wrappedMethod itself, causing infinite recursion.
219235
markAsInstrumented(wrappedMethod);
220236

221237
// Replace the prototype method

0 commit comments

Comments
 (0)