diff --git a/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/init.js b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/init.js new file mode 100644 index 000000000000..eafdd7b8605e --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/init.js @@ -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, +}); diff --git a/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/template.html b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/template.html new file mode 100644 index 000000000000..7c26f485c41b --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/template.html @@ -0,0 +1,7 @@ + + + + + + + diff --git a/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/test.ts b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/test.ts new file mode 100644 index 000000000000..9242af676a96 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/test.ts @@ -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}/); + + // 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}$/); + }, +); diff --git a/dev-packages/browser-integration-tests/suites/profiling/legacyMode/test.ts b/dev-packages/browser-integration-tests/suites/profiling/legacyMode/test.ts index 4d8caa3a2be3..9726fd283cf4 100644 --- a/dev-packages/browser-integration-tests/suites/profiling/legacyMode/test.ts +++ b/dev-packages/browser-integration-tests/suites/profiling/legacyMode/test.ts @@ -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(req, 0); const profileEvent = properEnvelopeRequestParser(req, 1); + expect(profileEvent).toBeDefined(); const profile = profileEvent.profile; @@ -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'); }); diff --git a/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts b/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts index a16a054da45a..124dd3c2292e 100644 --- a/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts +++ b/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts @@ -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'; @@ -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(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'); + }, +); diff --git a/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/test.ts b/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/test.ts index 487cd05f1dbd..e05685ca4868 100644 --- a/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/test.ts +++ b/dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_overlapping-spans/test.ts @@ -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 }>; expect(spans.length).toBeGreaterThan(0); for (const span of spans) { diff --git a/packages/browser/src/profiling/utils.ts b/packages/browser/src/profiling/utils.ts index c50c76c84de4..dceb5e45a691 100644 --- a/packages/browser/src/profiling/utils.ts +++ b/packages/browser/src/profiling/utils.ts @@ -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 => { diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 766e66a72a99..3afe8fa2442c 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -1222,7 +1222,7 @@ export abstract class Client { this.emit('postprocessEvent', evt, hint); evt.contexts = { - trace: getTraceContextFromScope(currentScope), + trace: { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) }, ...evt.contexts, };