diff --git a/src/lib/network-error.test.ts b/src/lib/network-error.test.ts new file mode 100644 index 0000000000..7e91b14a08 --- /dev/null +++ b/src/lib/network-error.test.ts @@ -0,0 +1,39 @@ +import {describe, expect, test} from '@jest/globals' + +import {isNetworkError} from '#/lib/network-error' + +describe('isNetworkError', () => { + test.each([ + 'Network request failed', + 'TypeError: Failed to fetch', + 'Error: fetch failed: java.net.UnknownHostException', + 'The Internet connection appears to be offline', + 'The network connection was lost', + 'A server with the specified hostname could not be found', + 'TypeError: Network request timed out', + ])('detects %s', message => { + expect(isNetworkError(new Error(message))).toBe(true) + }) + + test('checks wrapped error causes', () => { + const error = new Error('Unable to fulfill XRPC request', { + cause: new Error('fetch failed: connection closed'), + }) + + expect(isNetworkError(error)).toBe(true) + }) + + test('does not treat arbitrary fetch-handler failures as network errors', () => { + expect( + isNetworkError( + new Error( + 'Unexpected fetchHandler() error: URL.canParse is not a function', + ), + ), + ).toBe(false) + }) + + test('does not match words ending in load', () => { + expect(isNetworkError(new Error('Multipart upload failed'))).toBe(false) + }) +}) diff --git a/src/lib/network-error.ts b/src/lib/network-error.ts new file mode 100644 index 0000000000..a5647338f4 --- /dev/null +++ b/src/lib/network-error.ts @@ -0,0 +1,39 @@ +const NETWORK_ERROR_PATTERNS = [ + /\babort(?:ed|error)?\b/i, + /network request failed/i, + /failed to fetch/i, + /fetch failed/i, + /\bload failed\b/i, + /upstream service unreachable/i, + /networkerror when attempting to fetch resource/i, + /internet connection appears to be offline/i, + /network connection was lost/i, + /unknownhostexception/i, + /unable to resolve host/i, + /server with the specified hostname could not be found/i, + /network request timed out/i, + /connectexception: failed to connect/i, + /sslhandshakeexception: connection closed/i, +] + +/** + * Detects transport failures across the error strings produced by web, native + * fetch, and the XRPC clients. Error causes are checked because the XRPC + * clients wrap the platform-specific fetch error. + */ +export function isNetworkError(value: unknown): boolean { + return isNetworkErrorInner(value, new Set()) +} + +function isNetworkErrorInner(value: unknown, seen: Set): boolean { + if (NETWORK_ERROR_PATTERNS.some(pattern => pattern.test(String(value)))) { + return true + } + + if (typeof value !== 'object' || value === null || seen.has(value)) { + return false + } + + seen.add(value) + return 'cause' in value && isNetworkErrorInner(value.cause, seen) +} diff --git a/src/lib/strings/errors.ts b/src/lib/strings/errors.ts index dc3f667efa..31cd7a15b8 100644 --- a/src/lib/strings/errors.ts +++ b/src/lib/strings/errors.ts @@ -1,8 +1,11 @@ import {LexError} from '@atproto/lex' import {t} from '@lingui/core/macro' +import {isNetworkError} from '#/lib/network-error' import {isXrpcError} from '#/lib/xrpc-error' +export {isNetworkError} from '#/lib/network-error' + /** * The text to show the user when no special case applies. * @@ -79,26 +82,6 @@ export function cleanError(e: unknown): string { return toDisplayString(e, str) } -const NETWORK_ERRORS = [ - 'Abort', - 'Network request failed', - 'Failed to fetch', - 'fetch failed', - 'Load failed', - 'Upstream service unreachable', - 'NetworkError when attempting to fetch resource', -] - -export function isNetworkError(e: unknown) { - const str = String(e) - for (const err of NETWORK_ERRORS) { - if (str.includes(err)) { - return true - } - } - return false -} - /** * The PDS answers an app-password-scope rejection with the lexicon code * `InvalidToken` and a message of 'Bad token scope' or 'Bad token method' diff --git a/src/logger/__tests__/logger.test.ts b/src/logger/__tests__/logger.test.ts index e82f644a53..3b11283a6f 100644 --- a/src/logger/__tests__/logger.test.ts +++ b/src/logger/__tests__/logger.test.ts @@ -301,6 +301,15 @@ describe('general functionality', () => { timestamp, ) + // call sites do not consistently use the same metadata key + sentryTransport( + LogLevel.Error, + Logger.Context.Default, + 'request failed', + {underlyingError: new Error('fetch failed: connection closed')}, + timestamp, + ) + jest.runAllTimers() expect(Sentry.captureMessage).not.toHaveBeenCalled() @@ -312,6 +321,15 @@ describe('general functionality', () => { {}, timestamp, ) + + // network error in metadata with an Error object message + sentryTransport( + LogLevel.Error, + Logger.Context.Default, + new Error('request failed'), + {underlyingError: new Error('fetch failed: connection closed')}, + timestamp, + ) expect(Sentry.captureException).not.toHaveBeenCalled() }) diff --git a/src/logger/sentry/network-errors.test.ts b/src/logger/sentry/network-errors.test.ts new file mode 100644 index 0000000000..173af6ae21 --- /dev/null +++ b/src/logger/sentry/network-errors.test.ts @@ -0,0 +1,102 @@ +import {type Procedure, XrpcFetchError, XrpcResponseError} from '@atproto/lex' +import {describe, expect, test} from '@jest/globals' +import {type ErrorEvent} from '@sentry/react-native' + +import { + dropExpectedNetworkErrors, + isExpectedSentryNetworkError, +} from '#/logger/sentry/network-errors' + +const method = { + nsid: 'app.bsky.test.getTest', + type: 'query', +} as unknown as Procedure + +function xrpcResponseError(status: number) { + return new XrpcResponseError(method, new Response(null, {status}), undefined) +} + +describe('isExpectedSentryNetworkError', () => { + test.each([502, 503, 504])('detects transient upstream status %s', status => { + expect(isExpectedSentryNetworkError(xrpcResponseError(status))).toBe(true) + }) + + test.each([429, 500])('keeps actionable HTTP status %s', status => { + expect(isExpectedSentryNetworkError(xrpcResponseError(status))).toBe(false) + }) + + test('checks an XRPC fetch error cause', () => { + expect( + isExpectedSentryNetworkError( + new XrpcFetchError(method, new Error('fetch failed: connection reset')), + ), + ).toBe(true) + }) + + test('keeps implementation errors wrapped by XRPC fetch errors', () => { + expect( + isExpectedSentryNetworkError( + new XrpcFetchError( + method, + new TypeError('URL.canParse is not a function'), + ), + ), + ).toBe(false) + }) +}) + +describe('dropExpectedNetworkErrors', () => { + test('drops automatic captures using the original exception', () => { + const event = {type: undefined} satisfies ErrorEvent + + expect( + dropExpectedNetworkErrors(event, { + originalException: new Error('fetch failed: connection closed'), + }), + ).toBeNull() + }) + + test('checks every exception in a linked error chain', () => { + const event = { + type: undefined, + exception: { + values: [ + {type: 'Error', value: 'Network request failed'}, + { + type: 'XrpcInternalError', + value: 'Unable to fulfill XRPC request', + }, + ], + }, + } satisfies ErrorEvent + + expect(dropExpectedNetworkErrors(event, {})).toBeNull() + }) + + test('drops serialized transient upstream errors', () => { + const event = { + type: undefined, + exception: { + values: [{type: 'XrpcResponseError', value: 'Upstream Timeout'}], + }, + } satisfies ErrorEvent + + expect(dropExpectedNetworkErrors(event, {})).toBeNull() + }) + + test('keeps non-network events', () => { + const event = { + type: undefined, + exception: { + values: [ + { + type: 'XrpcFetchError', + value: 'URL.canParse is not a function', + }, + ], + }, + } satisfies ErrorEvent + + expect(dropExpectedNetworkErrors(event, {})).toBe(event) + }) +}) diff --git a/src/logger/sentry/network-errors.ts b/src/logger/sentry/network-errors.ts new file mode 100644 index 0000000000..21a22f1a60 --- /dev/null +++ b/src/logger/sentry/network-errors.ts @@ -0,0 +1,56 @@ +import {XrpcResponseError} from '@atproto/lex' +import {type ErrorEvent} from '@sentry/react-native' + +import {isNetworkError} from '#/lib/network-error' + +const TRANSIENT_UPSTREAM_ERRORS = new Set([ + 'UpstreamFailure', + 'NotEnoughResources', + 'UpstreamTimeout', +]) + +const TRANSIENT_UPSTREAM_MESSAGES = [ + /upstream\s*failure/i, + /not\s*enough\s*resources/i, + /upstream\s*timeout/i, + /operation timed out, please try again/i, +] + +/** + * Sentry should not report expected transport failures or transient upstream + * availability errors. Keep this narrower than `XrpcError.shouldRetry()`: + * fetch handlers can wrap implementation bugs, while 429s and generic 500s + * can reveal actionable client or server regressions. + */ +export function isExpectedSentryNetworkError(value: unknown): boolean { + if (isNetworkError(value)) { + return true + } + + if ( + value instanceof XrpcResponseError && + TRANSIENT_UPSTREAM_ERRORS.has(value.error) + ) { + return true + } + + const message = String(value) + return TRANSIENT_UPSTREAM_MESSAGES.some(pattern => pattern.test(message)) +} + +/** Global backstop for automatic captures that bypass the logger transport. */ +export function dropExpectedNetworkErrors( + event: ErrorEvent, + hint: {originalException?: unknown}, +): ErrorEvent | null { + const candidates: unknown[] = [hint.originalException, event.message] + + for (const exception of event.exception?.values ?? []) { + candidates.push(exception.value) + if (exception.type && exception.value) { + candidates.push(`${exception.type}: ${exception.value}`) + } + } + + return candidates.some(isExpectedSentryNetworkError) ? null : event +} diff --git a/src/logger/sentry/setup/index.ts b/src/logger/sentry/setup/index.ts index d4607e08a1..1d0d6a936d 100644 --- a/src/logger/sentry/setup/index.ts +++ b/src/logger/sentry/setup/index.ts @@ -1,5 +1,6 @@ import {getGlobalScope, init} from '@sentry/react-native' +import {dropExpectedNetworkErrors} from '#/logger/sentry/network-errors' import * as env from '#/env' init({ @@ -10,6 +11,7 @@ init({ environment: env.ENV, dist: env.BUNDLE_IDENTIFIER, release: env.RELEASE_VERSION, + beforeSend: dropExpectedNetworkErrors, ignoreErrors: [ /* * Unknown internals errors diff --git a/src/logger/transports/sentry.ts b/src/logger/transports/sentry.ts index 92a7b92f67..9b5fd6d8ac 100644 --- a/src/logger/transports/sentry.ts +++ b/src/logger/transports/sentry.ts @@ -1,5 +1,5 @@ -import {isNetworkError} from '#/lib/strings/errors' import {Sentry} from '#/logger/sentry/lib' +import {isExpectedSentryNetworkError} from '#/logger/sentry/network-errors' import {LogLevel, type Transport} from '#/logger/types' import {prepareMetadata} from '#/logger/util' @@ -47,14 +47,14 @@ export const sentryTransport: Transport = ( timestamp: timestamp / 1000, // Sentry expects seconds }) - // We don't want to send any network errors to sentry. The underlying - // cause is often passed via metadata rather than the message itself, so - // check the common metadata keys too. + /* + * Keep the breadcrumb, but don't send expected network failures as events. + * The underlying cause is often passed in metadata rather than the message + * itself, and call sites do not consistently use the same metadata key. + */ if ( - isNetworkError(message) || - isNetworkError(metadata.safeMessage) || - isNetworkError(metadata.message) || - isNetworkError(metadata.error) + isExpectedSentryNetworkError(message) || + Object.values(metadata).some(isExpectedSentryNetworkError) ) { return } @@ -74,8 +74,10 @@ export const sentryTransport: Transport = ( }) } } else { - // We don't want to send any network errors to sentry - if (isNetworkError(message)) { + if ( + isExpectedSentryNetworkError(message) || + Object.values(metadata).some(isExpectedSentryNetworkError) + ) { return }