Skip to content
Merged
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 @@ -7,8 +7,8 @@ const expressPort = 3030;
*/
const config = {
testDir: './tests',
/* Maximum time one test can run for. */
timeout: 150_000,
/* Maximum time one test can run for. Spans take ~2min to become queryable via the trace endpoint. */
timeout: 210_000,
expect: {
/**
* Maximum time expect() should wait for the condition to be met.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
import * as Sentry from '@sentry/node';

let lastTransactionId: string | undefined;
let lastTransactionTraceId: string | undefined;
let lastErrorTraceId: string | undefined;

Sentry.init({
traceLifecycle: 'static',
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
includeLocalVariables: true,
tracesSampleRate: 1,
beforeSend(event) {
lastErrorTraceId = event.contexts?.trace?.trace_id;
return event;
},
beforeSendTransaction(event) {
lastTransactionId = event.event_id;
lastTransactionTraceId = event.contexts?.trace?.trace_id;
return event;
},
});
Expand Down Expand Up @@ -37,6 +44,7 @@ app.get('/test-transaction', function (req, res) {

res.send({
transactionId: lastTransactionId,
traceId: lastTransactionTraceId,
});
});
});
Expand All @@ -46,7 +54,7 @@ app.get('/test-error', async function (req, res) {

await Sentry.flush(2000);

res.send({ exceptionId });
res.send({ exceptionId, traceId: lastErrorTraceId });
});

app.get('/test-exception/:id', function (req, _res) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,45 +1,22 @@
import { expect, test } from '@playwright/test';

const EVENT_POLLING_TIMEOUT = 90_000;

const authToken = process.env.E2E_TEST_AUTH_TOKEN;
const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG;
const sentryTestProject = process.env.E2E_TEST_SENTRY_PROJECT;
import { EVENT_POLLING_OPTIONS, findErrorInTrace, findTransactionInTrace } from './utils/sentry-api';

test('Sends exception to Sentry', async ({ baseURL }) => {
const response = await fetch(`${baseURL}/test-error`);
const { exceptionId } = await response.json();

const url = `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${exceptionId}/`;
const { exceptionId, traceId } = await response.json();

console.log(`Polling for error eventId: ${exceptionId}`);
console.log(`Polling for error eventId: ${exceptionId} in trace: ${traceId}`);

await expect
.poll(
async () => {
const response = await fetch(url, { headers: { Authorization: `Bearer ${authToken}` } });
return response.status;
},
{ timeout: EVENT_POLLING_TIMEOUT },
)
.toBe(200);
await expect.poll(() => findErrorInTrace(traceId, exceptionId), EVENT_POLLING_OPTIONS).toBeDefined();
});

test('Sends transaction to Sentry', async ({ baseURL }) => {
const response = await fetch(`${baseURL}/test-transaction`);
const { transactionId } = await response.json();

const url = `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionId}/`;
const { transactionId, traceId } = await response.json();

console.log(`Polling for transaction eventId: ${transactionId}`);
console.log(`Polling for transaction eventId: ${transactionId} in trace: ${traceId}`);

await expect
.poll(
async () => {
const response = await fetch(url, { headers: { Authorization: `Bearer ${authToken}` } });
return response.status;
},
{ timeout: EVENT_POLLING_TIMEOUT },
)
.toBe(200);
.poll(() => findTransactionInTrace(traceId, transactionId), EVENT_POLLING_OPTIONS)
.toMatchObject({ op: 'e2e-test' });
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
const authToken = process.env.E2E_TEST_AUTH_TOKEN;
const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG;

/**
* Spans only become queryable once they have made it through to EAP, which takes
* noticeably longer than the error pipeline (~2min vs ~20s when this was measured).
*/
export const EVENT_POLLING_OPTIONS = { timeout: 180_000, intervals: [5_000] };

/**
* A node of the span tree returned by the organization trace endpoint. Spans, errors and
* occurrences all share this shape and are discriminated by `event_type`.
*/
export interface TraceItem {
event_id?: string;
/** On spans this is the event id of the transaction the span belongs to. */
transaction_id?: string;
event_type?: 'span' | 'error' | 'occurrence' | 'uptime_check';
op?: string;
is_transaction?: boolean;
children?: TraceItem[];
errors?: TraceItem[];
occurrences?: TraceItem[];
}

export async function fetchTrace(traceId: string): Promise<TraceItem[]> {
const response = await fetch(
`https://sentry.io/api/0/organizations/${sentryTestOrgSlug}/trace/${traceId}/?statsPeriod=1h`,
{ headers: { Authorization: `Bearer ${authToken}` } },
);

// The trace endpoint is org scoped, so the auth token needs `org:read` on top of the
// project scopes the other assertions rely on. That never resolves by waiting, so fail
// loudly instead of polling until the timeout and reporting it as a missing event.
if (response.status === 401 || response.status === 403) {
throw new Error(
`Trace lookup for ${traceId} was rejected with ${response.status}: ${await response.text()}. ` +
'E2E_TEST_AUTH_TOKEN needs the `org:read` scope.',
);
}

// Empty traces and the occasional rate limit are expected while polling, so treat anything
// else that is not a success as "not there yet" -- but log it, since a rejected request and
// a trace that has not landed are otherwise indistinguishable.
if (!response.ok) {
console.log(`Trace lookup for ${traceId} returned ${response.status}: ${await response.text()}`);
return [];
}

return await response.json();
}

/**
* Errors attach to whichever span was active when they were captured, and relocate from the
* top level into that span once it lands, so a given event can surface at any depth.
*/
export function flattenTrace(items: TraceItem[]): TraceItem[] {
return items.flatMap(item => [
item,
...flattenTrace(item.children ?? []),
...flattenTrace(item.errors ?? []),
...flattenTrace(item.occurrences ?? []),
]);
}

export async function findErrorInTrace(traceId: string, eventId: string): Promise<TraceItem | undefined> {
return flattenTrace(await fetchTrace(traceId)).find(item => item.event_type === 'error' && item.event_id === eventId);
}

export async function findTransactionInTrace(traceId: string, eventId: string): Promise<TraceItem | undefined> {
return flattenTrace(await fetchTrace(traceId)).find(item => item.is_transaction && item.transaction_id === eventId);
}
Comment on lines +70 to +72

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 findTransactionInTrace function incorrectly uses transaction_id instead of event_id to find a transaction node, which will cause the lookup to always fail.
Severity: MEDIUM

Suggested Fix

In the findTransactionInTrace function, change the condition to find the transaction by its event_id. Modify the find call from item => item.is_transaction && item.transaction_id === eventId to item => item.is_transaction && item.event_id === eventId.

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/e2e-tests/test-applications/node-express-send-to-sentry/tests/utils/sentry-api.ts#L70-L72

Potential issue: The `findTransactionInTrace` function attempts to locate a transaction
within a trace by matching `item.is_transaction && item.transaction_id === eventId`.
However, according to Sentry's data model and the interface comments, the
`transaction_id` field is present on child spans to reference their parent transaction,
not on the transaction event itself. The transaction's own unique identifier is
`event_id`. Because the function is looking for a transaction node (`is_transaction:
true`) using the wrong field (`transaction_id`), the condition will never be met. This
will cause e2e tests that poll for transactions using this utility to time out and fail.

Also affects:

  • dev-packages/e2e-tests/test-applications/react-send-to-sentry/tests/utils/sentry-api.ts:70~72

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

Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import { devices } from '@playwright/test';
*/
const config = {
testDir: './tests',
/* Maximum time one test can run for. */
timeout: 150_000,
/* Maximum time one test can run for. Spans take ~2min to become queryable via the trace endpoint. */
timeout: 210_000,
expect: {
/**
* Maximum time expect() should wait for the condition to be met.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
interface RecordedEvent {
eventId: string;
traceId: string;
op?: string;
}

interface Window {
recordedTransactions?: string[];
capturedExceptionId?: string;
recordedTransactions?: RecordedEvent[];
capturedException?: RecordedEvent;
sentryReplayId?: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,22 @@ Object.defineProperty(window, 'sentryReplayId', {
},
});

// The trace id is recorded alongside the event id because events are looked up through the
// organization trace endpoint, which is keyed by trace rather than by event.
Sentry.addEventProcessor(event => {
if (
event.type === 'transaction' &&
(event.contexts?.trace?.op === 'pageload' || event.contexts?.trace?.op === 'navigation')
) {
const eventId = event.event_id;
if (eventId) {
window.recordedTransactions = window.recordedTransactions || [];
window.recordedTransactions.push(eventId);
}
const eventId = event.event_id;
const traceId = event.contexts?.trace?.trace_id;
const op = event.contexts?.trace?.op;

if (!eventId || !traceId) {
return event;
}

if (event.type === 'transaction' && (op === 'pageload' || op === 'navigation')) {
window.recordedTransactions = window.recordedTransactions || [];
window.recordedTransactions.push({ eventId, traceId, op });
} else if (!event.type && event.exception) {
window.capturedException = { eventId, traceId };
}

return event;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ const Index = () => {
value="Capture Exception"
id="exception-button"
onClick={() => {
const eventId = Sentry.captureException(new Error('I am an error!'));
window.capturedExceptionId = eventId;
Sentry.captureException(new Error('I am an error!'));
}}
/>
<Link to="/user/5" id="navigation">
Expand Down
Loading
Loading