Skip to content
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

fix(utils): Fix envelope parsing edge cases #13191

Closed
wants to merge 18 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ sentryTest(
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: traceId,
type: 'span',
Copy link
Member

Choose a reason for hiding this comment

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

h: We should not add this field to standalone span envelopes.

});

expect(transactionEnvelopeItem).toEqual({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ sentryTest('sends a segment span envelope', async ({ getLocalTestPath, page }) =
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.stringMatching(/^[0-9a-f]{32}$/),
type: 'span',
is_segment: true,
segment_id: spanJson.span_id,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,5 +95,6 @@ sentryTest('should capture an INP click event span after pageload', async ({ bro
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: traceId,
type: 'span',
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ sentryTest(
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: traceId,
type: 'span',
});
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ sentryTest(
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: traceId,
type: 'span',
});
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ sentryTest(
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: traceId,
type: 'span',
});
},
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ sentryTest('error after navigation has navigation traceId', async ({ getLocalTes
await page.locator('#errorBtn').click();
const [errorEvent, errorTraceHeader] = await errorEventPromise;

expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');
Copy link
Member

Choose a reason for hiding this comment

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

h: let's not change this please unless there's a good reason to (is there?). So far, sending error events without a type worked well.


const errorTraceContext = errorEvent.contexts?.trace;
expect(errorTraceContext).toEqual({
Expand Down Expand Up @@ -157,10 +157,10 @@ sentryTest('error during navigation has new navigation traceId', async ({ getLoc
const envelopes = await envelopeRequestsPromise;

const [navigationEvent, navigationTraceHeader] = envelopes.find(envelope => envelope[0].type === 'transaction')!;
const [errorEvent, errorTraceHeader] = envelopes.find(envelope => !envelope[0].type)!;
const [errorEvent, errorTraceHeader] = envelopes.find(envelope => envelope[0].type !== 'transaction')!;

expect(navigationEvent.type).toEqual('transaction');
expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');

const navigationTraceContext = navigationEvent?.contexts?.trace;
expect(navigationTraceContext).toMatchObject({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ sentryTest('error after <meta> tag pageload has pageload traceId', async ({ getL
await page.locator('#errorBtn').click();
const [errorEvent, errorTraceHeader] = await errorEventPromise;

expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');
expect(errorEvent.contexts?.trace).toEqual({
trace_id: META_TAG_TRACE_ID,
parent_span_id: META_TAG_PARENT_SPAN_ID,
Expand Down Expand Up @@ -153,7 +153,7 @@ sentryTest('error during <meta> tag pageload has pageload traceId', async ({ get
const [pageloadEvent, pageloadTraceHeader] = envelopes.find(
eventAndHeader => eventAndHeader[0].type === 'transaction',
)!;
const [errorEvent, errorTraceHeader] = envelopes.find(eventAndHeader => !eventAndHeader[0].type)!;
const [errorEvent, errorTraceHeader] = envelopes.find(eventAndHeader => eventAndHeader[0].type !== 'transaction')!;

expect(pageloadEvent.type).toEqual('transaction');
expect(pageloadEvent?.contexts?.trace).toMatchObject({
Expand All @@ -173,7 +173,7 @@ sentryTest('error during <meta> tag pageload has pageload traceId', async ({ get
trace_id: META_TAG_TRACE_ID,
});

expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');
expect(errorEvent?.contexts?.trace).toEqual({
trace_id: META_TAG_TRACE_ID,
parent_span_id: META_TAG_PARENT_SPAN_ID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ sentryTest('error after pageload has pageload traceId', async ({ getLocalTestUrl
const [errorEvent, errorTraceHeader] = await errorEventPromise;

const errorTraceContext = errorEvent.contexts?.trace;
expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');

expect(errorTraceContext).toEqual({
trace_id: pageloadTraceContext?.trace_id,
Expand Down Expand Up @@ -145,7 +145,7 @@ sentryTest('error during pageload has pageload traceId', async ({ getLocalTestUr
const [pageloadEvent, pageloadTraceHeader] = envelopes.find(
eventAndHeader => eventAndHeader[0].type === 'transaction',
)!;
const [errorEvent, errorTraceHeader] = envelopes.find(eventAndHeader => !eventAndHeader[0].type)!;
const [errorEvent, errorTraceHeader] = envelopes.find(eventAndHeader => eventAndHeader[0].type !== 'transaction')!;

const pageloadTraceContext = pageloadEvent?.contexts?.trace;

Expand All @@ -167,7 +167,7 @@ sentryTest('error during pageload has pageload traceId', async ({ getLocalTestUr

const errorTraceContext = errorEvent?.contexts?.trace;

expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');
expect(errorTraceContext).toEqual({
trace_id: pageloadTraceContext?.trace_id,
span_id: expect.stringMatching(/^[0-9a-f]{16}$/),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ sentryTest('error has new traceId after navigation', async ({ getLocalTestUrl, p
await page.locator('#errorBtn').click();
const [errorEvent, errorTraceHeader] = await errorEventPromise;

expect(errorEvent.type).toEqual(undefined);
expect(errorEvent.type).toEqual('event');
expect(errorEvent.contexts?.trace).toEqual({
trace_id: META_TAG_TRACE_ID,
parent_span_id: META_TAG_PARENT_SPAN_ID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('sends an error', async ({ page }) => {
const errorPromise = waitForError('angular-17', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/`);
Expand Down Expand Up @@ -35,7 +35,7 @@ test('assigns the correct transaction value after a navigation', async ({ page }
});

const errorPromise = waitForError('angular-17', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('sends an error', async ({ page }) => {
const errorPromise = waitForError('angular-18', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/`);
Expand Down Expand Up @@ -35,7 +35,7 @@ test('assigns the correct transaction value after a navigation', async ({ page }
});

const errorPromise = waitForError('angular-18', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ test('sends an INP span during pageload', async ({ page }) => {
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.any(String),
type: 'span',
origin: 'auto.http.browser.inp',
exclusive_time: expect.any(Number),
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
Expand Down Expand Up @@ -96,6 +97,7 @@ test('sends an INP span after pageload', async ({ page }) => {
exclusive_time: expect.any(Number),
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
segment_id: expect.any(String),
type: 'span',
});
});

Expand Down Expand Up @@ -140,6 +142,7 @@ test('sends an INP span during navigation', async ({ page }) => {
exclusive_time: expect.any(Number),
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
segment_id: expect.any(String),
type: 'span',
});
});

Expand Down Expand Up @@ -194,5 +197,6 @@ test('sends an INP span after navigation', async ({ page }) => {
exclusive_time: expect.any(Number),
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
segment_id: expect.any(String),
type: 'span',
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('sends an error', async ({ page }) => {
const errorPromise = waitForError('ember-classic', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
Copy link
Member

Choose a reason for hiding this comment

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

m: I don't think this change is correct because event.type could be a lot more than 'transaction' or undefined. However, it probably doesn't matter because waitForError already checks for type === undefined before even invoking the callback :)

});

await page.goto(`/`);
Expand Down Expand Up @@ -35,7 +35,7 @@ test('assigns the correct transaction value after a navigation', async ({ page }
});

const errorPromise = waitForError('ember-classic', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/tracing`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('sends an error', async ({ page }) => {
const errorPromise = waitForError('ember-embroider', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/`);
Expand Down Expand Up @@ -35,7 +35,7 @@ test('assigns the correct transaction value after a navigation', async ({ page }
});

const errorPromise = waitForError('ember-embroider', async errorEvent => {
return !errorEvent.type;
return errorEvent.type !== 'transaction';
});

await page.goto(`/tracing`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('Sends exception to Sentry', async ({ baseURL }) => {
const errorEventPromise = waitForError('nestjs', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

const response = await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('Sends unexpected exception to Sentry if thrown in module with global filter', async ({ baseURL }) => {
const errorEventPromise = waitForError('nestjs', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an uncaught exception!';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an uncaught exception!';
});

const response = await fetch(`${baseURL}/example-module/unexpected-exception`);
Expand Down Expand Up @@ -33,7 +33,7 @@ test('Sends unexpected exception to Sentry if thrown in module that was register
baseURL,
}) => {
const errorEventPromise = waitForError('nestjs', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an uncaught exception!';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an uncaught exception!';
});

const response = await fetch(`${baseURL}/example-module-wrong-order/unexpected-exception`);
Expand Down Expand Up @@ -120,7 +120,10 @@ test('Does not handle expected exception if exception is thrown in module regist
baseURL,
}) => {
const errorEventPromise = waitForError('nestjs', event => {
return !event.type && event.exception?.values?.[0]?.value === 'Something went wrong in the example module!';
return (
event.type !== 'transaction' &&
event.exception?.values?.[0]?.value === 'Something went wrong in the example module!'
);
});

const response = await fetch(`${baseURL}/example-module-wrong-order/expected-exception`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-connect', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception';
});

await fetch(`${baseURL}/test-exception`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct context when instrumentation was set up incorrectly', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-express', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-express', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-fastify', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Returns 400 from failed assert', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-koa', event => {
return !event.type && event.exception?.values?.[0]?.value === 'ctx.assert failed';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'ctx.assert failed';
});

const res = await fetch(`${baseURL}/test-assert/false`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-koa', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('Sends exception to Sentry', async ({ baseURL }) => {
const errorEventPromise = waitForError('nestjs', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

const response = await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-otel-custom-sampler', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-otel-sdk-node', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ baseURL }) => {
const errorEventPromise = waitForError('node-otel-without-tracing', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'This is an exception with id 123';
});

await fetch(`${baseURL}/test-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('Sends correct error event', async ({ page }) => {
const errorEventPromise = waitForError('react-17', event => {
return !event.type && event.exception?.values?.[0]?.value === 'I am an error!';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'I am an error!';
});

await page.goto('/');
Expand Down Expand Up @@ -35,7 +35,7 @@ test('Sets correct transactionName', async ({ page }) => {
});

const errorEventPromise = waitForError('react-17', event => {
return !event.type && event.exception?.values?.[0]?.value === 'I am an error!';
return event.type !== 'transaction' && event.exception?.values?.[0]?.value === 'I am an error!';
});

await page.goto('/');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ test('sends an INP span', async ({ page }) => {
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: expect.any(String),
type: 'span',
origin: 'auto.http.browser.inp',
exclusive_time: expect.any(Number),
measurements: { inp: { unit: 'millisecond', value: expect.any(Number) } },
Expand Down
Loading
Loading