Skip to content

fix(cloudflare): Require a parent span for storage and D1 operations #25860

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.

Sign up for GitHub

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

Closed
lazerg wants to merge 1 commit into from
+117 −2
Closed

fix(cloudflare): Require a parent span for storage and D1 operations #25860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cloudflare/src/instrumentations/instrumentDurableObjectStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export function instrumentDurableObjectStorage(
{
// Use underscore naming to match Cloudflare's native instrumentation (e.g., "durable_object_storage_get")
name: `durable_object_storage_${methodName}`,
onlyIfParent: true,
attributes: {
[SENTRY_OP]: DB,
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down
1 change: 1 addition & 0 deletions packages/cloudflare/src/instrumentations/instrumentDurableObjectSyncKvStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export function instrumentDurableObjectSyncKvStorage(syncKv: SyncKvStorage): Syn
return startSpan(
{
name: `durable_object_storage_kv_${methodName}`,
onlyIfParent: true,
attributes: {
[SENTRY_OP]: DB,
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down
3 changes: 2 additions & 1 deletion packages/cloudflare/src/instrumentations/instrumentSqlStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function instrumentSqlStorage(sql: SqlStorage): SqlStorage {
// steps. We still start the span so it records its dropped span
// outcome. A query that may target a `cf_` or `pi_` table takes the full path,
// because an internal query must start no span.
return startSpan({ name: 'exec', attributes: SPAN_ATTRIBUTES }, callOriginal);
return startSpan({ name: 'exec', onlyIfParent: true, attributes: SPAN_ATTRIBUTES }, callOriginal);
}

const sanitizedQuery = sanitizeSqlQuery(query);
Expand All @@ -56,6 +56,7 @@ export function instrumentSqlStorage(sql: SqlStorage): SqlStorage {
return startSpan(
{
name: querySummary || sanitizedQuery,
onlyIfParent: true,
attributes: {
...SPAN_ATTRIBUTES,
'db.query.text': sanitizedQuery,
Expand Down
2 changes: 2 additions & 0 deletions packages/cloudflare/src/instrumentations/worker/instrumentD1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ function createStartSpanOptions(queryText: string | undefined, type: D1QueryType

return {
name,
onlyIfParent: true,
attributes: {
[SENTRY_OP]: DB_QUERY,
'db.system.name': 'cloudflare-d1',
Expand Down Expand Up @@ -174,6 +175,7 @@ function instrumentBatch(
return startSpan(
{
name: 'D1 batch',
onlyIfParent: true,
attributes: {
[SENTRY_OP]: DB_QUERY,
'db.system.name': 'cloudflare-d1',
Expand Down
1 change: 1 addition & 0 deletions packages/cloudflare/src/instrumentations/worker/instrumentKV.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ function createSpanOptions(bindingName: string, operation: KVOperation, args: un
const keys = getCacheKeys(operation, args);
return {
name: cacheOperation,
onlyIfParent: true,
attributes: {
[SENTRY_OP]: cacheOperation,
[CACHE_OPERATION]: cacheOperationName,
Expand Down
24 changes: 24 additions & 0 deletions packages/cloudflare/test/instrumentDurableObjectStorage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_get',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -51,6 +52,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_get',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -74,6 +76,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_put',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -95,6 +98,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_put',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -118,6 +122,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_delete',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -139,6 +144,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_delete',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -162,6 +168,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_list',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -185,6 +192,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_setAlarm',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down Expand Up @@ -269,6 +277,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_getAlarm',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -290,6 +299,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_deleteAlarm',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand All @@ -313,6 +323,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'SELECT',
onlyIfParent: true,
attributes: {
'sentry.op': 'db.query',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object_sql',
Expand All @@ -327,6 +338,18 @@ describe('instrumentDurableObjectStorage', () => {
);
});

it('only creates spans when there is an active parent span', async () => {
const client = initTestClient({ tracesSampleRate: 1 });
const startedSpans: string[] = [];
client.on('spanStart', span => startedSpans.push(sentryCore.spanToJSON(span).name));
const instrumented = instrumentDurableObjectStorage(createMockStorage());

await instrumented.get('outside');
await sentryCore.startSpan({ name: 'parent' }, () => instrumented.get('inside'));

expect(startedSpans).toEqual(['parent', 'durable_object_storage_get']);
});

describe('framework-internal KV keys', () => {
it('does not create a span for a cf_-prefixed get', async () => {
const startSpanSpy = vi.spyOn(sentryCore, 'startSpan');
Expand Down Expand Up @@ -437,6 +460,7 @@ describe('instrumentDurableObjectStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_kv_get',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down
18 changes: 18 additions & 0 deletions packages/cloudflare/test/instrumentDurableObjectSyncKvStorage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ import { SENTRY_ORIGIN } from '@sentry/conventions/attributes';
import * as sentryCore from '@sentry/core';
import { afterEach, describe, expect, it, vi } from 'vitest';
import { instrumentDurableObjectSyncKvStorage } from '../src/instrumentations/instrumentDurableObjectSyncKvStorage';
import { initTestClient, resetSdk } from './testUtils';

describe('instrumentDurableObjectSyncKvStorage', () => {
afterEach(() => {
vi.restoreAllMocks();
resetSdk();
});

describe('get', () => {
Expand All @@ -19,6 +21,7 @@ describe('instrumentDurableObjectSyncKvStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_kv_get',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down Expand Up @@ -62,6 +65,7 @@ describe('instrumentDurableObjectSyncKvStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_kv_put',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down Expand Up @@ -94,6 +98,7 @@ describe('instrumentDurableObjectSyncKvStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_kv_delete',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down Expand Up @@ -127,6 +132,7 @@ describe('instrumentDurableObjectSyncKvStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'durable_object_storage_kv_list',
onlyIfParent: true,
attributes: {
'sentry.op': 'db',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object',
Expand Down Expand Up @@ -162,6 +168,18 @@ describe('instrumentDurableObjectSyncKvStorage', () => {
});
});

it('only creates spans when there is an active parent span', () => {
const client = initTestClient({ tracesSampleRate: 1 });
const startedSpans: string[] = [];
client.on('spanStart', span => startedSpans.push(sentryCore.spanToJSON(span).name));
const instrumented = instrumentDurableObjectSyncKvStorage(createMockSyncKv());

instrumented.get('outside');
sentryCore.startSpan({ name: 'parent' }, () => instrumented.get('inside'));

expect(startedSpans).toEqual(['parent', 'durable_object_storage_kv_get']);
});

describe('non-instrumented properties', () => {
it('passes through unknown properties without instrumentation', () => {
const startSpanSpy = vi.spyOn(sentryCore, 'startSpan');
Expand Down
16 changes: 16 additions & 0 deletions packages/cloudflare/test/instrumentSqlStorage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe('instrumentSqlStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'SELECT users',
onlyIfParent: true,
attributes: {
'sentry.op': 'db.query',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object_sql',
Expand All @@ -49,6 +50,7 @@ describe('instrumentSqlStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'SELECT users',
onlyIfParent: true,
attributes: {
'sentry.op': 'db.query',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object_sql',
Expand Down Expand Up @@ -82,6 +84,7 @@ describe('instrumentSqlStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'INSERT users',
onlyIfParent: true,
attributes: {
'sentry.op': 'db.query',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object_sql',
Expand Down Expand Up @@ -152,6 +155,18 @@ describe('instrumentSqlStorage', () => {
expect(mockSql.exec).toHaveBeenCalledTimes(2);
});

it('only creates spans when there is an active parent span', () => {
const client = initTestClient({ tracesSampleRate: 1 });
const startedSpans: string[] = [];
client.on('spanStart', span => startedSpans.push(sentryCore.spanToJSON(span).name));
const instrumented = instrumentSqlStorage(createMockSqlStorage());

instrumented.exec('SELECT * FROM users');
sentryCore.startSpan({ name: 'parent' }, () => instrumented.exec('SELECT * FROM users'));

expect(startedSpans).toEqual(['parent', 'SELECT users']);
});

describe('internal storage queries', () => {
it('does not create a span for Cloudflare-internal queries', () => {
const startSpanSpy = vi.spyOn(sentryCore, 'startSpan');
Expand Down Expand Up @@ -302,6 +317,7 @@ describe('instrumentSqlStorage', () => {
expect(startSpanSpy).toHaveBeenCalledWith(
{
name: 'exec',
onlyIfParent: true,
attributes: {
'sentry.op': 'db.query',
[SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object_sql',
Expand Down
28 changes: 28 additions & 0 deletions packages/cloudflare/test/instrumentations/worker/instrumentD1.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { D1Database, D1DatabaseSession, D1PreparedStatement } from '@cloudf
import * as SentryCore from '@sentry/core';
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest';
import { instrumentD1 } from '../../../src/instrumentations/worker/instrumentD1';
import { initTestClient, resetSdk } from '../../testUtils';

const MOCK_FIRST_RETURN_VALUE = { id: 1, name: 'Foo' };

Expand Down Expand Up @@ -57,6 +58,10 @@ describe('instrumentD1', () => {
vi.clearAllMocks();
});

afterEach(() => {
resetSdk();
});

const startSpanSpy = vi.spyOn(SentryCore, 'startSpan');
const addBreadcrumbSpy = vi.spyOn(SentryCore, 'addBreadcrumb');

Expand All @@ -83,6 +88,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'SELECT * FROM users',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -185,6 +191,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'INSERT INTO users (name) VALUES (?)',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -239,6 +246,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'INSERT INTO users (name) VALUES (?)',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -293,6 +301,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'SELECT * FROM users',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -344,6 +353,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'D1 batch',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -383,6 +393,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'D1 batch',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -412,6 +423,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'CREATE TABLE users (id INTEGER PRIMARY KEY)',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -450,6 +462,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'SELECT * FROM users',
onlyIfParent: true,
},
expect.any(Function),
);
Expand Down Expand Up @@ -487,6 +500,7 @@ describe('instrumentD1', () => {
'sentry.origin': 'auto.db.cloudflare.d1',
},
name: 'D1 batch',
onlyIfParent: true,
},
expect.any(Function),
);
Expand All @@ -508,6 +522,20 @@ describe('instrumentD1', () => {
});
});

test('only creates spans when there is an active parent span', async () => {
const client = initTestClient({ tracesSampleRate: 1 });
const startedSpans: string[] = [];
client.on('spanStart', span => startedSpans.push(SentryCore.spanToJSON(span).name));
const instrumentedDb = instrumentD1(createMockD1Database());

await instrumentedDb.prepare('SELECT * FROM users').first();
await instrumentedDb.batch([instrumentedDb.prepare('SELECT 1')]);
await instrumentedDb.exec('SELECT 1');
await SentryCore.startSpan({ name: 'parent' }, () => instrumentedDb.prepare('SELECT * FROM users').first());

expect(startedSpans).toEqual(['parent', 'SELECT users']);
});

describe('double instrumentation prevention', () => {
test('does not double-instrument the same database', async () => {
const db = createMockD1Database();
Expand Down
Loading
Loading