refactor(datadog): remove shouldKeepEvent's instance-endpoint gate, which no RUM error can reach

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

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',

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 <anonymous> @ <anonymous>: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 <anonymous> @ ${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 <anonymous> @ ${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 <anonymous> @ 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 <anonymous> @ <anonymous>: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 <anonymous> @ ${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);
-	});
 });

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;
 }

tsconfig.node.json

@@ -25,6 +25,7 @@
 		"noUncheckedSideEffectImports": true
 	},
 	"include": [
-		"vite.config.ts"
+		"vite.config.ts",
+		"vite.config.test.ts"
 	]
 }