fix(browser): Forward worker metadata for third-party error filtering #18756
Conversation
The `thirdPartyErrorFilterIntegration` was not able to identify first-party worker code as because module metadata stayed in the worker's separate global scope and wasn't accessible to the main thread. We now forward the metadata the same way we forward debug ids to the main thread which allows first-party worker code to be identified as such. Closes: #18705
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| // Must have at least one of: debug IDs, module metadata, or worker error | ||
| const hasDebugIds = '_sentryDebugIds' in eventData; | ||
| const hasModuleMetadata = '_sentryModuleMetadata' in eventData; | ||
| const hasWorkerError = '_sentryWorkerError' in eventData; | ||
|
|
||
| if (!hasDebugIds && !hasWorkerError) { | ||
| if (!hasDebugIds && !hasModuleMetadata && !hasWorkerError) { | ||
| return false; | ||
| } | ||
|
|
||
| // Validate debug IDs if present | ||
| if (hasDebugIds && !(isPlainObject(eventData._sentryDebugIds) || eventData._sentryDebugIds === undefined)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Validate module metadata if present | ||
| if ( | ||
| hasModuleMetadata && | ||
| !(isPlainObject(eventData._sentryModuleMetadata) || eventData._sentryModuleMetadata === undefined) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
l: I'm wondering if we should turn around this validation logic to default to false and only emit true in case we find fitting fields on the event data? This would make it easier to add new fields because we don't have to always extend the first if (!hasDebugIds && !hasModuleMetadata && !hasWorkerError) check. wdyt?
(logaf-l so feel free to skip)
There was a problem hiding this comment.
I think that ends up with even more complicated code because we still need to ensure that present fields are validated, we can't exit early basically. The code is more readable this way, unless I'm missing something 😅.
packages/core/src/metadata.ts
Outdated
| * @param metadataMap - A map of filename to metadata object | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function mergeMetadataMap(metadataMap: Record<string, any>): void { |
There was a problem hiding this comment.
q: IIUC we need this because we need to update the filenameMetadataMap once we receive the message from the worker, correct?
There was a problem hiding this comment.
Yep, but based on your follow up question I reworked this, no need for it anymore!
| */ | ||
| export function registerWebWorker({ self }: RegisterWebWorkerOptions): void { | ||
| // Send debug IDs to parent thread | ||
| const moduleMetadata = self._sentryModuleMetadata ? getFilenameToMetadataMap(defaultStackParser) : undefined; |
There was a problem hiding this comment.
naive q: why do we need to parse the self._sentryModuleMetadata here? can't we do this in the main thread in mergeMetadata, potentially by re-calling ensureMetadataStacksAreParsed?
I'm probably missing something, so just asking 😅
There was a problem hiding this comment.
No, you're right. We don't need to parse the module metadata on the worker, can just pass it along and handle everything on the main-thread. That should reduce code and some api I exposed.
Thanks!
| interface WebWorkerMessage { | ||
| _sentryMessage: boolean; | ||
| _sentryDebugIds?: Record<string, string>; | ||
| _sentryModuleMetadata?: Record<string, any>; // eslint-disable-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
To be aligned with the way we define it in the main-thread, maybe we can change that at a later point?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
andreiborza
force-pushed
the
ab/third-party-errors-workers
branch
from
65aefd3 to
72a0090
Compare
andreiborza
force-pushed
the
ab/third-party-errors-workers
branch
from
72a0090 to
81ddea1
Compare
The
thirdPartyErrorFilterIntegrationwas not able to identify first-party worker code as because module metadata stayed in the worker's separate global scope and wasn't accessible to the main thread.We now forward the metadata the same way we forward debug ids to the main thread which allows first-party worker code to be identified as such.
Closes: #18705