Skip to content

fix(browser): Fix missing traces for user feedback #19660

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

Merged
s1gr1d merged 5 commits into from
Mar 9, 2026
Merged

fix(browser): Fix missing traces for user feedback #19660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1389a04
fix(browser): Fix missing traces for user feedback
andreiborza Mar 5, 2026
a5c830b
guard profiling data attachment; add tests
s1gr1d Mar 6, 2026
ea6aefc
wait for text
s1gr1d Mar 6, 2026
68b967a
add context
s1gr1d Mar 6, 2026
ada3e88
fix bundle tests
s1gr1d Mar 9, 2026
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
21 changes: 21 additions & 0 deletions dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as Sentry from '@sentry/browser';
// Import these separately so that generatePlugin can handle them for CDN scenarios
// eslint-disable-next-line import/no-duplicates
import { browserProfilingIntegration } from '@sentry/browser';
// eslint-disable-next-line import/no-duplicates
import { feedbackIntegration } from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
integrations: [
Sentry.browserTracingIntegration(),
browserProfilingIntegration(),
feedbackIntegration({
tags: { from: 'integration init' },
}),
],
tracesSampleRate: 1.0,
profilesSampleRate: 1,
});
7 changes: 7 additions & 0 deletions ...ages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!doctype html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body></body>
</html>
66 changes: 66 additions & 0 deletions dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { expect } from '@playwright/test';
import { sentryTest } from '../../../utils/fixtures';
import {
envelopeRequestParser,
getEnvelopeType,
shouldSkipFeedbackTest,
shouldSkipTracingTest,
waitForTransactionRequest,
} from '../../../utils/helpers';

sentryTest(
'feedback should have trace_id when profiling is enabled and idle span has ended',
async ({ getLocalTestUrl, page, browserName }) => {
if (shouldSkipFeedbackTest() || shouldSkipTracingTest() || browserName !== 'chromium') {
sentryTest.skip();
}

const url = await getLocalTestUrl({
testDir: __dirname,
handleLazyLoadedFeedback: true,
responseHeaders: { 'Document-Policy': 'js-profiling' },
});

// Wait for the pageload transaction to be sent (idle span ended)
const pageloadRequestPromise = waitForTransactionRequest(page, event => event.contexts?.trace?.op === 'pageload');

const feedbackRequestPromise = page.waitForResponse(res => {
const req = res.request();
const postData = req.postData();
if (!postData) {
return false;
}
try {
return getEnvelopeType(req) === 'feedback';
} catch {
return false;
}
});

await page.goto(url);

// Wait for the idle page load span to finish
const pageLoadEvent = envelopeRequestParser(await pageloadRequestPromise);

// Submit feedback after idle span ended — no active span
await page.getByText('Report a Bug').waitFor({ state: 'visible' });
await page.getByText('Report a Bug').click();
await page.locator('[name="name"]').fill('Jane Doe');
await page.locator('[name="email"]').fill('janedoe@example.org');
await page.locator('[name="message"]').fill('feedback after idle span ended');
await page.locator('[data-sentry-feedback] .btn--primary').click();

const feedbackEvent = envelopeRequestParser((await feedbackRequestPromise).request());

expect(feedbackEvent.contexts?.trace?.trace_id).toMatch(/\w{32}/);
expect(feedbackEvent.contexts?.trace?.span_id).toMatch(/\w{16}/);
Copy link

@cursor cursor bot Mar 6, 2026

Choose a reason for hiding this comment

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

Test uses weak unanchored regex for trace assertions

Low Severity

The assertions for trace_id and span_id use /\w{32}/ and /\w{16}/ — unanchored regexes with \w (which matches underscores and uppercase letters). This is inconsistent with the stricter /^[a-f\d]{32}$/ used on line 62 in the same test. Since these are the core assertions validating the fix (that feedback events get proper trace context), they don't thoroughly verify the newly added behavior. A string longer than 32 characters or containing non-hex characters would still pass.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot


// contexts.trace.data must include thread.id to identify which thread is associated with the transaction
expect(pageLoadEvent.contexts?.trace?.data?.['thread.id']).toBe('0');
expect(pageLoadEvent.contexts?.trace?.data?.['thread.name']).toBe('main');

// fixme: figure out why profiler_id is set on feedback and not on pageload transaction
// todo: rename to `profiler_id`
expect(feedbackEvent.contexts?.profile?.profile_id).toMatch(/^[a-f\d]{32}$/);
},
);
6 changes: 6 additions & 0 deletions dev-packages/browser-integration-tests/suites/profiling/legacyMode/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ sentryTest('sends profile envelope in legacy mode', async ({ page, getLocalTestU
const url = await getLocalTestUrl({ testDir: __dirname, responseHeaders: { 'Document-Policy': 'js-profiling' } });

const req = await waitForTransactionRequestOnUrl(page, url);
const transactionEvent = properEnvelopeRequestParser<Event>(req, 0);
const profileEvent = properEnvelopeRequestParser<Profile>(req, 1);

expect(profileEvent).toBeDefined();

const profile = profileEvent.profile;
Expand All @@ -54,4 +56,8 @@ sentryTest('sends profile envelope in legacy mode', async ({ page, getLocalTestU
minSampleDurationMs: 20,
isChunkFormat: false,
});

// contexts.trace.data must include thread.id to identify which thread is associated with the transaction
expect(transactionEvent?.contexts?.trace?.data?.['thread.id']).toBe('0');
expect(transactionEvent?.contexts?.trace?.data?.['thread.name']).toBe('main');
});
26 changes: 25 additions & 1 deletion ...ges/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { expect } from '@playwright/test';
import type { ProfileChunkEnvelope } from '@sentry/core';
import type { Event, ProfileChunkEnvelope } from '@sentry/core';
import { sentryTest } from '../../../utils/fixtures';
import {
countEnvelopes,
getMultipleSentryEnvelopeRequests,
properEnvelopeRequestParser,
properFullEnvelopeRequestParser,
shouldSkipTracingTest,
waitForTransactionRequestOnUrl,
} from '../../../utils/helpers';
import { validateProfile, validateProfilePayloadMetadata } from '../test-utils';

Expand Down Expand Up @@ -96,3 +98,25 @@ sentryTest(
});
},
);

sentryTest(
'attaches profiler_id and thread data to transaction events (trace mode)',
async ({ page, getLocalTestUrl, browserName }) => {
if (shouldSkipTracingTest() || browserName !== 'chromium') {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname, responseHeaders: { 'Document-Policy': 'js-profiling' } });

const req = await waitForTransactionRequestOnUrl(page, url);
const transactionEvent = properEnvelopeRequestParser<Event>(req, 0) as any;

expect(transactionEvent?.type).toBe('transaction');

expect(transactionEvent?.contexts?.profile?.profiler_id).toMatch(/^[a-f\d]{32}$/);

// contexts.trace.data must include thread.id to identify which thread is associated with the transaction
expect(transactionEvent?.contexts?.trace?.data?.['thread.id']).toBe('0');
expect(transactionEvent?.contexts?.trace?.data?.['thread.name']).toBe('main');
},
);
5 changes: 4 additions & 1 deletion ...s/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,12 @@ sentryTest('attaches thread data to child spans (trace mode)', async ({ page, ge

const profilerId = rootSpan?.contexts?.profile?.profiler_id as string | undefined;
expect(typeof profilerId).toBe('string');

expect(profilerId).toMatch(/^[a-f\d]{32}$/);

// contexts.trace.data must include thread.id to identify which thread is associated with the transaction
expect(rootSpan?.contexts?.trace?.data?.['thread.id']).toBe('0');
expect(rootSpan?.contexts?.trace?.data?.['thread.name']).toBe('main');

const spans = (rootSpan?.spans ?? []) as Array<{ data?: Record<string, unknown> }>;
expect(spans.length).toBeGreaterThan(0);
for (const span of spans) {
Expand Down
22 changes: 12 additions & 10 deletions packages/browser/src/profiling/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,16 +798,18 @@ export function attachProfiledThreadToEvent(event: Event): Event {
return event;
}

// @ts-expect-error the trace fallback value is wrong, though it should never happen
// and in case it does, we dont want to override whatever was passed initially.
event.contexts.trace = {
...(event.contexts?.trace ?? {}),
data: {
...(event.contexts?.trace?.data ?? {}),
['thread.id']: PROFILER_THREAD_ID_STRING,
['thread.name']: PROFILER_THREAD_NAME,
},
};
// Only mutate the trace context when it already has a trace_id — that
// guarantees `applySpanToEvent` has already run, and we are not creating a partial trace context from scratch.
if (event.contexts.trace?.trace_id) {
event.contexts.trace = {
...event.contexts.trace,
data: {
...(event.contexts.trace.data ?? {}),
['thread.id']: PROFILER_THREAD_ID_STRING,
['thread.name']: PROFILER_THREAD_NAME,
},
};
}

// Attach thread info to individual spans so that spans can be associated with the profiled thread on the UI even if contexts are missing.
event.spans?.forEach(span => {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,7 +1222,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
this.emit('postprocessEvent', evt, hint);

evt.contexts = {
trace: getTraceContextFromScope(currentScope),
trace: { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) },
Copy link

@cursor cursor bot Mar 6, 2026

Choose a reason for hiding this comment

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

Trace context merge is overwritten by spread order

Medium Severity

The computed merge { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) } on line 1225 is immediately overwritten by ...evt.contexts on line 1226, because later properties in an object literal override earlier ones with the same key. When evt.contexts has a trace property, the spread replaces the merged trace with the original unmerged value, making this change a no-op compared to the old code. If the intent was to ensure scope trace data supplements existing trace context, trace needs to appear after ...evt.contexts in the object literal.

Fix in Cursor Fix in Web

...evt.contexts,
};
Comment on lines 1224 to 1227
Copy link

@sentry sentry bot Mar 6, 2026

Choose a reason for hiding this comment

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

Bug: The trace property in evt.contexts is overwritten by the object spread that follows it, preventing the scope's trace context from being correctly merged.
Severity: CRITICAL

Suggested Fix

To ensure the merged trace context is preserved, move the trace property definition after the spread of evt.contexts. The correct implementation should be: evt.contexts = { ...evt.contexts, trace: { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) } };

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: packages/core/src/client.ts#L1224-L1227

Potential issue: In `_prepareEvent`, the code attempts to merge trace context from the
scope into an event's contexts. However, due to the property order in the object
literal, the `trace` property is defined and then immediately overwritten by the spread
of the original `evt.contexts`. The line `...evt.contexts` comes after the new `trace`
property, causing the original `evt.contexts.trace` to replace the newly merged one.
This negates the intended merge, preventing trace information from the scope from being
added to events that already have a trace context, such as user feedback events.

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


Expand Down
Loading