Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ it('cacheClient: true - detached work events ARE captured', async ({ signal }) =
});

it('cacheClient: false - repro #22545: detached work events are silently dropped', async ({ signal }) => {
const runner = createRunner(__dirname).ignore('transaction', 'span').start(signal);
const runner = createRunner(__dirname).ignore('span').start(signal);

// Make the request that spawns detached work
await runner.makeRequest('get', '/no-cache/detached?id=repro-1');
Expand All @@ -135,7 +135,7 @@ it('cacheClient: true - dedupe drops the same error across invocations', async (
// A shared client shares its dedupe state, so the same error captured by two separate
// invocations is reported only once — the second is dropped as a duplicate.
const runner = createRunner(__dirname)
.ignore('transaction', 'span')
.ignore('span')
.unordered()
.failOnUnexpected()
.expect(errorEventExpectation('Same error', CAPTURE_MECHANISM))
Expand All @@ -159,7 +159,7 @@ it('cacheClient: true - dedupe drops the same error across invocations', async (
it('cacheClient: false - dedupe does not persist across invocations', async ({ signal }) => {
// A fresh client per invocation means fresh dedupe state, so each invocation reports
// the same error independently.
const runner = createRunner(__dirname).ignore('transaction', 'span').start(signal);
const runner = createRunner(__dirname).ignore('span').start(signal);

for (let i = 0; i < 3; i++) {
await runner.makeRequestAndWaitForEnvelope(
Expand All @@ -174,7 +174,7 @@ it('cacheClient: false - dedupe does not persist across invocations', async ({ s
// also start reusing the isolation scope `setTag`/`setUser` write to. The uncached counterpart of
// this test lives in the `durable-object-scope` suite.
it('cacheClient: true - two consecutive invocations get different isolation scopes', async ({ signal }) => {
const runner = createRunner(__dirname).ignore('transaction', 'span').start(signal);
const runner = createRunner(__dirname).ignore('span').start(signal);

await runner.makeRequestAndWaitForEnvelope('get', '/cache/scope?id=scope-shared&seed=1', (envelope: Envelope) => {
const event = envelope[1]?.[0]?.[1] as Event;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ interface Env {
export default Sentry.withSentry(
(env: Env) => ({
dsn: env.SENTRY_DSN,
traceLifecycle: 'static',
tracesSampleRate: 1,
}),
{
Comment on lines 9 to 14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The D1 batch test can fail due to a race condition where parent and child spans arrive in separate envelopes, but the test asserts they are in the same one.
Severity: MEDIUM

Suggested Fix

The test should be updated to handle spans arriving in separate envelopes. Use collectStreamedSpans() to wait for all spans before making assertions, which is the pattern used in the Prisma and Vercel AI tests. Alternatively, restructure the assertions so they do not depend on finding parent and child spans within the same envelope.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: dev-packages/cloudflare-integration-tests/suites/d1/index.ts#L9-L14

Potential issue: The D1 batch integration test is susceptible to a race condition. The
test expects a parent segment span and its child 'D1 batch' span to be present in the
same envelope when using `.unordered()` mode. However, due to the test runner's
streaming nature and timed buffer flushes, these spans can arrive in separate envelopes.
When this occurs, the logic attempting to find the child span fails because it's not in
the same envelope as the parent. Consequently, a later assertion,
`expect(directBatchSpan).toBeDefined()`, fails, leading to flaky test outcomes in CI
environments.

Also affects:

  • dev-packages/cloudflare-integration-tests/suites/d1/test.ts:85~110

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down
193 changes: 89 additions & 104 deletions dev-packages/cloudflare-integration-tests/suites/d1/test.ts
Original file line number Diff line number Diff line change
@@ -1,54 +1,37 @@
import type { Envelope } from '@sentry/core';
import type { Envelope, SerializedStreamedSpan } from '@sentry/core';
import { expect, it } from 'vitest';
import { createRunner } from '../../runner';
import { getSpansFromEnvelope } from '../../spanUtils';

function envelopeItemType(envelope: Envelope): string | undefined {
return envelope[1][0]?.[0]?.type as string | undefined;
}

function envelopeItem(envelope: Envelope): Record<string, unknown> {
return envelope[1][0]![1] as Record<string, unknown>;
}

function findD1Spans(envelope: Envelope): Array<Record<string, unknown>> {
if (envelopeItemType(envelope) !== 'transaction') return [];
const tx = envelopeItem(envelope);
const spans = (tx.spans as Array<Record<string, unknown>>) || [];
return spans.filter(s => (s.op as string) === 'db.query');
}
// `cloudflare.d1.duration` is only an integer when the query happens to take a whole number of
// milliseconds, so the type can't be pinned down.
const NUMBER_ATTRIBUTE = { type: expect.stringMatching(/^(?:integer|double)$/), value: expect.any(Number) };

it('instruments D1 prepare().all() automatically via env', async ({ signal }) => {
const runner = createRunner(__dirname)
.ignore('event')
.expect((envelope: Envelope) => {
expect(envelopeItemType(envelope)).toBe('transaction');
const d1Spans = findD1Spans(envelope);
expect(d1Spans.length).toBeGreaterThanOrEqual(1);

const querySpan = d1Spans.find(s => s.description === 'SELECT * FROM users WHERE id = ?');
expect(querySpan).toBeDefined();
expect(querySpan).toEqual({
data: {
'db.system.name': 'cloudflare-d1',
'db.operation.name': 'all',
'db.query.text': 'SELECT * FROM users WHERE id = ?',
'db.query.summary': 'SELECT users',
'cloudflare.d1.duration': expect.any(Number),
'cloudflare.d1.rows_read': expect.any(Number),
'cloudflare.d1.rows_written': expect.any(Number),
'sentry.op': 'db.query',
'sentry.origin': 'auto.db.cloudflare.d1',
},
description: 'SELECT * FROM users WHERE id = ?',
op: 'db.query',
origin: 'auto.db.cloudflare.d1',
status: 'ok',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.any(String),
});
const spans = getSpansFromEnvelope(envelope);
const segmentSpan = spans.find(span => span.is_segment);

// The D1 span is named after its query summary rather than the full query text.
const querySpan = spans.find(span => span.attributes['db.operation.name']?.value === 'all');
expect(querySpan?.name).toBe('SELECT users');
expect(querySpan?.parent_span_id).toBe(segmentSpan?.span_id);
expect(querySpan?.status).toBe('ok');
expect(querySpan?.attributes).toEqual(
expect.objectContaining({
'sentry.op': { type: 'string', value: 'db.query' },
'sentry.origin': { type: 'string', value: 'auto.db.cloudflare.d1' },
'db.system.name': { type: 'string', value: 'cloudflare-d1' },
'db.operation.name': { type: 'string', value: 'all' },
'db.query.text': { type: 'string', value: 'SELECT * FROM users WHERE id = ?' },
'db.query.summary': { type: 'string', value: 'SELECT users' },
'cloudflare.d1.duration': NUMBER_ATTRIBUTE,
'cloudflare.d1.rows_read': NUMBER_ATTRIBUTE,
'cloudflare.d1.rows_written': NUMBER_ATTRIBUTE,
}),
);
})
.start(signal);

Expand All @@ -58,10 +41,10 @@ it('instruments D1 prepare().all() automatically via env', async ({ signal }) =>

it('captures error event when a D1 query references a non-existent table', async ({ signal }) => {
const runner = createRunner(__dirname)
.ignore('transaction')
.ignore('span')
.expect((envelope: Envelope) => {
expect(envelopeItemType(envelope)).toBe('event');
const event = envelopeItem(envelope);
expect(envelope[1][0]?.[0]?.type).toBe('event');
const event = envelope[1][0]![1] as Record<string, unknown>;
expect(event.level).toBe('error');

const values = (event.exception as { values: Array<Record<string, unknown>> })?.values;
Expand Down Expand Up @@ -102,32 +85,26 @@ it('instruments D1 exec() automatically via env', async ({ signal }) => {
const runner = createRunner(__dirname)
.ignore('event')
.expect((envelope: Envelope) => {
expect(envelopeItemType(envelope)).toBe('transaction');
const d1Spans = findD1Spans(envelope);

const execSpan = d1Spans.find(
s => s.description === 'CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)',
const spans = getSpansFromEnvelope(envelope);
const segmentSpan = spans.find(span => span.is_segment);

const execSpan = spans.find(span => span.attributes['db.operation.name']?.value === 'exec');
expect(execSpan?.name).toBe('CREATE TABLE users');
expect(execSpan?.parent_span_id).toBe(segmentSpan?.span_id);
expect(execSpan?.status).toBe('ok');
expect(execSpan?.attributes).toEqual(
expect.objectContaining({
'sentry.op': { type: 'string', value: 'db.query' },
'sentry.origin': { type: 'string', value: 'auto.db.cloudflare.d1' },
'db.system.name': { type: 'string', value: 'cloudflare-d1' },
'db.operation.name': { type: 'string', value: 'exec' },
Comment on lines +99 to +100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: D1 integration tests will fail because they assert parent-child span relationships in a single envelope, which is not guaranteed with streaming span emission.
Severity: MEDIUM

Suggested Fix

Update the D1 integration tests to handle spans arriving across multiple, separate envelopes. Use a utility like collectStreamedSpans() to accumulate all spans from the test run before performing assertions that rely on relationships between different spans.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: dev-packages/cloudflare-integration-tests/suites/d1/test.ts#L99-L100

Potential issue: The removal of `traceLifecycle: 'static'` enables streaming span
emission for D1 integration tests. However, the tests for `prepare().all()`, `exec()`,
and `batch()` assert parent-child span relationships within a single-envelope callback,
for example `expect(querySpan?.parent_span_id).toBe(segmentSpan?.span_id)`. With
streaming, the parent `segmentSpan` and child `querySpan` can arrive in separate
envelopes. When this happens, one of the spans will be `undefined` during the check,
causing the test assertion to fail and making the test suite flaky.

Also affects:

  • dev-packages/cloudflare-integration-tests/suites/d1/test.ts:129~130

'db.query.text': {
type: 'string',
value: 'CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)',
},
'db.query.summary': { type: 'string', value: 'CREATE TABLE users' },
}),
);
expect(execSpan).toBeDefined();
expect(execSpan).toEqual({
data: {
'db.system.name': 'cloudflare-d1',
'db.operation.name': 'exec',
'db.query.text': 'CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)',
'db.query.summary': 'CREATE TABLE users',
'sentry.op': 'db.query',
'sentry.origin': 'auto.db.cloudflare.d1',
},
description: 'CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)',
op: 'db.query',
origin: 'auto.db.cloudflare.d1',
status: 'ok',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.any(String),
});
})
.start(signal);

Expand All @@ -136,20 +113,30 @@ it('instruments D1 exec() automatically via env', async ({ signal }) => {
});

it('instruments D1 withSession().batch() identically to db.batch()', async ({ signal }) => {
let directBatchSpan: Record<string, unknown> | undefined;
let sessionBatchSpan: Record<string, unknown> | undefined;
let directBatchSpan: SerializedStreamedSpan | undefined;
let sessionBatchSpan: SerializedStreamedSpan | undefined;

const runner = createRunner(__dirname)
.ignore('event')
.expect((envelope: Envelope) => {
expect(envelopeItem(envelope).transaction).toBe('GET /batch');
const spans = getSpansFromEnvelope(envelope);
// Both routes are raw URLs, so the streamed segment name keeps the method only and the
// request is identified through `url.path`.
expect(spans.find(span => span.is_segment)?.attributes['url.path']).toEqual({
type: 'string',
value: '/batch',
});

directBatchSpan = findD1Spans(envelope).find(s => s.description === 'D1 batch');
directBatchSpan = spans.find(span => span.name === 'D1 batch');
})
.expect((envelope: Envelope) => {
expect(envelopeItem(envelope).transaction).toBe('GET /with-session/batch');
const spans = getSpansFromEnvelope(envelope);
expect(spans.find(span => span.is_segment)?.attributes['url.path']).toEqual({
type: 'string',
value: '/with-session/batch',
});

sessionBatchSpan = findD1Spans(envelope).find(s => s.description === 'D1 batch');
sessionBatchSpan = spans.find(span => span.name === 'D1 batch');
})
.unordered()
.start(signal);
Expand All @@ -161,16 +148,19 @@ it('instruments D1 withSession().batch() identically to db.batch()', async ({ si
expect(directBatchSpan).toBeDefined();
expect(sessionBatchSpan).toBeDefined();

const normalize = (span: Record<string, unknown>): Record<string, unknown> => {
// Ids and timestamps differ between the two requests, everything else must match.
const normalize = (span: SerializedStreamedSpan): Record<string, unknown> => {
const {
span_id: _spanId,
parent_span_id: _parentSpanId,
start_timestamp: _start,
timestamp: _end,
end_timestamp: _end,
trace_id: _traceId,
attributes,
...rest
} = span;
return rest;
const { 'sentry.segment.id': _segmentId, ...restAttributes } = attributes;
return { ...rest, attributes: restAttributes };
};

expect(normalize(sessionBatchSpan!)).toEqual(normalize(directBatchSpan!));
Expand All @@ -180,30 +170,25 @@ it('instruments D1 batch() automatically via env', async ({ signal }) => {
const runner = createRunner(__dirname)
.ignore('event')
.expect((envelope: Envelope) => {
expect(envelopeItemType(envelope)).toBe('transaction');
const d1Spans = findD1Spans(envelope);

const batchSpan = d1Spans.find(s => s.description === 'D1 batch');
expect(batchSpan).toBeDefined();
expect(batchSpan).toEqual({
data: {
'db.system.name': 'cloudflare-d1',
'db.operation.name': 'batch',
'db.query.text': 'INSERT INTO users (name) VALUES (?)\nINSERT INTO users (name) VALUES (?)',
'db.operation.batch.size': 2,
'sentry.op': 'db.query',
'sentry.origin': 'auto.db.cloudflare.d1',
},
description: 'D1 batch',
op: 'db.query',
origin: 'auto.db.cloudflare.d1',
status: 'ok',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.any(String),
});
const spans = getSpansFromEnvelope(envelope);
const segmentSpan = spans.find(span => span.is_segment);

const batchSpan = spans.find(span => span.name === 'D1 batch');
expect(batchSpan?.parent_span_id).toBe(segmentSpan?.span_id);
expect(batchSpan?.status).toBe('ok');
expect(batchSpan?.attributes).toEqual(
expect.objectContaining({
'sentry.op': { type: 'string', value: 'db.query' },
'sentry.origin': { type: 'string', value: 'auto.db.cloudflare.d1' },
'db.system.name': { type: 'string', value: 'cloudflare-d1' },
'db.operation.name': { type: 'string', value: 'batch' },
'db.query.text': {
type: 'string',
value: 'INSERT INTO users (name) VALUES (?)\nINSERT INTO users (name) VALUES (?)',
},
'db.operation.batch.size': { type: 'integer', value: 2 },
}),
);
})
.start(signal);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { expect, it } from 'vitest';
import { createRunner } from '../../runner';

it('two consecutive invocations get different isolation scopes', async ({ signal }) => {
const runner = createRunner(__dirname).ignore('transaction', 'span').start(signal);
const runner = createRunner(__dirname).ignore('span').start(signal);

await runner.makeRequestAndWaitForEnvelope('get', '/scope?seed=1', (envelope: Envelope) => {
const event = envelope[1]?.[0]?.[1] as Event;
Expand All @@ -23,7 +23,7 @@ it('two consecutive invocations get different isolation scopes', async ({ signal
});

it('a nested direct call within one invocation shares the same isolation scope', async ({ signal }) => {
const runner = createRunner(__dirname).ignore('transaction', 'span').start(signal);
const runner = createRunner(__dirname).ignore('span').start(signal);

await runner.makeRequestAndWaitForEnvelope('get', '/nested', (envelope: Envelope) => {
const event = envelope[1]?.[0]?.[1] as Event;
Expand Down Expand Up @@ -51,7 +51,7 @@ it('a nested direct call within one invocation shares the same isolation scope',
});

it('a nested call into another instrumented handler shares the same isolation scope', async ({ signal }) => {
const runner = createRunner(__dirname).ignore('transaction', 'span').start(signal);
const runner = createRunner(__dirname).ignore('span').start(signal);

await runner.makeRequestAndWaitForEnvelope('get', '/reentrant', (envelope: Envelope) => {
const event = envelope[1]?.[0]?.[1] as Event;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ class TestDurableObjectBase extends DurableObject<Env> {
export const TestDurableObject = Sentry.instrumentDurableObjectWithSentry(
(env: Env) => ({
dsn: env.SENTRY_DSN,
traceLifecycle: 'static',
tracesSampleRate: 1.0,
}),
TestDurableObjectBase,
Expand All @@ -28,7 +27,6 @@ export const TestDurableObject = Sentry.instrumentDurableObjectWithSentry(
export default Sentry.withSentry(
(env: Env) => ({
dsn: env.SENTRY_DSN,
traceLifecycle: 'static',
tracesSampleRate: 1.0,
}),
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ interface Env {
export default Sentry.withSentry(
(env: Env) => ({
dsn: env.SENTRY_DSN,
traceLifecycle: 'static',
tracesSampleRate: 1,
integrations: [Sentry.prismaIntegration()],
}),
Expand Down
Loading
Loading