diff --git a/AGENTS.md b/AGENTS.md index ac6c2c8fc..2d9d0a7ef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -689,9 +689,9 @@ For a hook whose job is redaction, that is the one failure mode that matters, an instead of testing it for truthiness. Same reason the URL redaction runs **before** `shouldKeepEvent` and the error-text redaction -**after** it. The filter attributes errors on the raw stack and raw `error.resource.url`, so those -can't be rewritten ahead of it — but it reads no URL field, and it can itself throw on a malformed -error field, which would ship the event with its URLs untouched. +**after** it. The filter attributes errors on the raw stack, so that can't be rewritten ahead of +it — but it reads no URL field, and it can itself throw on a malformed error field, which would +ship the event with its URLs untouched. `error.stack` is **not** the engine's stack string. The SDK parses it and re-serializes every stack it records, so what reaches `beforeSend` is always this shape, whatever the engine produced — no V8 diff --git a/src/integrations/datadog/beforeSend.test.ts b/src/integrations/datadog/beforeSend.test.ts index 60642d99d..86a062a48 100644 --- a/src/integrations/datadog/beforeSend.test.ts +++ b/src/integrations/datadog/beforeSend.test.ts @@ -284,9 +284,8 @@ describe('beforeSend', () => { expect(beforeSend(event)).toBe(true); }); - // Each field is read twice, by the filter and by the redaction, so one case pins both guards. The - // 5xx message makes the filter coerce `resource.url`: under `?? ''` the array stringifies into an - // instance endpoint and the event is silently dropped rather than kept. + // The filter and the redaction both read `stack`, so that case pins both guards; the redaction alone + // reads the other two. it.each([ 'stack', 'handling_stack', diff --git a/src/integrations/datadog/shouldKeepEvent.test.ts b/src/integrations/datadog/shouldKeepEvent.test.ts index ba1c21e31..2d78afd03 100644 --- a/src/integrations/datadog/shouldKeepEvent.test.ts +++ b/src/integrations/datadog/shouldKeepEvent.test.ts @@ -160,27 +160,6 @@ describe('shouldKeepEvent', () => { ).toBe(false); }); - // The name gate above deliberately does not fire for a nameless timeout, so this is the only - // thing dropping the tracked *resource* errors, which need not carry `type: 'AxiosError'`. - it('discards timeout errors against instance/cluster resource URLs, in either spelling', () => { - expect( - shouldKeepEvent( - errorEvent({ - message: 'timeout of 60000ms exceeded', - resource: { url: 'https://api.harper.fast/HDBInstance/ins-123/operation' }, - }), - ), - ).toBe(false); - expect( - shouldKeepEvent( - errorEvent({ - message: 'timeout exceeded', - resource: { url: 'https://api.harper.fast/Cluster/clu-123/operation' }, - }), - ), - ).toBe(false); - }); - // Pins that the name gate only ever adds drops: the numeric spelling is still dropped without // a name, so a timeout Studio re-wraps into a plain `Error` (`secrets.ts`) behaves as before, // while the bare spelling — indistinguishable from prose without a name — is reported. @@ -191,22 +170,6 @@ describe('shouldKeepEvent', () => { expect(shouldKeepEvent(errorEvent({ message: 'timeout exceeded' }))).toBe(true); }); - it('discards network failures against an instance/cluster operation endpoint', () => { - expect( - shouldKeepEvent( - errorEvent({ - message: 'Network Error', - resource: { url: 'https://api.harper.fast/Cluster/clu-123/operation' }, - }), - ), - ).toBe(false); - expect( - shouldKeepEvent( - errorEvent({ source: 'network', resource: { url: 'https://api.harper.fast/HDBInstance/ins-9/operation' } }), - ), - ).toBe(false); - }); - // Regression test for issue #1386: a session expiring / signing out mid-poll leaves // an in-flight request that 401s; these reach RUM as handled AxiosErrors with no URL. it('discards 401 auth failures on the message alone but keeps 403', () => { @@ -243,28 +206,29 @@ describe('shouldKeepEvent', () => { expect(shouldKeepEvent(errorEvent({ message: 'Failed to deploy component: invalid package' }))).toBe(true); }); - it('discards 5xx errors against an instance/cluster operation endpoint', () => { - expect( - shouldKeepEvent( - errorEvent({ - message: 'AxiosError: Request failed with status code 500', - resource: { url: 'https://api.harper.fast/HDBInstance/ins-123/operation' }, - }), - ), - ).toBe(false); - }); + // #1669 owns whether these console-relayed request failures should be dropped. Until then they + // are kept, and nothing about the request can change that: the SDK never names one (#1690). + describe('request failures', () => { + const instanceOperationUrls = [ + 'https://api.harper.fast/HDBInstance/ins-123/operation', + 'https://api.harper.fast/Cluster/clu-123/operation', + ]; - it('keeps 5xx errors that are not attributable to an instance/cluster operation endpoint', () => { - // An unattributed 5xx could be a genuine backend failure worth surfacing. - expect(shouldKeepEvent(errorEvent({ message: 'Request failed with status code 500' }))).toBe(true); - expect( - shouldKeepEvent( - errorEvent({ - message: 'Request failed with status code 502', - resource: { url: 'https://api.harper.fast/Organization/org-1' }, - }), - ), - ).toBe(true); + it.each([ + 'AxiosError: Request failed with status code 500', + 'AxiosError: Request failed with status code 502', + 'AxiosError: Network Error', + ])('keeps a console-relayed %s', (message) => { + expect(shouldKeepEvent(errorEvent({ type: 'AxiosError', source: 'console', message }))).toBe(true); + }); + + it.each(instanceOperationUrls)('keeps them with an error.resource.url of %s', (url) => { + for (const message of ['Request failed with status code 500', 'Network Error', 'timeout exceeded']) { + expect(shouldKeepEvent(errorEvent({ message }))).toBe(true); + expect(shouldKeepEvent(errorEvent({ message, resource: { url } }))).toBe(true); + } + expect(shouldKeepEvent(errorEvent({ source: 'network', resource: { url } }))).toBe(true); + }); }); // Regression tests for the Reo.dev analytics errors that reached Error Tracking on @@ -360,6 +324,126 @@ describe('shouldKeepEvent', () => { }); }); + // Regression tests for #1696, on the stacks RUM recorded for GTM's LinkedIn Insight tag. The guard + // frame is `installBrowserTranslationDomGuard`'s `insertBefore`, which `vite.config.ts` builds into + // the Datadog chunk; each browser names it differently. + describe('vendor tags injected through the DOM guard', () => { + const gtm = 'https://www.googletagmanager.com/gtm.js?id=GTM-5QQX432'; + const guardChunk = 'https://fabric.harper.fast/assets/vendor-datadog-CLUcJXOo.js:3:3191'; + const lintrk = (guardFrame: string, inlineFrame = ' at @ :1:8') => + errorEvent({ + type: 'TypeError', + source: 'source', + message: 'window.lintrk is not a function', + stack: [ + 'TypeError: window.lintrk is not a function', + inlineFrame, + guardFrame, + ` at @ ${gtm}:546:207`, + ` at WS @ ${gtm}:552:412`, + ` at Qd.VS @ ${gtm}:552:96`, + ].join('\n'), + }); + + it.each([ + ` at Node.insertBefore @ ${guardChunk}`, + ` at QI.Node.insertBefore @ ${guardChunk}`, + ` at @ ${guardChunk}`, + ])('discards a tag that throws while GTM inserts it (%s)', (guardFrame) => { + expect(shouldKeepEvent(lintrk(guardFrame))).toBe(false); + }); + + // Firefox names the inline script after the page, in a form that carries no parseable URL. + it('discards the Firefox shape', () => { + expect( + shouldKeepEvent( + lintrk( + ` at QI/Node.prototype.insertBefore @ ${guardChunk}`, + ' at @ https://fabric.harper.fast/#/sign-in line 829 > injectedScript:1:8', + ), + ), + ).toBe(false); + }); + + // The neutral frame is what the build placement buys: in the app entry it is a Studio frame. + it('keeps the same stack while the guard is built into the app entry', () => { + expect( + shouldKeepEvent( + lintrk(' at QI.Node.insertBefore @ https://fabric.harper.fast/assets/index-BmfggGwq.js:829:50787'), + ), + ).toBe(true); + }); + + it('keeps a Studio error that passes through the guard', () => { + expect( + shouldKeepEvent( + errorEvent({ + type: 'HierarchyRequestError', + source: 'source', + message: "Failed to execute 'insertBefore' on 'Node': The new child element contains the parent.", + stack: [ + "HierarchyRequestError: Failed to execute 'insertBefore' on 'Node': The new child element contains the parent.", + ` at QI.Node.insertBefore @ ${guardChunk}`, + ' at Ua @ https://fabric.harper.fast/assets/vendor-react-B2kMp9xQ.js:8:41022', + ' at Gs @ https://fabric.harper.fast/assets/vendor-react-B2kMp9xQ.js:8:47135', + ].join('\n'), + }), + ), + ).toBe(true); + expect( + shouldKeepEvent( + errorEvent({ + message: 'Boom', + stack: [ + 'TypeError: Boom', + ' at @ :1:8', + ` at QI.Node.insertBefore @ ${guardChunk}`, + ' at useGTM @ https://fabric.harper.fast/assets/index-BmfggGwq.js:5:1234', + ].join('\n'), + }), + ), + ).toBe(true); + }); + + it('keeps a stack whose only located frame is the guard', () => { + expect( + shouldKeepEvent( + errorEvent({ + message: 'Boom', + stack: ['TypeError: Boom', ` at QI.Node.insertBefore @ ${guardChunk}`].join('\n'), + }), + ), + ).toBe(true); + }); + + it('discards a stack of nothing but GTM frames', () => { + expect( + shouldKeepEvent( + errorEvent({ + message: 'Failed to fetch', + stack: [ + 'TypeError: Failed to fetch', + ` at @ ${guardChunk}`, + ` at wp.sendRequest @ ${gtm}:463:308`, + ` at Qu @ ${gtm}:470:87`, + ].join('\n'), + }), + ), + ).toBe(false); + }); + + it('keeps a frame on a host that merely starts with the GTM host', () => { + expect( + shouldKeepEvent( + errorEvent({ + message: 'Boom', + stack: ['TypeError: Boom', ' at x @ https://www.googletagmanager.com.example.net/gtm.js:1:1'].join('\n'), + }), + ), + ).toBe(true); + }); + }); + // Regression tests for #1645. Stacks below are the real ones RUM recorded, truncated. describe('browser extension content scripts', () => { it('discards errors whose stack is nothing but extension frames', () => { @@ -712,15 +796,4 @@ describe('shouldKeepEvent', () => { ).toBe(true); }); }); - - it('keeps non-timeout network failures that are not attributable to an instance endpoint', () => { - // Without an instance/cluster URL we cannot tell a real backend failure from an - // expected one, so a bare "Network Error" stays visible (unlike timeouts). - expect(shouldKeepEvent(errorEvent({ message: 'Network Error' }))).toBe(true); - expect( - shouldKeepEvent( - errorEvent({ message: 'Network Error', resource: { url: 'https://api.harper.fast/Organization/org-1' } }), - ), - ).toBe(true); - }); }); diff --git a/src/integrations/datadog/shouldKeepEvent.ts b/src/integrations/datadog/shouldKeepEvent.ts index da7f185ca..f1c54a696 100644 --- a/src/integrations/datadog/shouldKeepEvent.ts +++ b/src/integrations/datadog/shouldKeepEvent.ts @@ -24,11 +24,12 @@ export interface DatadogErrorEvent { } /** - * Scripts we embed but do not ship or control — currently only the Reo.dev analytics - * bundle (`src/integrations/reo/reo.ts` loads it from this CDN). Errors thrown inside - * them are vendor bugs we can neither reproduce nor fix. + * Scripts we embed but do not ship or control: the Reo.dev analytics bundle + * (`src/integrations/reo/reo.ts`) and Google Tag Manager (`src/integrations/google/gtm.ts`), whose + * container injects the marketing tags. Errors thrown inside them are vendor bugs we can neither + * reproduce nor fix. */ -const THIRD_PARTY_SCRIPT_FRAME = /^https?:\/\/static\.reo\.dev\//; +const THIRD_PARTY_SCRIPT_FRAME = /^https?:\/\/(?:static\.reo\.dev|www\.googletagmanager\.com)\//; /** * A content script the user's own browser extension injected into the page. Matched on the @@ -80,6 +81,9 @@ const NESTED_ORIGIN_PREFIX = /^blob:/; * The Datadog browser SDK monkey-patches `fetch`/XHR, so its own bundle sits at the top * of every stack it instruments — including stacks whose real origin is third-party. * Treat it as instrumentation rather than as a Studio frame when attributing an error. + * `vite.config.ts` puts `installBrowserTranslationDomGuard` in this chunk for the same reason: + * every script that inserts a node runs through its `insertBefore`, so a vendor tag that throws + * while GTM injects it would otherwise carry a Studio frame (#1696). */ const INSTRUMENTATION_FRAME = /\/assets\/vendor-datadog-[^/\s]*\.js/; @@ -114,16 +118,14 @@ function originatesInThirdPartyScript(stack: string, type: string | undefined, m * Datadog `beforeSend` predicate: returns true to keep the event, false to discard * it (false is what tells RUM to drop the event). Wired straight into `beforeSend`. * - * Reaching an instance can legitimately fail when it's down, restarting, or the - * user lacks Fabric Connect — these are expected states, not Studio bugs, and they - * otherwise flood Error Tracking. We drop connectivity-class errors (timeouts, - * network failures) so they don't create issues. - * - * These errors reach RUM two ways: as tracked resource errors (which carry an - * `error.resource.url`) and as handled AxiosErrors surfaced through `console.error` - * — most notably React Query's global error handler — which do NOT. The URL-based - * endpoint check below only ever sees the former, so the URL-less timeouts slipped - * past it and flooded Error Tracking (issue #1371). + * The browser RUM SDK makes an error event from a thrown exception (`source: 'source'`), a + * `console.error` call (`'console'`), `addError` (`'custom'` — `addReactError` in `main.tsx`) or a + * browser report (`'report'`). A failed request is a `resource` event, never an error: + * `ErrorSource.NETWORK` is declared but nothing emits it, and `processError` + * (`browser-rum-core/…/errorCollection`) builds no `error.resource`. So a handled request failure + * reaches this filter only as whatever logged it, mostly React Query's global handler, with nothing + * naming the endpoint. Attribute by message, name and stack; an unreachable instance is visible only + * as `@type:resource @resource.status_code:0`. */ export function shouldKeepEvent(event: DatadogErrorEvent) { if (event.type !== 'error') { @@ -132,11 +134,9 @@ export function shouldKeepEvent(event: DatadogErrorEvent) { // Type-checked, not `?? ''`: a malformed field would throw out of this filter, which the SDK // swallows into shipping the event unfiltered. const message = typeof event.error?.message === 'string' ? event.error.message : ''; - const source = event.error?.source; // Aborted requests are client-initiated cancellations (navigating away, a // component unmounting, React Query cancelling an in-flight query) — not failures. - // Safe to drop globally regardless of endpoint. if (/Request aborted/i.test(message)) { return false; } @@ -215,11 +215,9 @@ export function shouldKeepEvent(event: DatadogErrorEvent) { // A request timeout is a connectivity-class failure, never a Studio bug: the instance, // cluster, or backend was too slow to answer, and backend latency is tracked server-side. - // The handled AxiosErrors that flood Error Tracking (#1371) carry no resource URL, so the - // endpoint gate below cannot reach them. The bare spelling needs the name because on its - // own it would also discard a Harper-relayed `Operation timeout exceeded`; the numeric one - // stays name-free, so a timeout Studio re-wraps into a plain `Error` (`secrets.ts`) is - // dropped exactly as before. + // The bare spelling needs the name because on its own it would also discard a Harper-relayed + // `Operation timeout exceeded`; the numeric one stays name-free, so a timeout Studio re-wraps + // into a plain `Error` (`secrets.ts`) is dropped exactly as before. if ( NUMERIC_TIMEOUT_MESSAGE.test(message) || (errorName === 'AxiosError' && BARE_TIMEOUT_MESSAGE.test(message)) @@ -256,26 +254,5 @@ export function shouldKeepEvent(event: DatadogErrorEvent) { return false; } - const url = typeof event.error?.resource?.url === 'string' ? event.error.resource.url : ''; - const isInstanceEndpoint = /\/(HDBInstance|Cluster)\/[^/]+\/operation/.test(url); - - // A 5xx or a bare timeout from an instance/cluster operation endpoint is the instance itself - // failing to answer (broken, unsupported, or mid-restart) — a server-side condition tracked - // on the backend, not a Studio bug. The URL attributes it whatever the thrown value's name - // is, which is what reaches the tracked resource errors the name gate above cannot. - if ( - isInstanceEndpoint - && (/Request failed with status code 5\d\d\b/i.test(message) || BARE_TIMEOUT_MESSAGE.test(message)) - ) { - return false; - } - - // Other network failures (connection refused, DNS, offline) against an - // instance/cluster operation endpoint — expected when the instance is unreachable. - const isNetworkFailure = /Network Error/i.test(message) || source === 'network'; - if (isInstanceEndpoint && isNetworkFailure) { - return false; - } - return true; } diff --git a/tsconfig.node.json b/tsconfig.node.json index b0cf4fa87..d5b42dad2 100644 --- a/tsconfig.node.json +++ b/tsconfig.node.json @@ -25,6 +25,7 @@ "noUncheckedSideEffectImports": true }, "include": [ - "vite.config.ts" + "vite.config.ts", + "vite.config.test.ts" ] } diff --git a/vite.config.test.ts b/vite.config.test.ts new file mode 100644 index 000000000..f4f508cba --- /dev/null +++ b/vite.config.test.ts @@ -0,0 +1,31 @@ +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; +import viteConfig from './vite.config'; + +function buildOutput() { + const config = viteConfig({ mode: 'prod', command: 'build', isSsrBuild: false, isPreview: false }); + if (typeof config === 'function' || config instanceof Promise) { + throw new Error('vite.config.ts no longer returns a plain config object'); + } + return config.build?.rollupOptions?.output as { manualChunks?: unknown; chunkFileNames?: unknown }; +} + +// `shouldKeepEvent` reads a `vendor-datadog` frame as neutral instrumentation (#1696). +describe('manualChunks', () => { + it('builds the translation DOM guard into the Datadog chunk', () => { + const { manualChunks, chunkFileNames } = buildOutput(); + const guard = path.resolve(__dirname, 'src/lib/installBrowserTranslationDomGuard.ts'); + const chunkOf = manualChunks as (id: string) => string | undefined; + + expect(chunkOf(guard)).toBe('vendor-datadog'); + expect(chunkOf(path.resolve(__dirname, 'node_modules/@datadog/browser-rum/esm/index.js'))).toBe('vendor-datadog'); + expect(chunkFileNames).toBe('assets/[name]-[hash].js'); + }); + + it('leaves the rest of Studio to the automatic split', () => { + const chunkOf = buildOutput().manualChunks as (id: string) => string | undefined; + + expect(chunkOf(path.resolve(__dirname, 'src/main.tsx'))).toBeUndefined(); + expect(chunkOf(path.resolve(__dirname, 'src/lib/installStaleDeployReload.ts'))).toBeUndefined(); + }); +}); diff --git a/vite.config.ts b/vite.config.ts index 23b54fcf7..7e743cd2d 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -205,6 +205,12 @@ export default defineConfig(({ mode }) => ({ // Forcing them into manual chunks does the opposite — it promotes them to // eager — so leave them to auto-split. manualChunks(id) { + // The translation DOM guard wraps `Node.prototype.insertBefore`, so it is on the stack of + // every script that inserts a node, vendor tags included. `shouldKeepEvent` treats frames + // from this chunk as neutral instrumentation; in the app entry they would read as Studio's. + if (id.replace(/\\/g, '/').endsWith('/src/lib/installBrowserTranslationDomGuard.ts')) { + return 'vendor-datadog'; + } if (!id.includes('node_modules')) { return; } // Resolve the npm package name from the module path so rules match on