Skip to content

Commit a2cb952

Browse files
committed
guard profiling data attachment; add tests
1 parent 1389a04 commit a2cb952

2 files changed

Lines changed: 21 additions & 15 deletions

File tree

  • dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling
  • packages/browser/src/profiling

dev-packages/browser-integration-tests/suites/feedback/captureFeedbackWithProfiling/test.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ sentryTest(
3939

4040
await page.goto(url);
4141

42-
// Wait for the idle span to finish
43-
await pageloadRequestPromise;
42+
// Wait for the idle page load span to finish
43+
const pageLoadEvent = envelopeRequestParser(await pageloadRequestPromise);
4444

4545
// Submit feedback after idle span ended — no active span
4646
await page.getByText('Report a Bug').click();
@@ -51,10 +51,14 @@ sentryTest(
5151

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

54-
// BUG: With profiling enabled and no active span, attachProfiledThreadToEvent
55-
// creates a partial trace context { data: { thread.id, thread.name } } that
56-
// overwrites the real trace context (with trace_id/span_id) via spread in _prepareEvent.
5754
expect(feedbackEvent.contexts?.trace?.trace_id).toMatch(/\w{32}/);
5855
expect(feedbackEvent.contexts?.trace?.span_id).toMatch(/\w{16}/);
56+
57+
// contexts.trace.data must include thread.id to identify which thread is associated with the transaction
58+
expect(pageLoadEvent.contexts?.trace?.data?.['thread.id']).toBe('0');
59+
expect(pageLoadEvent.contexts?.trace?.data?.['thread.name']).toBe('main');
60+
61+
// fixme: figure out why profiler_id is set on feedback and not on pageload transaction
62+
expect(feedbackEvent.contexts?.profile?.profiler_id).toMatch(/^[a-f\d]{32}$/);
5963
},
6064
);

packages/browser/src/profiling/utils.ts

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -798,16 +798,18 @@ export function attachProfiledThreadToEvent(event: Event): Event {
798798
return event;
799799
}
800800

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

812814
// Attach thread info to individual spans so that spans can be associated with the profiled thread on the UI even if contexts are missing.
813815
event.spans?.forEach(span => {

0 commit comments

Comments
 (0)