From 0081baf12eb8e8e6295204c61355701e71298a1a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 15 Jun 2023 14:59:52 +0200 Subject: [PATCH 1/3] fix(core): Temporarily store debug IDs in stack frame and put them into `debug_meta` before sending --- packages/core/src/utils/prepareEvent.ts | 49 +++++++++++--- packages/core/test/lib/prepareEvent.test.ts | 71 ++++++++++++++++----- 2 files changed, 96 insertions(+), 24 deletions(-) diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index 474a95c7823b..b24c6b57cfe0 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -38,9 +38,9 @@ export function prepareEvent( applyClientOptions(prepared, options); applyIntegrationsMetadata(prepared, integrations); - // Only apply debug metadata to error events. + // Only put debug IDs onto frames for error events. if (event.type === undefined) { - applyDebugMetadata(prepared, options.stackParser); + applyDebugIds(prepared, options.stackParser); } // If we have scope given to us, use it as the base for further modifications. @@ -75,6 +75,11 @@ export function prepareEvent( } return result.then(evt => { + // TODO explain why this is here + if (evt) { + applyDebugMeta(evt); + } + if (typeof normalizeDepth === 'number' && normalizeDepth > 0) { return normalizeEvent(evt, normalizeDepth, normalizeMaxBreadth); } @@ -121,9 +126,9 @@ function applyClientOptions(event: Event, options: ClientOptions): void { const debugIdStackParserCache = new WeakMap>(); /** - * Applies debug metadata images to the event in order to apply source maps by looking up their debug ID. + * Puts debug IDs into the stack frames of an error event. */ -export function applyDebugMetadata(event: Event, stackParser: StackParser): void { +export function applyDebugIds(event: Event, stackParser: StackParser): void { const debugIdMap = GLOBAL_OBJ._sentryDebugIds; if (!debugIdMap) { @@ -160,27 +165,55 @@ export function applyDebugMetadata(event: Event, stackParser: StackParser): void return acc; }, {}); - // Get a Set of filenames in the stack trace - const errorFileNames = new Set(); try { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion event!.exception!.values!.forEach(exception => { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion exception.stacktrace!.frames!.forEach(frame => { if (frame.filename) { - errorFileNames.add(frame.filename); + frame.debug_id = filenameDebugIdMap[frame.filename]; } }); }); } catch (e) { // To save bundle size we're just try catching here instead of checking for the existence of all the different objects. } +} + +/** + * Moves debug IDs from the stack frames of an error event into the debug_meta field. + */ +export function applyDebugMeta(event: Event): void { + // Extract debug IDs and filenames from the stack frames on the event. + const filenameDebugIdMap: Record = {}; + try { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + event.exception!.values!.forEach(exception => { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + exception.stacktrace!.frames!.forEach(frame => { + if (frame.debug_id) { + if (frame.abs_path) { + filenameDebugIdMap[frame.abs_path] = frame.debug_id; + } else if (frame.filename) { + filenameDebugIdMap[frame.filename] = frame.debug_id; + } + delete frame.debug_id; + } + }); + }); + } catch (e) { + // To save bundle size we're just try catching here instead of checking for the existence of all the different objects. + } + + if (Object.keys(filenameDebugIdMap).length === 0) { + return; + } // Fill debug_meta information event.debug_meta = event.debug_meta || {}; event.debug_meta.images = event.debug_meta.images || []; const images = event.debug_meta.images; - errorFileNames.forEach(filename => { + Object.keys(filenameDebugIdMap).forEach(filename => { if (filenameDebugIdMap[filename]) { images.push({ type: 'sourcemap', diff --git a/packages/core/test/lib/prepareEvent.test.ts b/packages/core/test/lib/prepareEvent.test.ts index d08b4ace221d..1e8b53e60f8c 100644 --- a/packages/core/test/lib/prepareEvent.test.ts +++ b/packages/core/test/lib/prepareEvent.test.ts @@ -1,14 +1,14 @@ import type { Event } from '@sentry/types'; import { createStackParser, GLOBAL_OBJ } from '@sentry/utils'; -import { applyDebugMetadata } from '../../src/utils/prepareEvent'; +import { applyDebugIds, applyDebugMeta } from '../../src/utils/prepareEvent'; -describe('applyDebugMetadata', () => { +describe('applyDebugIds', () => { afterEach(() => { GLOBAL_OBJ._sentryDebugIds = undefined; }); - it('should put debug source map images in debug_meta field', () => { + it("should put debug IDs into an event's stack frames", () => { GLOBAL_OBJ._sentryDebugIds = { 'filename1.js\nfilename1.js': 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', 'filename2.js\nfilename2.js': 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb', @@ -34,35 +34,74 @@ describe('applyDebugMetadata', () => { }, }; - applyDebugMetadata(event, stackParser); + applyDebugIds(event, stackParser); - expect(event.debug_meta?.images).toContainEqual({ - type: 'sourcemap', - code_file: 'filename1.js', + expect(event.exception?.values?.[0].stacktrace?.frames).toContainEqual({ + filename: 'filename1.js', debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', }); - expect(event.debug_meta?.images).toContainEqual({ - type: 'sourcemap', - code_file: 'filename2.js', + expect(event.exception?.values?.[0].stacktrace?.frames).toContainEqual({ + filename: 'filename2.js', debug_id: 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb', }); // expect not to contain an image for the stack frame that doesn't have a corresponding debug id - expect(event.debug_meta?.images).not.toContainEqual( + expect(event.exception?.values?.[0].stacktrace?.frames).not.toContainEqual( expect.objectContaining({ - type: 'sourcemap', - code_file: 'filename3.js', + filename3: 'filename3.js', + debug_id: expect.any(String), }), ); // expect not to contain an image for the debug id mapping that isn't contained in the stack trace - expect(event.debug_meta?.images).not.toContainEqual( + expect(event.exception?.values?.[0].stacktrace?.frames).not.toContainEqual( expect.objectContaining({ - type: 'sourcemap', - code_file: 'filename4.js', + filename3: 'filename4.js', debug_id: 'cccccccc-cccc-4ccc-cccc-cccccccccc', }), ); }); }); + +describe('applyDebugMeta', () => { + it("should move the debug IDs inside an event's stack frame into the debug_meta field", () => { + const event: Event = { + exception: { + values: [ + { + stacktrace: { + frames: [ + { filename: 'filename1.js', debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa' }, + { filename: 'filename2.js', debug_id: 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb' }, + { filename: 'filename1.js', debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa' }, + { filename: 'filename3.js' }, + ], + }, + }, + ], + }, + }; + + applyDebugMeta(event); + + expect(event.exception?.values?.[0].stacktrace?.frames).toEqual([ + { filename: 'filename1.js' }, + { filename: 'filename2.js' }, + { filename: 'filename1.js' }, + { filename: 'filename3.js' }, + ]); + + expect(event.debug_meta?.images).toContainEqual({ + type: 'sourcemap', + code_file: 'filename1.js', + debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', + }); + + expect(event.debug_meta?.images).toContainEqual({ + type: 'sourcemap', + code_file: 'filename2.js', + debug_id: 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb', + }); + }); +}); From 56b6feba3f8b90b84a1b25aaf7459af8fee6f10f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 16 Jun 2023 11:38:49 +0200 Subject: [PATCH 2/3] Rmove redundant check --- packages/core/src/utils/prepareEvent.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index b24c6b57cfe0..d100518cef28 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -214,13 +214,11 @@ export function applyDebugMeta(event: Event): void { event.debug_meta.images = event.debug_meta.images || []; const images = event.debug_meta.images; Object.keys(filenameDebugIdMap).forEach(filename => { - if (filenameDebugIdMap[filename]) { - images.push({ - type: 'sourcemap', - code_file: filename, - debug_id: filenameDebugIdMap[filename], - }); - } + images.push({ + type: 'sourcemap', + code_file: filename, + debug_id: filenameDebugIdMap[filename], + }); }); } From a97549aab91100154632ac43a6c3e352f44adad5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 16 Jun 2023 11:45:16 +0200 Subject: [PATCH 3/3] Add comment --- packages/core/src/utils/prepareEvent.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index d100518cef28..84bfd404c56f 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -75,8 +75,11 @@ export function prepareEvent( } return result.then(evt => { - // TODO explain why this is here if (evt) { + // We apply the debug_meta field only after all event processors have ran, so that if any event processors modified + // file names (e.g.the RewriteFrames integration) the filename -> debug ID relationship isn't destroyed. + // This should not cause any PII issues, since we're only moving data that is already on the event and not adding + // any new data applyDebugMeta(evt); }