From 31cef090b1cd20c61a030b19b625b0c622565169 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 29 Aug 2023 16:50:39 +0200 Subject: [PATCH 01/29] ref(replay): Restructure event type export naming (#8866) This renames & restructures the type exports from `@sentry/replay`. We want to deprecate the `@sentry/replay` package eventually - users should import everything they need directly from e.g. `@sentry/browser` or `@sentry/react`. ref https://github.com/getsentry/sentry-javascript/issues/8864 --- .../utils/replayHelpers.ts | 8 +- packages/browser/src/index.ts | 12 ++ packages/replay/.eslintignore | 2 - packages/replay/.eslintrc.js | 6 + .../replay/src/coreHandlers/handleClick.ts | 12 +- packages/replay/src/index.ts | 18 ++- packages/replay/src/replay.ts | 11 +- packages/replay/src/types/deprecated.ts | 30 ++++ packages/replay/src/types/replay.ts | 6 +- packages/replay/src/types/replayFrame.ts | 138 +++++++++--------- packages/replay/src/types/rrweb.ts | 40 ++--- packages/replay/src/util/createBreadcrumb.ts | 6 +- .../replay/src/util/handleRecordingEmit.ts | 4 +- .../test/integration/errorSampleRate.test.ts | 44 +++--- .../test/integration/eventProcessors.test.ts | 5 +- .../replay/test/integration/events.test.ts | 19 +-- .../replay/test/integration/flush.test.ts | 11 +- .../test/integration/rateLimiting.test.ts | 8 +- .../test/integration/sendReplayEvent.test.ts | 15 +- .../replay/test/integration/session.test.ts | 23 ++- packages/replay/test/integration/stop.test.ts | 5 +- packages/replay/test/mocks/mockRrweb.ts | 7 +- .../unit/eventBuffer/EventBufferArray.test.ts | 18 +-- .../EventBufferCompressionWorker.test.ts | 18 +-- .../unit/eventBuffer/EventBufferProxy.test.ts | 3 +- .../replay/test/unit/util/addEvent.test.ts | 6 +- .../unit/util/handleRecordingEmit.test.ts | 4 +- packages/replay/test/utils/getTestEvent.ts | 26 ++++ 28 files changed, 295 insertions(+), 210 deletions(-) create mode 100644 packages/replay/src/types/deprecated.ts create mode 100644 packages/replay/test/utils/getTestEvent.ts diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index 5ddcf0c46e05..bbc02d50494f 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -1,12 +1,12 @@ import type { fullSnapshotEvent, incrementalSnapshotEvent } from '@sentry-internal/rrweb'; import { EventType } from '@sentry-internal/rrweb'; +import type { ReplayEventWithTime } from '@sentry/browser'; import type { InternalEventContext, RecordingEvent, ReplayContainer, Session, } from '@sentry/replay/build/npm/types/types'; -import type { eventWithTime } from '@sentry/replay/build/npm/types/types/rrweb'; import type { Breadcrumb, Event, ReplayEvent, ReplayRecordingMode } from '@sentry/types'; import pako from 'pako'; import type { Page, Request, Response } from 'playwright'; @@ -22,12 +22,12 @@ export type PerformanceSpan = { data: Record; }; -export type FullRecordingSnapshot = eventWithTime & { +export type FullRecordingSnapshot = ReplayEventWithTime & { timestamp: 0; data: fullSnapshotEvent['data']; }; -export type IncrementalRecordingSnapshot = eventWithTime & { +export type IncrementalRecordingSnapshot = ReplayEventWithTime & { timestamp: 0; data: incrementalSnapshotEvent['data']; }; @@ -270,7 +270,7 @@ function getOptionsEvents(replayRequest: Request): CustomRecordingEvent[] { export function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { const replayRequest = getRequest(resOrReq); return ( - (replayEnvelopeRequestParser(replayRequest, 5) as eventWithTime[]) + (replayEnvelopeRequestParser(replayRequest, 5) as ReplayEventWithTime[]) .sort((a, b) => a.timestamp - b.timestamp) // source 1 is MouseMove, which is a bit flaky and we don't care about .filter( diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 4ca6c7352261..60a2bef7a2f9 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -21,6 +21,18 @@ const INTEGRATIONS = { export { INTEGRATIONS as Integrations }; export { Replay } from '@sentry/replay'; +export type { + ReplayEventType, + ReplayEventWithTime, + ReplayBreadcrumbFrame, + ReplayBreadcrumbFrameEvent, + ReplayOptionFrameEvent, + ReplayFrame, + ReplayFrameEvent, + ReplaySpanFrame, + ReplaySpanFrameEvent, +} from '@sentry/replay'; + export { BrowserTracing, defaultRequestInstrumentationOptions, diff --git a/packages/replay/.eslintignore b/packages/replay/.eslintignore index 82f028e93347..c76c6c2d64d1 100644 --- a/packages/replay/.eslintignore +++ b/packages/replay/.eslintignore @@ -4,5 +4,3 @@ demo/build/ # TODO: Check if we can re-introduce linting in demo demo metrics -# For whatever reason, the eslint-ignore comment in this file is not working, so skipping this file -src/types/rrweb.ts diff --git a/packages/replay/.eslintrc.js b/packages/replay/.eslintrc.js index 6db928fcb1b9..4f69827ac50b 100644 --- a/packages/replay/.eslintrc.js +++ b/packages/replay/.eslintrc.js @@ -32,5 +32,11 @@ module.exports = { '@typescript-eslint/no-floating-promises': 'off', }, }, + { + files: ['src/types/deprecated.ts'], + rules: { + '@typescript-eslint/naming-convention': 'off', + }, + }, ], }; diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts index c4243c1e7e1b..91928ea6ac3f 100644 --- a/packages/replay/src/coreHandlers/handleClick.ts +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -1,7 +1,13 @@ import type { Breadcrumb } from '@sentry/types'; import { WINDOW } from '../constants'; -import type { MultiClickFrame, ReplayClickDetector, ReplayContainer, SlowClickConfig, SlowClickFrame } from '../types'; +import type { + ReplayClickDetector, + ReplayContainer, + ReplayMultiClickFrame, + ReplaySlowClickFrame, + SlowClickConfig, +} from '../types'; import { timestampToS } from '../util/timestamp'; import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; import { getClickTargetNode } from './util/domUtils'; @@ -216,7 +222,7 @@ export class ClickDetector implements ReplayClickDetector { const timeAfterClickMs = Math.min(click.mutationAfter || this._timeout, this._timeout) * 1000; const endReason = timeAfterClickMs < this._timeout * 1000 ? 'mutation' : 'timeout'; - const breadcrumb: SlowClickFrame = { + const breadcrumb: ReplaySlowClickFrame = { type: 'default', message: clickBreadcrumb.message, timestamp: clickBreadcrumb.timestamp, @@ -239,7 +245,7 @@ export class ClickDetector implements ReplayClickDetector { // Multi click if (clickCount > 1) { - const breadcrumb: MultiClickFrame = { + const breadcrumb: ReplayMultiClickFrame = { type: 'default', message: clickBreadcrumb.message, timestamp: clickBreadcrumb.timestamp, diff --git a/packages/replay/src/index.ts b/packages/replay/src/index.ts index 3b05202fc541..412354f1dc54 100644 --- a/packages/replay/src/index.ts +++ b/packages/replay/src/index.ts @@ -1,12 +1,16 @@ export { Replay } from './integration'; + export type { - EventType, - eventWithTime, - BreadcrumbFrame, - BreadcrumbFrameEvent, - OptionFrameEvent, + ReplayEventType, + ReplayEventWithTime, + ReplayBreadcrumbFrame, + ReplayBreadcrumbFrameEvent, + ReplayOptionFrameEvent, ReplayFrame, ReplayFrameEvent, - SpanFrame, - SpanFrameEvent, + ReplaySpanFrame, + ReplaySpanFrameEvent, } from './types'; + +// TODO (v8): Remove deprecated types +export * from './types/deprecated'; diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index eec3edf45083..a96910e02f31 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -25,12 +25,12 @@ import type { AddEventResult, AddUpdateCallback, AllPerformanceEntry, - BreadcrumbFrame, EventBuffer, InternalEventContext, PopEventContext, RecordingEvent, RecordingOptions, + ReplayBreadcrumbFrame, ReplayContainer as ReplayContainerInterface, ReplayPluginOptions, SendBufferedReplayOptions, @@ -38,6 +38,7 @@ import type { SlowClickConfig, Timeouts, } from './types'; +import { ReplayEventTypeCustom } from './types'; import { addEvent } from './util/addEvent'; import { addGlobalListeners } from './util/addGlobalListeners'; import { addMemoryEntry } from './util/addMemoryEntry'; @@ -688,7 +689,7 @@ export class ReplayContainer implements ReplayContainerInterface { this.addUpdate(() => { void addEvent(this, { - type: EventType.Custom, + type: ReplayEventTypeCustom, timestamp: breadcrumb.timestamp || 0, data: { tag: 'breadcrumb', @@ -919,7 +920,7 @@ export class ReplayContainer implements ReplayContainerInterface { /** * Tasks to run when we consider a page to be hidden (via blurring and/or visibility) */ - private _doChangeToBackgroundTasks(breadcrumb?: BreadcrumbFrame): void { + private _doChangeToBackgroundTasks(breadcrumb?: ReplayBreadcrumbFrame): void { if (!this.session) { return; } @@ -939,7 +940,7 @@ export class ReplayContainer implements ReplayContainerInterface { /** * Tasks to run when we consider a page to be visible (via focus and/or visibility) */ - private _doChangeToForegroundTasks(breadcrumb?: BreadcrumbFrame): void { + private _doChangeToForegroundTasks(breadcrumb?: ReplayBreadcrumbFrame): void { if (!this.session) { return; } @@ -992,7 +993,7 @@ export class ReplayContainer implements ReplayContainerInterface { /** * Helper to create (and buffer) a replay breadcrumb from a core SDK breadcrumb */ - private _createCustomBreadcrumb(breadcrumb: BreadcrumbFrame): void { + private _createCustomBreadcrumb(breadcrumb: ReplayBreadcrumbFrame): void { this.addUpdate(() => { void this.throttledAddEvent({ type: EventType.Custom, diff --git a/packages/replay/src/types/deprecated.ts b/packages/replay/src/types/deprecated.ts new file mode 100644 index 000000000000..ff57070bf2ee --- /dev/null +++ b/packages/replay/src/types/deprecated.ts @@ -0,0 +1,30 @@ +import type { + ReplayBreadcrumbFrame, + ReplayBreadcrumbFrameEvent, + ReplayEventType, + ReplayEventWithTime, + ReplayOptionFrameEvent, + ReplaySpanFrame, + ReplaySpanFrameEvent, +} from '.'; + +/** @deprecated use ReplayEventType instead */ +export type EventType = ReplayEventType; + +/** @deprecated use ReplayEventWithTime instead */ +export type eventWithTime = ReplayEventWithTime; + +/** @deprecated use ReplayBreadcrumbFrame instead */ +export type BreadcrumbFrame = ReplayBreadcrumbFrame; + +/** @deprecated use ReplayBreadcrumbFrameEvent instead */ +export type BreadcrumbFrameEvent = ReplayBreadcrumbFrameEvent; + +/** @deprecated use ReplayOptionFrameEvent instead */ +export type OptionFrameEvent = ReplayOptionFrameEvent; + +/** @deprecated use ReplaySpanFrame instead */ +export type SpanFrame = ReplaySpanFrame; + +/** @deprecated use ReplaySpanFrameEvent instead */ +export type SpanFrameEvent = ReplaySpanFrameEvent; diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts index 00f363003979..1ae48dfecfc3 100644 --- a/packages/replay/src/types/replay.ts +++ b/packages/replay/src/types/replay.ts @@ -13,10 +13,10 @@ import type { SKIPPED, THROTTLED } from '../util/throttle'; import type { AllPerformanceEntry } from './performance'; import type { ReplayFrameEvent } from './replayFrame'; import type { ReplayNetworkRequestOrResponse } from './request'; -import type { eventWithTime, recordOptions } from './rrweb'; +import type { ReplayEventWithTime, RrwebRecordOptions } from './rrweb'; -export type RecordingEvent = ReplayFrameEvent | eventWithTime; -export type RecordingOptions = recordOptions; +export type RecordingEvent = ReplayFrameEvent | ReplayEventWithTime; +export type RecordingOptions = RrwebRecordOptions; export interface SendReplayData { recordingData: ReplayRecordingData; diff --git a/packages/replay/src/types/replayFrame.ts b/packages/replay/src/types/replayFrame.ts index 5eca0be68466..296556b6c6d0 100644 --- a/packages/replay/src/types/replayFrame.ts +++ b/packages/replay/src/types/replayFrame.ts @@ -9,11 +9,11 @@ import type { PaintData, ResourceData, } from './performance'; -import type { EventType } from './rrweb'; +import type { ReplayEventTypeCustom } from './rrweb'; type AnyRecord = Record; // eslint-disable-line @typescript-eslint/no-explicit-any -interface BaseBreadcrumbFrame { +interface ReplayBaseBreadcrumbFrame { timestamp: number; /** * For compatibility reasons @@ -24,7 +24,7 @@ interface BaseBreadcrumbFrame { message?: string; } -interface BaseDomFrameData { +interface ReplayBaseDomFrameData { nodeId?: number; node?: { id: number; @@ -35,84 +35,84 @@ interface BaseDomFrameData { } /* Breadcrumbs from Core SDK */ -interface ConsoleFrameData { +interface ReplayConsoleFrameData { logger: string; arguments?: unknown[]; } -interface ConsoleFrame extends BaseBreadcrumbFrame { +interface ReplayConsoleFrame extends ReplayBaseBreadcrumbFrame { category: 'console'; level: Breadcrumb['level']; message: string; - data: ConsoleFrameData; + data: ReplayConsoleFrameData; } -type ClickFrameData = BaseDomFrameData; -interface ClickFrame extends BaseBreadcrumbFrame { +type ReplayClickFrameData = ReplayBaseDomFrameData; +interface ReplayClickFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.click'; message: string; - data: ClickFrameData; + data: ReplayClickFrameData; } -interface InputFrame extends BaseBreadcrumbFrame { +interface ReplayInputFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.input'; message: string; } /* Breadcrumbs from Replay */ -interface MutationFrameData { +interface ReplayMutationFrameData { count: number; limit: boolean; } -interface MutationFrame extends BaseBreadcrumbFrame { +interface ReplayMutationFrame extends ReplayBaseBreadcrumbFrame { category: 'replay.mutations'; - data: MutationFrameData; + data: ReplayMutationFrameData; } -interface KeyboardEventFrameData extends BaseDomFrameData { +interface ReplayKeyboardEventFrameData extends ReplayBaseDomFrameData { metaKey: boolean; shiftKey: boolean; ctrlKey: boolean; altKey: boolean; key: string; } -interface KeyboardEventFrame extends BaseBreadcrumbFrame { +interface ReplayKeyboardEventFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.keyDown'; - data: KeyboardEventFrameData; + data: ReplayKeyboardEventFrameData; } -interface BlurFrame extends BaseBreadcrumbFrame { +interface ReplayBlurFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.blur'; } -interface FocusFrame extends BaseBreadcrumbFrame { +interface ReplayFocusFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.focus'; } -interface SlowClickFrameData extends ClickFrameData { +interface ReplaySlowClickFrameData extends ReplayClickFrameData { url: string; route?: string; timeAfterClickMs: number; endReason: string; clickCount?: number; } -export interface SlowClickFrame extends BaseBreadcrumbFrame { +export interface ReplaySlowClickFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.slowClickDetected'; - data: SlowClickFrameData; + data: ReplaySlowClickFrameData; } -interface MultiClickFrameData extends ClickFrameData { +interface ReplayMultiClickFrameData extends ReplayClickFrameData { url: string; route?: string; clickCount: number; metric: true; } -export interface MultiClickFrame extends BaseBreadcrumbFrame { +export interface ReplayMultiClickFrame extends ReplayBaseBreadcrumbFrame { category: 'ui.multiClick'; - data: MultiClickFrameData; + data: ReplayMultiClickFrameData; } -interface OptionFrame { +interface ReplayOptionFrame { blockAllMedia: boolean; errorSampleRate: number; maskAllInputs: boolean; @@ -126,19 +126,19 @@ interface OptionFrame { useCompressionOption: boolean; } -export type BreadcrumbFrame = - | ConsoleFrame - | ClickFrame - | InputFrame - | KeyboardEventFrame - | BlurFrame - | FocusFrame - | SlowClickFrame - | MultiClickFrame - | MutationFrame - | BaseBreadcrumbFrame; +export type ReplayBreadcrumbFrame = + | ReplayConsoleFrame + | ReplayClickFrame + | ReplayInputFrame + | ReplayKeyboardEventFrame + | ReplayBlurFrame + | ReplayFocusFrame + | ReplaySlowClickFrame + | ReplayMultiClickFrame + | ReplayMutationFrame + | ReplayBaseBreadcrumbFrame; -interface BaseSpanFrame { +interface ReplayBaseSpanFrame { op: string; description: string; startTimestamp: number; @@ -146,55 +146,61 @@ interface BaseSpanFrame { data?: undefined | AnyRecord; } -interface HistoryFrame extends BaseSpanFrame { +interface ReplayHistoryFrame extends ReplayBaseSpanFrame { data: HistoryData; op: 'navigation.push'; } -interface LargestContentfulPaintFrame extends BaseSpanFrame { +interface ReplayLargestContentfulPaintFrame extends ReplayBaseSpanFrame { data: LargestContentfulPaintData; op: 'largest-contentful-paint'; } -interface MemoryFrame extends BaseSpanFrame { +interface ReplayMemoryFrame extends ReplayBaseSpanFrame { data: MemoryData; op: 'memory'; } -interface NavigationFrame extends BaseSpanFrame { +interface ReplayNavigationFrame extends ReplayBaseSpanFrame { data: NavigationData; op: 'navigation.navigate' | 'navigation.reload' | 'navigation.back_forward'; } -interface PaintFrame extends BaseSpanFrame { +interface ReplayPaintFrame extends ReplayBaseSpanFrame { data: PaintData; op: 'paint'; } -interface RequestFrame extends BaseSpanFrame { +interface ReplayRequestFrame extends ReplayBaseSpanFrame { data: NetworkRequestData; op: 'resource.fetch' | 'resource.xhr'; } -interface ResourceFrame extends BaseSpanFrame { +interface ReplayResourceFrame extends ReplayBaseSpanFrame { data: ResourceData; - op: 'resource.css' | 'resource.iframe' | 'resource.img' | 'resource.link' | 'resource.other' | 'resource.script'; -} - -export type SpanFrame = - | BaseSpanFrame - | HistoryFrame - | RequestFrame - | LargestContentfulPaintFrame - | MemoryFrame - | NavigationFrame - | PaintFrame - | ResourceFrame; - -export type ReplayFrame = BreadcrumbFrame | SpanFrame; + op: + | 'resource.css' + | 'resource.ReplayiFrame' + | 'resource.img' + | 'resource.link' + | 'resource.other' + | 'resource.script'; +} + +export type ReplaySpanFrame = + | ReplayBaseSpanFrame + | ReplayHistoryFrame + | ReplayRequestFrame + | ReplayLargestContentfulPaintFrame + | ReplayMemoryFrame + | ReplayNavigationFrame + | ReplayPaintFrame + | ReplayResourceFrame; + +export type ReplayFrame = ReplayBreadcrumbFrame | ReplaySpanFrame; interface RecordingCustomEvent { - type: EventType.Custom; + type: typeof ReplayEventTypeCustom; timestamp: number; data: { tag: string; @@ -202,10 +208,10 @@ interface RecordingCustomEvent { }; } -export interface BreadcrumbFrameEvent extends RecordingCustomEvent { +export interface ReplayBreadcrumbFrameEvent extends RecordingCustomEvent { data: { tag: 'breadcrumb'; - payload: BreadcrumbFrame; + payload: ReplayBreadcrumbFrame; /** * This will indicate to backend to additionally log as a metric */ @@ -213,18 +219,18 @@ export interface BreadcrumbFrameEvent extends RecordingCustomEvent { }; } -export interface SpanFrameEvent extends RecordingCustomEvent { +export interface ReplaySpanFrameEvent extends RecordingCustomEvent { data: { tag: 'performanceSpan'; - payload: SpanFrame; + payload: ReplaySpanFrame; }; } -export interface OptionFrameEvent extends RecordingCustomEvent { +export interface ReplayOptionFrameEvent extends RecordingCustomEvent { data: { tag: 'options'; - payload: OptionFrame; + payload: ReplayOptionFrame; }; } -export type ReplayFrameEvent = BreadcrumbFrameEvent | SpanFrameEvent | OptionFrameEvent; +export type ReplayFrameEvent = ReplayBreadcrumbFrameEvent | ReplaySpanFrameEvent | ReplayOptionFrameEvent; diff --git a/packages/replay/src/types/rrweb.ts b/packages/replay/src/types/rrweb.ts index 75a596fb4385..bc78c5811b12 100644 --- a/packages/replay/src/types/rrweb.ts +++ b/packages/replay/src/types/rrweb.ts @@ -1,25 +1,29 @@ -/* eslint-disable @typescript-eslint/naming-convention */ - -type blockClass = string | RegExp; -type maskTextClass = string | RegExp; +type ClassOption = string | RegExp; /** Duplicate this from @sentry-internal/rrweb so we can export this as well. */ -export enum EventType { - DomContentLoaded = 0, - Load = 1, - FullSnapshot = 2, - IncrementalSnapshot = 3, - Meta = 4, - Custom = 5, - Plugin = 6, -} +export const ReplayEventTypeDomContentLoaded = 0; +export const ReplayEventTypeLoad = 1; +export const ReplayEventTypeFullSnapshot = 2; +export const ReplayEventTypeIncrementalSnapshot = 3; +export const ReplayEventTypeMeta = 4; +export const ReplayEventTypeCustom = 5; +export const ReplayEventTypePlugin = 6; + +export type ReplayEventType = + | typeof ReplayEventTypeDomContentLoaded + | typeof ReplayEventTypeLoad + | typeof ReplayEventTypeFullSnapshot + | typeof ReplayEventTypeIncrementalSnapshot + | typeof ReplayEventTypeMeta + | typeof ReplayEventTypeCustom + | typeof ReplayEventTypePlugin; /** * This is a partial copy of rrweb's eventWithTime type which only contains the properties * we specifcally need in the SDK. */ -export type eventWithTime = { - type: EventType; +export type ReplayEventWithTime = { + type: ReplayEventType; data: unknown; timestamp: number; delay?: number; @@ -30,12 +34,12 @@ export type eventWithTime = { * we specifically us in the SDK. Users can specify additional properties, hence we add the * Record union type. */ -export type recordOptions = { +export type RrwebRecordOptions = { maskAllText?: boolean; maskAllInputs?: boolean; - blockClass?: blockClass; + blockClass?: ClassOption; ignoreClass?: string; - maskTextClass?: maskTextClass; + maskTextClass?: ClassOption; maskTextSelector?: string; blockSelector?: string; maskInputOptions?: Record; diff --git a/packages/replay/src/util/createBreadcrumb.ts b/packages/replay/src/util/createBreadcrumb.ts index 5cf044333876..6f053f08617b 100644 --- a/packages/replay/src/util/createBreadcrumb.ts +++ b/packages/replay/src/util/createBreadcrumb.ts @@ -1,11 +1,11 @@ -import type { BreadcrumbFrame } from '../types/replayFrame'; +import type { ReplayBreadcrumbFrame } from '../types/replayFrame'; /** * Create a breadcrumb for a replay. */ export function createBreadcrumb( - breadcrumb: Omit & Partial>, -): BreadcrumbFrame { + breadcrumb: Omit & Partial>, +): ReplayBreadcrumbFrame { return { timestamp: Date.now() / 1000, type: 'default', diff --git a/packages/replay/src/util/handleRecordingEmit.ts b/packages/replay/src/util/handleRecordingEmit.ts index 9e22e57f6255..f8f00c820ad9 100644 --- a/packages/replay/src/util/handleRecordingEmit.ts +++ b/packages/replay/src/util/handleRecordingEmit.ts @@ -2,7 +2,7 @@ import { EventType } from '@sentry-internal/rrweb'; import { logger } from '@sentry/utils'; import { saveSession } from '../session/saveSession'; -import type { AddEventResult, OptionFrameEvent, RecordingEvent, ReplayContainer } from '../types'; +import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayOptionFrameEvent } from '../types'; import { addEvent } from './addEvent'; import { logInfo } from './log'; @@ -102,7 +102,7 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa /** * Exported for tests */ -export function createOptionsEvent(replay: ReplayContainer): OptionFrameEvent { +export function createOptionsEvent(replay: ReplayContainer): ReplayOptionFrameEvent { const options = replay.getOptions(); return { type: EventType.Custom, diff --git a/packages/replay/test/integration/errorSampleRate.test.ts b/packages/replay/test/integration/errorSampleRate.test.ts index e56edae0f723..0db920894291 100644 --- a/packages/replay/test/integration/errorSampleRate.test.ts +++ b/packages/replay/test/integration/errorSampleRate.test.ts @@ -17,6 +17,7 @@ import type { RecordMock } from '../index'; import { BASE_TIMESTAMP } from '../index'; import { resetSdkMock } from '../mocks/resetSdkMock'; import type { DomHandler } from '../types'; +import { getTestEventCheckout, getTestEventIncremental } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -58,7 +59,7 @@ describe('Integration | errorSampleRate', () => { }); it('uploads a replay when `Sentry.captureException` is called and continues recording', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); const optionsEvent = createOptionsEvent(replay); @@ -144,7 +145,7 @@ describe('Integration | errorSampleRate', () => { }); it('manually flushes replay and does not continue to record', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); const optionsEvent = createOptionsEvent(replay); @@ -227,7 +228,7 @@ describe('Integration | errorSampleRate', () => { }); it('handles multiple simultaneous flushes', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); const optionsEvent = createOptionsEvent(replay); @@ -368,7 +369,7 @@ describe('Integration | errorSampleRate', () => { const ELAPSED = 5000; jest.advanceTimersByTime(ELAPSED); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); addEvent(replay, TEST_EVENT); document.dispatchEvent(new Event('visibilitychange')); @@ -381,7 +382,7 @@ describe('Integration | errorSampleRate', () => { }); it('does not upload a replay event if 5 seconds have elapsed since the last replay event occurred', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); // Pretend 5 seconds have passed const ELAPSED = 5000; @@ -396,7 +397,7 @@ describe('Integration | errorSampleRate', () => { }); it('does not upload a replay event if 15 seconds have elapsed since the last replay upload', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); // Fire a new event every 4 seconds, 4 times [...Array(4)].forEach(() => { mockRecord._emitter(TEST_EVENT); @@ -462,11 +463,10 @@ describe('Integration | errorSampleRate', () => { jest.advanceTimersByTime(waitTime + 1); await new Promise(process.nextTick); - const TEST_EVENT = { + const TEST_EVENT = getTestEventIncremental({ data: { name: 'lost event' }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT); jest.runAllTimers(); @@ -506,11 +506,10 @@ describe('Integration | errorSampleRate', () => { jest.advanceTimersByTime(waitTime + 1); await new Promise(process.nextTick); - const TEST_EVENT = { + const TEST_EVENT = getTestEventIncremental({ data: { name: 'lost event' }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT); jest.runAllTimers(); @@ -566,11 +565,10 @@ describe('Integration | errorSampleRate', () => { // Idle for 15 minutes jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1); - const TEST_EVENT = { + const TEST_EVENT = getTestEventIncremental({ data: { name: 'lost event' }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT); expect(replay).not.toHaveLastSentReplay(); @@ -615,7 +613,7 @@ describe('Integration | errorSampleRate', () => { }); it('has the correct timestamps with deferred root event and last replay update', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); const optionsEvent = createOptionsEvent(replay); @@ -658,7 +656,7 @@ describe('Integration | errorSampleRate', () => { it('has correct timestamps when error occurs much later than initial pageload/checkout', async () => { const ELAPSED = BUFFER_CHECKOUT_TIME; const TICK = 20; - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); // add a mock performance event @@ -710,7 +708,7 @@ describe('Integration | errorSampleRate', () => { it('stops replay when user goes idle', async () => { jest.setSystemTime(BASE_TIMESTAMP); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); @@ -753,7 +751,7 @@ describe('Integration | errorSampleRate', () => { const sessionId = replay.session?.id; jest.setSystemTime(BASE_TIMESTAMP); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); @@ -821,7 +819,7 @@ describe('Integration | errorSampleRate', () => { it('does not stop replay based on earliest event in buffer', async () => { jest.setSystemTime(BASE_TIMESTAMP); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP - 60000, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP - 60000 }); mockRecord._emitter(TEST_EVENT); expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); @@ -955,7 +953,7 @@ it('handles buffer sessions that previously had an error', async () => { jest.runAllTimers(); await new Promise(process.nextTick); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); expect(replay).not.toHaveLastSentReplay(); @@ -992,7 +990,7 @@ it('handles buffer sessions that never had an error', async () => { jest.runAllTimers(); await new Promise(process.nextTick); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); expect(replay).not.toHaveLastSentReplay(); @@ -1037,7 +1035,7 @@ it('sends a replay after loading the session from storage', async () => { jest.runAllTimers(); await new Promise(process.nextTick); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); expect(replay).not.toHaveLastSentReplay(); diff --git a/packages/replay/test/integration/eventProcessors.test.ts b/packages/replay/test/integration/eventProcessors.test.ts index 363c5f2d5a74..b9c86a5c5966 100644 --- a/packages/replay/test/integration/eventProcessors.test.ts +++ b/packages/replay/test/integration/eventProcessors.test.ts @@ -3,6 +3,7 @@ import type { Event, Hub, Scope } from '@sentry/types'; import { BASE_TIMESTAMP } from '..'; import { resetSdkMock } from '../mocks/resetSdkMock'; +import { getTestEventIncremental } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -48,7 +49,7 @@ describe('Integration | eventProcessors', () => { scope.addEventProcessor(handler1); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); jest.runAllTimers(); @@ -59,7 +60,7 @@ describe('Integration | eventProcessors', () => { scope.addEventProcessor(handler2); - const TEST_EVENT2 = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT2 = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT2); jest.runAllTimers(); diff --git a/packages/replay/test/integration/events.test.ts b/packages/replay/test/integration/events.test.ts index c90f8ceed125..ab71f57c9c52 100644 --- a/packages/replay/test/integration/events.test.ts +++ b/packages/replay/test/integration/events.test.ts @@ -8,6 +8,7 @@ import { PerformanceEntryResource } from '../fixtures/performanceEntry/resource' import type { RecordMock } from '../index'; import { BASE_TIMESTAMP } from '../index'; import { resetSdkMock } from '../mocks/resetSdkMock'; +import { getTestEventCheckout } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -74,11 +75,9 @@ describe('Integration | events', () => { const ELAPSED = 5000; await advanceTimers(ELAPSED); - const TEST_EVENT = { - data: {}, + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP + ELAPSED, - type: 2, - }; + }); addEvent(replay, TEST_EVENT); WINDOW.dispatchEvent(new Event('blur')); @@ -112,11 +111,9 @@ describe('Integration | events', () => { const ELAPSED = 5000; await advanceTimers(ELAPSED); - const TEST_EVENT = { - data: {}, + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP + ELAPSED, - type: 2, - }; + }); addEvent(replay, TEST_EVENT); @@ -156,11 +153,9 @@ describe('Integration | events', () => { expect(mockTransportSend).toHaveBeenCalledTimes(0); // A new checkout occurs (i.e. a new session was started) - const TEST_EVENT = { - data: {}, + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP, - type: 2, - }; + }); addEvent(replay, TEST_EVENT); // This event will trigger a flush diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index 6f2d3b7d8ccd..e78032daaf8f 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -9,6 +9,7 @@ import { createPerformanceEntries } from '../../src/util/createPerformanceEntrie import { createPerformanceSpans } from '../../src/util/createPerformanceSpans'; import * as SendReplay from '../../src/util/sendReplay'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; +import { getTestEventCheckout } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -261,7 +262,7 @@ describe('Integration | flush', () => { }); // checkout - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); @@ -286,7 +287,7 @@ describe('Integration | flush', () => { }); // checkout - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); @@ -326,7 +327,7 @@ describe('Integration | flush', () => { }); // checkout - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); @@ -415,7 +416,7 @@ describe('Integration | flush', () => { replay.eventBuffer!.hasCheckout = true; // Add event that is too long after session start - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP + MAX_SESSION_LIFE + 100, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP + MAX_SESSION_LIFE + 100 }); mockRecord._emitter(TEST_EVENT); // no checkout! @@ -476,7 +477,7 @@ describe('Integration | flush', () => { }; // Add event inside of session life timespan - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP + 100, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP + 100 }); mockRecord._emitter(TEST_EVENT); await advanceTimers(160_000); diff --git a/packages/replay/test/integration/rateLimiting.test.ts b/packages/replay/test/integration/rateLimiting.test.ts index 291a95c4f94e..1bbaf9e66adf 100644 --- a/packages/replay/test/integration/rateLimiting.test.ts +++ b/packages/replay/test/integration/rateLimiting.test.ts @@ -7,6 +7,7 @@ import { clearSession } from '../../src/session/clearSession'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockSdk } from '../index'; import { mockRrweb } from '../mocks/mockRrweb'; +import { getTestEventCheckout, getTestEventIncremental } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -68,7 +69,7 @@ describe('Integration | rate-limiting behaviour', () => { expect(replay.session?.segmentId).toBe(0); jest.spyOn(replay, 'stop'); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); mockTransportSend.mockImplementationOnce(() => { return Promise.resolve({ statusCode: 429 }); @@ -99,11 +100,10 @@ describe('Integration | rate-limiting behaviour', () => { expect(mockTransportSend).toHaveBeenCalledTimes(1); // and let's also emit a new event and check that it is not recorded - const TEST_EVENT3 = { + const TEST_EVENT3 = getTestEventIncremental({ data: {}, timestamp: BASE_TIMESTAMP + 7 * DEFAULT_FLUSH_MIN_DELAY, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT3); // T = base + 80 diff --git a/packages/replay/test/integration/sendReplayEvent.test.ts b/packages/replay/test/integration/sendReplayEvent.test.ts index d6f26db6653c..4aef1ee94776 100644 --- a/packages/replay/test/integration/sendReplayEvent.test.ts +++ b/packages/replay/test/integration/sendReplayEvent.test.ts @@ -8,6 +8,7 @@ import { clearSession } from '../../src/session/clearSession'; import { addEvent } from '../../src/util/addEvent'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; +import { getTestEventCheckout, getTestEventIncremental } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -89,7 +90,7 @@ describe('Integration | sendReplayEvent', () => { const ELAPSED = 5000; jest.advanceTimersByTime(ELAPSED); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); addEvent(replay, TEST_EVENT); document.dispatchEvent(new Event('visibilitychange')); @@ -150,7 +151,7 @@ describe('Integration | sendReplayEvent', () => { }); it('uploads a replay event if 5 seconds have elapsed since the last replay event occurred', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); mockRecord._emitter(TEST_EVENT); // Pretend 5 seconds have passed const ELAPSED = 5000; @@ -169,7 +170,7 @@ describe('Integration | sendReplayEvent', () => { }); it('uploads a replay event if maxFlushDelay is set 15 seconds have elapsed since the last replay upload', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); // Fire a new event every 4 seconds, 4 times for (let i = 0; i < 4; i++) { mockRecord._emitter(TEST_EVENT); @@ -214,7 +215,7 @@ describe('Integration | sendReplayEvent', () => { const ELAPSED = 5000; jest.advanceTimersByTime(ELAPSED); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); const hiddenBreadcrumb = { type: 5, timestamp: +new Date(BASE_TIMESTAMP + ELAPSED) / 1000, @@ -252,7 +253,7 @@ describe('Integration | sendReplayEvent', () => { const ELAPSED = 5000; jest.advanceTimersByTime(ELAPSED); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); addEvent(replay, TEST_EVENT); document.dispatchEvent(new Event('visibilitychange')); @@ -302,7 +303,7 @@ describe('Integration | sendReplayEvent', () => { it('fails to upload data on first two calls and succeeds on the third', async () => { expect(replay.session?.segmentId).toBe(0); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); // Suppress console.errors const mockConsole = jest.spyOn(console, 'error').mockImplementation(jest.fn()); @@ -351,7 +352,7 @@ describe('Integration | sendReplayEvent', () => { }); it('fails to upload data and hits retry max and stops', async () => { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); const spyHandleException = jest.spyOn(SentryCore, 'captureException'); diff --git a/packages/replay/test/integration/session.test.ts b/packages/replay/test/integration/session.test.ts index 6e62b71ca09c..b0ea37e4e0f2 100644 --- a/packages/replay/test/integration/session.test.ts +++ b/packages/replay/test/integration/session.test.ts @@ -18,6 +18,7 @@ import { createOptionsEvent } from '../../src/util/handleRecordingEmit'; import { BASE_TIMESTAMP } from '../index'; import type { RecordMock } from '../mocks/mockRrweb'; import { resetSdkMock } from '../mocks/resetSdkMock'; +import { getTestEventCheckout, getTestEventIncremental } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -140,11 +141,10 @@ describe('Integration | session', () => { // Session has become in an idle state // // This event will put the Replay SDK into a paused state - const TEST_EVENT = { + const TEST_EVENT = getTestEventIncremental({ data: { name: 'lost event' }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT); // performance events can still be collected while recording is stopped @@ -260,11 +260,10 @@ describe('Integration | session', () => { // Session has become in an idle state // // This event will put the Replay SDK into a paused state - const TEST_EVENT = { + const TEST_EVENT = getTestEventIncremental({ data: { name: 'lost event' }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT); // performance events can still be collected while recording is stopped @@ -359,11 +358,10 @@ describe('Integration | session', () => { replay['_updateSessionActivity'](); // This should trigger a new session - const TEST_EVENT = { + const TEST_EVENT = getTestEventIncremental({ data: { name: 'lost event' }, timestamp: ELAPSED, - type: 3, - }; + }); mockRecord._emitter(TEST_EVENT); expect(replay).not.toHaveSameSession(initialSession); @@ -379,11 +377,10 @@ describe('Integration | session', () => { const newTimestamp = BASE_TIMESTAMP + ELAPSED; - const NEW_TEST_EVENT = { + const NEW_TEST_EVENT = getTestEventIncremental({ data: { name: 'test' }, timestamp: newTimestamp + DEFAULT_FLUSH_MIN_DELAY + 20, - type: 3, - }; + }); mockRecord._emitter(NEW_TEST_EVENT); const optionsEvent = createOptionsEvent(replay); @@ -438,7 +435,7 @@ describe('Integration | session', () => { const ELAPSED = 5000; await advanceTimers(ELAPSED); - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP }); addEvent(replay, TEST_EVENT); WINDOW.dispatchEvent(new Event('blur')); diff --git a/packages/replay/test/integration/stop.test.ts b/packages/replay/test/integration/stop.test.ts index a88c5de6a839..cdc980ae5b62 100644 --- a/packages/replay/test/integration/stop.test.ts +++ b/packages/replay/test/integration/stop.test.ts @@ -8,6 +8,7 @@ import { addEvent } from '../../src/util/addEvent'; import { createOptionsEvent } from '../../src/util/handleRecordingEmit'; // mock functions need to be imported first import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; +import { getTestEventIncremental } from '../utils/getTestEvent'; import { useFakeTimers } from '../utils/use-fake-timers'; useFakeTimers(); @@ -76,7 +77,7 @@ describe('Integration | stop', () => { const ELAPSED = 5000; // Not sure where the 20ms comes from tbh const EXTRA_TICKS = 20; - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); const previousSessionId = replay.session?.id; // stop replays @@ -144,7 +145,7 @@ describe('Integration | stop', () => { }); it('does not buffer new events after being stopped', async function () { - const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); addEvent(replay, TEST_EVENT); expect(replay.eventBuffer?.hasEvents).toBe(true); expect(mockRunFlush).toHaveBeenCalledTimes(0); diff --git a/packages/replay/test/mocks/mockRrweb.ts b/packages/replay/test/mocks/mockRrweb.ts index 0c7ef971483d..c6fbd2be6e55 100644 --- a/packages/replay/test/mocks/mockRrweb.ts +++ b/packages/replay/test/mocks/mockRrweb.ts @@ -1,6 +1,7 @@ import type { record as rrwebRecord } from '@sentry-internal/rrweb'; -import type { RecordingEvent } from '../../src/types'; +import type { RecordingEvent, ReplayEventWithTime } from '../../src/types'; +import { ReplayEventTypeFullSnapshot, ReplayEventTypeIncrementalSnapshot } from '../../src/types'; type RecordAdditionalProperties = { takeFullSnapshot: jest.Mock; @@ -16,11 +17,11 @@ type RecordAdditionalProperties = { export type RecordMock = jest.MockedFunction & RecordAdditionalProperties; -function createCheckoutPayload(isCheckout: boolean = true) { +function createCheckoutPayload(isCheckout: boolean = true): ReplayEventWithTime { return { data: { isCheckout }, timestamp: Date.now(), - type: isCheckout ? 2 : 3, + type: isCheckout ? ReplayEventTypeFullSnapshot : ReplayEventTypeIncrementalSnapshot, }; } diff --git a/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts index 494d03e9572f..8c8d9a7e99e5 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts @@ -2,8 +2,9 @@ import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; import { createEventBuffer } from '../../../src/eventBuffer'; import { EventBufferSizeExceededError } from '../../../src/eventBuffer/error'; import { BASE_TIMESTAMP } from '../../index'; +import { getTestEventIncremental } from '../../utils/getTestEvent'; -const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; +const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); describe('Unit | eventBuffer | EventBufferArray', () => { it('adds events to normal event buffer', async function () { @@ -51,11 +52,10 @@ describe('Unit | eventBuffer | EventBufferArray', () => { it('rejects if size exceeds limit', async function () { const buffer = createEventBuffer({ useCompression: false }); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await buffer.addEvent(largeEvent); await buffer.addEvent(largeEvent); @@ -67,11 +67,10 @@ describe('Unit | eventBuffer | EventBufferArray', () => { it('resets size limit on clear', async function () { const buffer = createEventBuffer({ useCompression: false }); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await buffer.addEvent(largeEvent); await buffer.addEvent(largeEvent); @@ -84,11 +83,10 @@ describe('Unit | eventBuffer | EventBufferArray', () => { it('resets size limit on finish', async function () { const buffer = createEventBuffer({ useCompression: false }); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await buffer.addEvent(largeEvent); await buffer.addEvent(largeEvent); diff --git a/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts index cab6855e411d..297744389cf6 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts @@ -7,8 +7,9 @@ import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; import { createEventBuffer } from '../../../src/eventBuffer'; import { EventBufferSizeExceededError } from '../../../src/eventBuffer/error'; import { EventBufferProxy } from '../../../src/eventBuffer/EventBufferProxy'; +import { getTestEventIncremental } from '../../utils/getTestEvent'; -const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; +const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); describe('Unit | eventBuffer | EventBufferCompressionWorker', () => { it('adds events to event buffer with compression worker', async function () { @@ -158,11 +159,10 @@ describe('Unit | eventBuffer | EventBufferCompressionWorker', () => { expect(buffer).toBeInstanceOf(EventBufferProxy); await buffer.ensureWorkerIsLoaded(); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await buffer.addEvent(largeEvent); await buffer.addEvent(largeEvent); @@ -179,11 +179,10 @@ describe('Unit | eventBuffer | EventBufferCompressionWorker', () => { expect(buffer).toBeInstanceOf(EventBufferProxy); await buffer.ensureWorkerIsLoaded(); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await buffer.addEvent(largeEvent); await buffer.addEvent(largeEvent); @@ -201,11 +200,10 @@ describe('Unit | eventBuffer | EventBufferCompressionWorker', () => { expect(buffer).toBeInstanceOf(EventBufferProxy); await buffer.ensureWorkerIsLoaded(); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await buffer.addEvent(largeEvent); await buffer.addEvent(largeEvent); diff --git a/packages/replay/test/unit/eventBuffer/EventBufferProxy.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferProxy.test.ts index c4a98f3c446e..6e51d11c2dab 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferProxy.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferProxy.test.ts @@ -4,9 +4,10 @@ import pako from 'pako'; import { BASE_TIMESTAMP } from '../..'; import { EventBufferProxy } from '../../../src/eventBuffer/EventBufferProxy'; +import { getTestEventIncremental } from '../../utils/getTestEvent'; import { createEventBuffer } from './../../../src/eventBuffer'; -const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; +const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP }); describe('Unit | eventBuffer | EventBufferProxy', () => { let consoleErrorSpy: jest.SpyInstance; diff --git a/packages/replay/test/unit/util/addEvent.test.ts b/packages/replay/test/unit/util/addEvent.test.ts index f00dc82d338f..6230bd40c21c 100644 --- a/packages/replay/test/unit/util/addEvent.test.ts +++ b/packages/replay/test/unit/util/addEvent.test.ts @@ -4,6 +4,7 @@ import { BASE_TIMESTAMP } from '../..'; import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; import type { EventBufferProxy } from '../../../src/eventBuffer/EventBufferProxy'; import { addEvent } from '../../../src/util/addEvent'; +import { getTestEventIncremental } from '../../utils/getTestEvent'; import { setupReplayContainer } from '../../utils/setupReplayContainer'; import { useFakeTimers } from '../../utils/use-fake-timers'; @@ -40,11 +41,10 @@ describe('Unit | util | addEvent', () => { }, }); - const largeEvent = { + const largeEvent = getTestEventIncremental({ data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, timestamp: BASE_TIMESTAMP, - type: 3, - }; + }); await (replay.eventBuffer as EventBufferProxy).ensureWorkerIsLoaded(); diff --git a/packages/replay/test/unit/util/handleRecordingEmit.test.ts b/packages/replay/test/unit/util/handleRecordingEmit.test.ts index 7978939291bd..73cab05a1535 100644 --- a/packages/replay/test/unit/util/handleRecordingEmit.test.ts +++ b/packages/replay/test/unit/util/handleRecordingEmit.test.ts @@ -1,7 +1,7 @@ import { EventType } from '@sentry-internal/rrweb'; import { BASE_TIMESTAMP } from '../..'; -import type { OptionFrameEvent } from '../../../src/types'; +import type { ReplayOptionFrameEvent } from '../../../src/types'; import * as SentryAddEvent from '../../../src/util/addEvent'; import { createOptionsEvent, getHandleRecordingEmit } from '../../../src/util/handleRecordingEmit'; import { setupReplayContainer } from '../../utils/setupReplayContainer'; @@ -9,7 +9,7 @@ import { useFakeTimers } from '../../utils/use-fake-timers'; useFakeTimers(); -let optionsEvent: OptionFrameEvent; +let optionsEvent: ReplayOptionFrameEvent; describe('Unit | util | handleRecordingEmit', () => { let addEventMock: jest.SpyInstance; diff --git a/packages/replay/test/utils/getTestEvent.ts b/packages/replay/test/utils/getTestEvent.ts new file mode 100644 index 000000000000..c62983b4c60d --- /dev/null +++ b/packages/replay/test/utils/getTestEvent.ts @@ -0,0 +1,26 @@ +import type { ReplayEventType, ReplayEventWithTime } from '../../src'; +import { ReplayEventTypeFullSnapshot, ReplayEventTypeIncrementalSnapshot } from '../../src/types'; + +export function getTestEvent({ + timestamp, + type, + data, +}: { + timestamp: number; + data?: any; + type: ReplayEventType; +}): ReplayEventWithTime { + return { + data: data || {}, + timestamp, + type, + }; +} + +export function getTestEventCheckout({ timestamp, data }: { timestamp: number; data?: any }): ReplayEventWithTime { + return getTestEvent({ timestamp, data, type: ReplayEventTypeFullSnapshot }); +} + +export function getTestEventIncremental({ timestamp, data }: { timestamp: number; data?: any }): ReplayEventWithTime { + return getTestEvent({ timestamp, data, type: ReplayEventTypeIncrementalSnapshot }); +} From aab71b1d769b6189c94943ab7639de2ee2801c2e Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 29 Aug 2023 11:29:54 -0400 Subject: [PATCH 02/29] fix(replay): Resume replay recording if paused when session is expired (#8889) Previously was attempting to take full snapshot when replay recording was not active. Fixes https://github.com/getsentry/sentry-javascript/issues/8885 --- packages/replay/src/replay.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index a96910e02f31..692a0043844b 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -646,7 +646,11 @@ export class ReplayContainer implements ReplayContainerInterface { } // Session is expired, trigger a full snapshot (which will create a new session) - this._triggerFullSnapshot(); + if (this.isPaused()) { + this.resume(); + } else { + this._triggerFullSnapshot(); + } return false; } From 723285fd8caf68bb489c9c5c8de68216d6a481d3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 30 Aug 2023 11:50:34 +0100 Subject: [PATCH 03/29] feat(nextjs): Add route handler instrumentation (#8832) --- .../app/route-handlers/[param]/edge/route.ts | 11 +++ .../app/route-handlers/[param]/error/route.ts | 3 + .../app/route-handlers/[param]/route.ts | 9 ++ .../tests/route-handlers.test.ts | 95 +++++++++++++++++++ packages/nextjs/rollup.npm.config.js | 1 + packages/nextjs/src/common/index.ts | 2 + packages/nextjs/src/common/types.ts | 7 ++ .../src/common/wrapRouteHandlerWithSentry.ts | 84 ++++++++++++++++ .../src/config/loaders/wrappingLoader.ts | 17 +++- .../templates/routeHandlerWrapperTemplate.ts | 75 +++++++++++++++ packages/nextjs/src/config/webpack.ts | 27 +++++- 11 files changed, 324 insertions(+), 7 deletions(-) create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/edge/route.ts create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/error/route.ts create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts create mode 100644 packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts create mode 100644 packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts create mode 100644 packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/edge/route.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/edge/route.ts new file mode 100644 index 000000000000..8c96a39e5554 --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/edge/route.ts @@ -0,0 +1,11 @@ +import { NextResponse } from 'next/server'; + +export const runtime = 'edge'; + +export async function PATCH() { + return NextResponse.json({ name: 'John Doe' }, { status: 401 }); +} + +export async function DELETE() { + throw new Error('route-handler-edge-error'); +} diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/error/route.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/error/route.ts new file mode 100644 index 000000000000..fd50ef5c8a44 --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/error/route.ts @@ -0,0 +1,3 @@ +export async function PUT() { + throw new Error('route-handler-error'); +} diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts new file mode 100644 index 000000000000..386b8c6e117f --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts @@ -0,0 +1,9 @@ +import { NextResponse } from 'next/server'; + +export async function GET() { + return NextResponse.json({ name: 'John Doe' }); +} + +export async function POST() { + return NextResponse.json({ name: 'John Doe' }, { status: 404 }); +} diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts new file mode 100644 index 000000000000..566df1abb1d1 --- /dev/null +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts @@ -0,0 +1,95 @@ +import { test, expect } from '@playwright/test'; +import { waitForTransaction, waitForError } from '../event-proxy-server'; + +test('Should create a transaction for route handlers', async ({ request }) => { + const routehandlerTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'GET /route-handlers/[param]'; + }); + + const response = await request.get('/route-handlers/foo'); + expect(await response.json()).toStrictEqual({ name: 'John Doe' }); + + const routehandlerTransaction = await routehandlerTransactionPromise; + + expect(routehandlerTransaction.contexts?.trace?.status).toBe('ok'); + expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); +}); + +test('Should create a transaction for route handlers and correctly set span status depending on http status', async ({ + request, +}) => { + const routehandlerTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'POST /route-handlers/[param]'; + }); + + const response = await request.post('/route-handlers/bar'); + expect(await response.json()).toStrictEqual({ name: 'John Doe' }); + + const routehandlerTransaction = await routehandlerTransactionPromise; + + expect(routehandlerTransaction.contexts?.trace?.status).toBe('not_found'); + expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); +}); + +test('Should record exceptions and transactions for faulty route handlers', async ({ request }) => { + const errorEventPromise = waitForError('nextjs-13-app-dir', errorEvent => { + return errorEvent?.exception?.values?.[0]?.value === 'route-handler-error'; + }); + + const routehandlerTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'PUT /route-handlers/[param]/error'; + }); + + await request.put('/route-handlers/baz/error').catch(() => { + // noop + }); + + const routehandlerTransaction = await routehandlerTransactionPromise; + const routehandlerError = await errorEventPromise; + + expect(routehandlerTransaction.contexts?.trace?.status).toBe('internal_error'); + expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); + + expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-error'); + expect(routehandlerError.tags?.transaction).toBe('PUT /route-handlers/[param]/error'); +}); + +test.describe('Edge runtime', () => { + test('should create a transaction for route handlers', async ({ request }) => { + const routehandlerTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'PATCH /route-handlers/[param]/edge'; + }); + + const response = await request.patch('/route-handlers/bar/edge'); + expect(await response.json()).toStrictEqual({ name: 'John Doe' }); + + const routehandlerTransaction = await routehandlerTransactionPromise; + + expect(routehandlerTransaction.contexts?.trace?.status).toBe('unauthenticated'); + expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); + }); + + test('should record exceptions and transactions for faulty route handlers', async ({ request }) => { + const errorEventPromise = waitForError('nextjs-13-app-dir', errorEvent => { + return errorEvent?.exception?.values?.[0]?.value === 'route-handler-edge-error'; + }); + + const routehandlerTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { + return transactionEvent?.transaction === 'DELETE /route-handlers/[param]/edge'; + }); + + await request.delete('/route-handlers/baz/edge').catch(() => { + // noop + }); + + const routehandlerTransaction = await routehandlerTransactionPromise; + const routehandlerError = await errorEventPromise; + + expect(routehandlerTransaction.contexts?.trace?.status).toBe('internal_error'); + expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); + expect(routehandlerTransaction.contexts?.runtime?.name).toBe('edge'); + + expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-edge-error'); + expect(routehandlerError.contexts?.runtime?.name).toBe('edge'); + }); +}); diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index ce15b951235e..e033fd6f90c1 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -30,6 +30,7 @@ export default [ 'src/config/templates/requestAsyncStorageShim.ts', 'src/config/templates/sentryInitWrapperTemplate.ts', 'src/config/templates/serverComponentWrapperTemplate.ts', + 'src/config/templates/routeHandlerWrapperTemplate.ts', ], packageSpecificConfig: { diff --git a/packages/nextjs/src/common/index.ts b/packages/nextjs/src/common/index.ts index ccd4a628634e..3aeef33af760 100644 --- a/packages/nextjs/src/common/index.ts +++ b/packages/nextjs/src/common/index.ts @@ -36,6 +36,8 @@ export { export { wrapServerComponentWithSentry } from './wrapServerComponentWithSentry'; +export { wrapRouteHandlerWithSentry } from './wrapRouteHandlerWithSentry'; + export { wrapApiHandlerWithSentryVercelCrons } from './wrapApiHandlerWithSentryVercelCrons'; export { wrapMiddlewareWithSentry } from './wrapMiddlewareWithSentry'; diff --git a/packages/nextjs/src/common/types.ts b/packages/nextjs/src/common/types.ts index dc838b214276..7c1ce7425108 100644 --- a/packages/nextjs/src/common/types.ts +++ b/packages/nextjs/src/common/types.ts @@ -8,6 +8,13 @@ export type ServerComponentContext = { baggageHeader?: string; }; +export interface RouteHandlerContext { + method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE' | 'HEAD' | 'OPTIONS'; + parameterizedRoute: string; + sentryTraceHeader?: string; + baggageHeader?: string; +} + export type VercelCronsConfig = { path?: string; schedule?: string }[] | undefined; // The `NextApiHandler` and `WrappedNextApiHandler` types are the same as the official `NextApiHandler` type, except: diff --git a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts new file mode 100644 index 000000000000..a4d7a96b94fd --- /dev/null +++ b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts @@ -0,0 +1,84 @@ +import { addTracingExtensions, captureException, flush, getCurrentHub, runWithAsyncContext, trace } from '@sentry/core'; +import { addExceptionMechanism, tracingContextFromHeaders } from '@sentry/utils'; + +import { isRedirectNavigationError } from './nextNavigationErrorUtils'; +import type { RouteHandlerContext } from './types'; +import { platformSupportsStreaming } from './utils/platformSupportsStreaming'; + +/** + * Wraps a Next.js route handler with performance and error instrumentation. + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function wrapRouteHandlerWithSentry any>( + routeHandler: F, + context: RouteHandlerContext, +): (...args: Parameters) => ReturnType extends Promise ? ReturnType : Promise> { + addTracingExtensions(); + + const { method, parameterizedRoute, baggageHeader, sentryTraceHeader } = context; + + return new Proxy(routeHandler, { + apply: (originalFunction, thisArg, args) => { + return runWithAsyncContext(async () => { + const hub = getCurrentHub(); + const currentScope = hub.getScope(); + + const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders( + sentryTraceHeader, + baggageHeader, + ); + currentScope.setPropagationContext(propagationContext); + + let res; + try { + res = await trace( + { + op: 'http.server', + name: `${method} ${parameterizedRoute}`, + status: 'ok', + ...traceparentData, + metadata: { + source: 'route', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + }, + async span => { + const response: Response = await originalFunction.apply(thisArg, args); + + try { + span?.setHttpStatus(response.status); + } catch { + // best effort + } + + return response; + }, + error => { + // Next.js throws errors when calling `redirect()`. We don't wanna report these. + if (!isRedirectNavigationError(error)) { + captureException(error, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + handled: false, + }); + return event; + }); + + return scope; + }); + } + }, + ); + } finally { + if (!platformSupportsStreaming() || process.env.NEXT_RUNTIME === 'edge') { + // 1. Edge tranpsort requires manual flushing + // 2. Lambdas require manual flushing to prevent execution freeze before the event is sent + await flush(1000); + } + } + + return res; + }); + }, + }); +} diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 1731722de7bb..4e19d27fea9b 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -40,12 +40,15 @@ const serverComponentWrapperTemplatePath = path.resolve( ); const serverComponentWrapperTemplateCode = fs.readFileSync(serverComponentWrapperTemplatePath, { encoding: 'utf8' }); +const routeHandlerWrapperTemplatePath = path.resolve(__dirname, '..', 'templates', 'routeHandlerWrapperTemplate.js'); +const routeHandlerWrapperTemplateCode = fs.readFileSync(routeHandlerWrapperTemplatePath, { encoding: 'utf8' }); + type LoaderOptions = { pagesDir: string; appDir: string; pageExtensionRegex: string; excludeServerRoutes: Array; - wrappingTargetKind: 'page' | 'api-route' | 'middleware' | 'server-component' | 'sentry-init'; + wrappingTargetKind: 'page' | 'api-route' | 'middleware' | 'server-component' | 'sentry-init' | 'route-handler'; sentryConfigFilePath?: string; vercelCronsConfig?: VercelCronsConfig; }; @@ -143,14 +146,14 @@ export default function wrappingLoader( // Inject the route and the path to the file we're wrapping into the template templateCode = templateCode.replace(/__ROUTE__/g, parameterizedPagesRoute.replace(/\\/g, '\\\\')); - } else if (wrappingTargetKind === 'server-component') { + } else if (wrappingTargetKind === 'server-component' || wrappingTargetKind === 'route-handler') { // Get the parameterized route name from this page's filepath const parameterizedPagesRoute = path.posix .normalize(path.relative(appDir, this.resourcePath)) // Add a slash at the beginning .replace(/(.*)/, '/$1') // Pull off the file name - .replace(/\/[^/]+\.(js|jsx|tsx)$/, '') + .replace(/\/[^/]+\.(js|ts|jsx|tsx)$/, '') // Remove routing groups: https://beta.nextjs.org/docs/routing/defining-routes#example-creating-multiple-root-layouts .replace(/\/(\(.*?\)\/)+/g, '/') // In case all of the above have left us with an empty string (which will happen if we're dealing with the @@ -172,7 +175,11 @@ export default function wrappingLoader( return; } - templateCode = serverComponentWrapperTemplateCode; + if (wrappingTargetKind === 'server-component') { + templateCode = serverComponentWrapperTemplateCode; + } else { + templateCode = routeHandlerWrapperTemplateCode; + } if (requestAsyncStorageModuleExists) { templateCode = templateCode.replace( @@ -199,7 +206,7 @@ export default function wrappingLoader( const componentTypeMatch = path.posix .normalize(path.relative(appDir, this.resourcePath)) - .match(/\/?([^/]+)\.(?:js|jsx|tsx)$/); + .match(/\/?([^/]+)\.(?:js|ts|jsx|tsx)$/); if (componentTypeMatch && componentTypeMatch[1]) { let componentType; diff --git a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts new file mode 100644 index 000000000000..813f860d35de --- /dev/null +++ b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts @@ -0,0 +1,75 @@ +// @ts-ignore Because we cannot be sure if the RequestAsyncStorage module exists (it is not part of the Next.js public +// API) we use a shim if it doesn't exist. The logic for this is in the wrapping loader. +// eslint-disable-next-line import/no-unresolved +import { requestAsyncStorage } from '__SENTRY_NEXTJS_REQUEST_ASYNC_STORAGE_SHIM__'; +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +import * as routeModule from '__SENTRY_WRAPPING_TARGET_FILE__'; +// eslint-disable-next-line import/no-extraneous-dependencies +import * as Sentry from '@sentry/nextjs'; + +import type { RequestAsyncStorage } from './requestAsyncStorageShim'; + +declare const requestAsyncStorage: RequestAsyncStorage; + +declare const routeModule: { + default: unknown; + GET?: (...args: unknown[]) => unknown; + POST?: (...args: unknown[]) => unknown; + PUT?: (...args: unknown[]) => unknown; + PATCH?: (...args: unknown[]) => unknown; + DELETE?: (...args: unknown[]) => unknown; + HEAD?: (...args: unknown[]) => unknown; + OPTIONS?: (...args: unknown[]) => unknown; +}; + +function wrapHandler(handler: T, method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE' | 'HEAD' | 'OPTIONS'): T { + // Running the instrumentation code during the build phase will mark any function as "dynamic" because we're accessing + // the Request object. We do not want to turn handlers dynamic so we skip instrumentation in the build phase. + if (process.env.NEXT_PHASE === 'phase-production-build') { + return handler; + } + + if (typeof handler !== 'function') { + return handler; + } + + return new Proxy(handler, { + apply: (originalFunction, thisArg, args) => { + let sentryTraceHeader: string | undefined | null = undefined; + let baggageHeader: string | undefined | null = undefined; + + // We try-catch here just in case the API around `requestAsyncStorage` changes unexpectedly since it is not public API + try { + const requestAsyncStore = requestAsyncStorage.getStore(); + sentryTraceHeader = requestAsyncStore?.headers.get('sentry-trace') ?? undefined; + baggageHeader = requestAsyncStore?.headers.get('baggage') ?? undefined; + } catch (e) { + /** empty */ + } + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + return Sentry.wrapRouteHandlerWithSentry(originalFunction as any, { + method, + parameterizedRoute: '__ROUTE__', + sentryTraceHeader, + baggageHeader, + }).apply(thisArg, args); + }, + }); +} + +export const GET = wrapHandler(routeModule.GET, 'GET'); +export const POST = wrapHandler(routeModule.POST, 'POST'); +export const PUT = wrapHandler(routeModule.PUT, 'PUT'); +export const PATCH = wrapHandler(routeModule.PATCH, 'PATCH'); +export const DELETE = wrapHandler(routeModule.DELETE, 'DELETE'); +export const HEAD = wrapHandler(routeModule.HEAD, 'HEAD'); +export const OPTIONS = wrapHandler(routeModule.OPTIONS, 'OPTIONS'); + +// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to +// not include anything whose name matchs something we've explicitly exported above. +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; +export default routeModule.default; diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index d711fef5c14f..5ca49b798aca 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -173,6 +173,14 @@ export function constructWebpackConfigFunction( ); }; + const isRouteHandlerResource = (resourcePath: string): boolean => { + const normalizedAbsoluteResourcePath = normalizeLoaderResourcePath(resourcePath); + return ( + normalizedAbsoluteResourcePath.startsWith(appDirPath + path.sep) && + !!normalizedAbsoluteResourcePath.match(/[\\/]route\.(js|ts)$/) + ); + }; + if (isServer && userSentryOptions.autoInstrumentServerFunctions !== false) { // It is very important that we insert our loaders at the beginning of the array because we expect any sort of transformations/transpilations (e.g. TS -> JS) to already have happened. @@ -245,7 +253,7 @@ export function constructWebpackConfigFunction( } if (isServer && userSentryOptions.autoInstrumentAppDirectory !== false) { - // Wrap page server components + // Wrap server components newConfig.module.rules.unshift({ test: isServerComponentResource, use: [ @@ -258,6 +266,20 @@ export function constructWebpackConfigFunction( }, ], }); + + // Wrap route handlers + newConfig.module.rules.unshift({ + test: isRouteHandlerResource, + use: [ + { + loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), + options: { + ...staticWrappingLoaderOptions, + wrappingTargetKind: 'route-handler', + }, + }, + ], + }); } if (isServer) { @@ -268,7 +290,8 @@ export function constructWebpackConfigFunction( isPageResource(resourcePath) || isApiRouteResource(resourcePath) || isMiddlewareResource(resourcePath) || - isServerComponentResource(resourcePath) + isServerComponentResource(resourcePath) || + isRouteHandlerResource(resourcePath) ); }, use: [ From 0138d1c40d123b2a33f83ecd6d403cf78ffafe26 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Aug 2023 13:08:55 +0200 Subject: [PATCH 04/29] ref: Avoid using global singleton for logger (#8880) This tries to simplify the logger to avoid the global lookup. If you have more than one utils, having multiple loggers would be the least of your problems. --- .../suites/public-api/debug/init.js | 8 ++++ .../suites/public-api/debug/test.ts | 41 +++++++++++++++++ packages/hub/test/scope.test.ts | 1 + .../integrations/test/captureconsole.test.ts | 10 ++++- packages/replay/test/mocks/resetSdkMock.ts | 11 +++-- packages/utils/src/instrument.ts | 17 ++++--- packages/utils/src/logger.ts | 44 ++++++++----------- 7 files changed, 97 insertions(+), 35 deletions(-) create mode 100644 packages/browser-integration-tests/suites/public-api/debug/init.js create mode 100644 packages/browser-integration-tests/suites/public-api/debug/test.ts diff --git a/packages/browser-integration-tests/suites/public-api/debug/init.js b/packages/browser-integration-tests/suites/public-api/debug/init.js new file mode 100644 index 000000000000..573e4fdcb621 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/debug/init.js @@ -0,0 +1,8 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + debug: true, +}); diff --git a/packages/browser-integration-tests/suites/public-api/debug/test.ts b/packages/browser-integration-tests/suites/public-api/debug/test.ts new file mode 100644 index 000000000000..ab417154ae55 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/debug/test.ts @@ -0,0 +1,41 @@ +/* eslint-disable no-console */ +import type { ConsoleMessage } from '@playwright/test'; +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../utils/fixtures'; + +sentryTest('logs debug messages correctly', async ({ getLocalTestUrl, page }) => { + const bundleKey = process.env.PW_BUNDLE || ''; + const hasDebug = !bundleKey.includes('_min'); + + const url = await getLocalTestUrl({ testDir: __dirname }); + + const consoleMessages: string[] = []; + + page.on('console', (msg: ConsoleMessage) => { + consoleMessages.push(msg.text()); + }); + + await page.goto(url); + + await page.evaluate(() => console.log('test log')); + + expect(consoleMessages).toEqual( + hasDebug + ? [ + 'Sentry Logger [log]: Integration installed: InboundFilters', + 'Sentry Logger [log]: Integration installed: FunctionToString', + 'Sentry Logger [log]: Integration installed: TryCatch', + 'Sentry Logger [log]: Integration installed: Breadcrumbs', + 'Sentry Logger [log]: Global Handler attached: onerror', + 'Sentry Logger [log]: Global Handler attached: onunhandledrejection', + 'Sentry Logger [log]: Integration installed: GlobalHandlers', + 'Sentry Logger [log]: Integration installed: LinkedErrors', + 'Sentry Logger [log]: Integration installed: Dedupe', + 'Sentry Logger [log]: Integration installed: HttpContext', + 'Sentry Logger [warn]: Discarded session because of missing or non-string release', + 'test log', + ] + : ['[Sentry] Cannot initialize SDK with `debug` option using a non-debug bundle.', 'test log'], + ); +}); diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index 4c0d830340ec..edcaad465930 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -9,6 +9,7 @@ describe('Scope', () => { afterEach(() => { jest.resetAllMocks(); jest.useRealTimers(); + GLOBAL_OBJ.__SENTRY__ = GLOBAL_OBJ.__SENTRY__ || {}; GLOBAL_OBJ.__SENTRY__.globalEventProcessors = undefined; }); diff --git a/packages/integrations/test/captureconsole.test.ts b/packages/integrations/test/captureconsole.test.ts index 0b851c493062..ba906f0ea2fd 100644 --- a/packages/integrations/test/captureconsole.test.ts +++ b/packages/integrations/test/captureconsole.test.ts @@ -1,7 +1,13 @@ /* eslint-disable @typescript-eslint/unbound-method */ import type { Event, Hub, Integration } from '@sentry/types'; import type { ConsoleLevel } from '@sentry/utils'; -import { addInstrumentationHandler, CONSOLE_LEVELS, GLOBAL_OBJ, originalConsoleMethods } from '@sentry/utils'; +import { + addInstrumentationHandler, + CONSOLE_LEVELS, + GLOBAL_OBJ, + originalConsoleMethods, + resetInstrumentationHandlers, +} from '@sentry/utils'; import { CaptureConsole } from '../src/captureconsole'; @@ -54,6 +60,8 @@ describe('CaptureConsole setup', () => { CONSOLE_LEVELS.forEach(key => { originalConsoleMethods[key] = _originalConsoleMethods[key]; }); + + resetInstrumentationHandlers(); }); describe('monkeypatching', () => { diff --git a/packages/replay/test/mocks/resetSdkMock.ts b/packages/replay/test/mocks/resetSdkMock.ts index 5d9782dc457d..356434373df6 100644 --- a/packages/replay/test/mocks/resetSdkMock.ts +++ b/packages/replay/test/mocks/resetSdkMock.ts @@ -1,3 +1,6 @@ +import type { EventProcessor } from '@sentry/types'; +import { getGlobalSingleton, resetInstrumentationHandlers } from '@sentry/utils'; + import type { Replay as ReplayIntegration } from '../../src'; import type { ReplayContainer } from '../../src/replay'; import type { RecordMock } from './../index'; @@ -17,9 +20,11 @@ export async function resetSdkMock({ replayOptions, sentryOptions, autoStart }: jest.setSystemTime(new Date(BASE_TIMESTAMP)); jest.clearAllMocks(); jest.resetModules(); - // NOTE: The listeners added to `addInstrumentationHandler` are leaking - // @ts-ignore Don't know if there's a cleaner way to clean up old event processors - globalThis.__SENTRY__.globalEventProcessors = []; + + // Clear all handlers that have been registered + resetInstrumentationHandlers(); + getGlobalSingleton('globalEventProcessors', () => []).length = 0; + const SentryUtils = await import('@sentry/utils'); jest.spyOn(SentryUtils, 'addInstrumentationHandler').mockImplementation((type, handler: (args: any) => any) => { if (type === 'dom') { diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 94812f47b252..157fe8a50c0b 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -11,7 +11,7 @@ import type { import { isString } from './is'; import type { ConsoleLevel } from './logger'; -import { CONSOLE_LEVELS, logger } from './logger'; +import { CONSOLE_LEVELS, logger, originalConsoleMethods } from './logger'; import { fill } from './object'; import { getFunctionName } from './stacktrace'; import { supportsHistory, supportsNativeFetch } from './supports'; @@ -94,6 +94,16 @@ export function addInstrumentationHandler(type: InstrumentHandlerType, callback: instrument(type); } +/** + * Reset all instrumentation handlers. + * This can be used by tests to ensure we have a clean slate of instrumentation handlers. + */ +export function resetInstrumentationHandlers(): void { + Object.keys(handlers).forEach(key => { + handlers[key as InstrumentHandlerType] = undefined; + }); +} + /** JSDoc */ function triggerHandlers(type: InstrumentHandlerType, data: any): void { if (!type || !handlers[type]) { @@ -113,11 +123,6 @@ function triggerHandlers(type: InstrumentHandlerType, data: any): void { } } -/** Only exported for testing & debugging. */ -export const originalConsoleMethods: { - [key in ConsoleLevel]?: (...args: any[]) => void; -} = {}; - /** JSDoc */ function instrumentConsole(): void { if (!('console' in GLOBAL_OBJ)) { diff --git a/packages/utils/src/logger.ts b/packages/utils/src/logger.ts index e773ae28f0b5..07642b8c4f04 100644 --- a/packages/utils/src/logger.ts +++ b/packages/utils/src/logger.ts @@ -1,6 +1,4 @@ -import type { WrappedFunction } from '@sentry/types'; - -import { getGlobalSingleton, GLOBAL_OBJ } from './worldwide'; +import { GLOBAL_OBJ } from './worldwide'; /** Prefix for logging strings */ const PREFIX = 'Sentry Logger '; @@ -9,7 +7,13 @@ export const CONSOLE_LEVELS = ['debug', 'info', 'warn', 'error', 'log', 'assert' export type ConsoleLevel = (typeof CONSOLE_LEVELS)[number]; type LoggerMethod = (...args: unknown[]) => void; -type LoggerConsoleMethods = Record<(typeof CONSOLE_LEVELS)[number], LoggerMethod>; +type LoggerConsoleMethods = Record; + +/** This may be mutated by the console instrumentation. */ +export const originalConsoleMethods: { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + [key in ConsoleLevel]?: (...args: any[]) => void; +} = {}; /** JSDoc */ interface Logger extends LoggerConsoleMethods { @@ -28,26 +32,24 @@ export function consoleSandbox(callback: () => T): T { return callback(); } - const originalConsole = GLOBAL_OBJ.console as Console & Record; - const wrappedLevels: Partial = {}; + const console = GLOBAL_OBJ.console as Console; + const wrappedFuncs: Partial = {}; + + const wrappedLevels = Object.keys(originalConsoleMethods) as ConsoleLevel[]; // Restore all wrapped console methods - CONSOLE_LEVELS.forEach(level => { - // TODO(v7): Remove this check as it's only needed for Node 6 - const originalWrappedFunc = - originalConsole[level] && (originalConsole[level] as WrappedFunction).__sentry_original__; - if (level in originalConsole && originalWrappedFunc) { - wrappedLevels[level] = originalConsole[level] as LoggerConsoleMethods[typeof level]; - originalConsole[level] = originalWrappedFunc as Console[typeof level]; - } + wrappedLevels.forEach(level => { + const originalConsoleMethod = originalConsoleMethods[level] as LoggerMethod; + wrappedFuncs[level] = console[level] as LoggerMethod | undefined; + console[level] = originalConsoleMethod; }); try { return callback(); } finally { // Revert restoration to wrapped state - Object.keys(wrappedLevels).forEach(level => { - originalConsole[level] = wrappedLevels[level as (typeof CONSOLE_LEVELS)[number]]; + wrappedLevels.forEach(level => { + console[level] = wrappedFuncs[level] as LoggerMethod; }); } } @@ -83,12 +85,4 @@ function makeLogger(): Logger { return logger as Logger; } -// Ensure we only have a single logger instance, even if multiple versions of @sentry/utils are being used -let logger: Logger; -if (__DEBUG_BUILD__) { - logger = getGlobalSingleton('logger', makeLogger); -} else { - logger = makeLogger(); -} - -export { logger }; +export const logger = makeLogger(); From 5cc706476e6173433c6c3b00c37b96f595a4b945 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Aug 2023 13:09:17 +0200 Subject: [PATCH 05/29] fix(node-experimental): Fix trace origins (#8908) --- packages/node-experimental/src/integrations/express.ts | 2 +- packages/node-experimental/src/integrations/fastify.ts | 2 +- packages/node-experimental/src/integrations/http.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/node-experimental/src/integrations/express.ts b/packages/node-experimental/src/integrations/express.ts index 8a97431b0b0a..9c30d70f4be8 100644 --- a/packages/node-experimental/src/integrations/express.ts +++ b/packages/node-experimental/src/integrations/express.ts @@ -32,7 +32,7 @@ export class Express extends NodePerformanceIntegration implements Integra new ExpressInstrumentation({ requestHook(span) { addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.http.otel-express', + origin: 'auto.http.otel.express', }); }, }), diff --git a/packages/node-experimental/src/integrations/fastify.ts b/packages/node-experimental/src/integrations/fastify.ts index bff0f058c5d1..e1098d8d89b9 100644 --- a/packages/node-experimental/src/integrations/fastify.ts +++ b/packages/node-experimental/src/integrations/fastify.ts @@ -32,7 +32,7 @@ export class Fastify extends NodePerformanceIntegration implements Integra new FastifyInstrumentation({ requestHook(span) { addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.http.otel-fastify', + origin: 'auto.http.otel.fastify', }); }, }), diff --git a/packages/node-experimental/src/integrations/http.ts b/packages/node-experimental/src/integrations/http.ts index e8f5300f6f73..6c31f9bce497 100644 --- a/packages/node-experimental/src/integrations/http.ts +++ b/packages/node-experimental/src/integrations/http.ts @@ -153,7 +153,7 @@ export class Http implements Integration { }, contexts: {}, metadata: {}, - origin: 'auto.http.otel-http', + origin: 'auto.http.otel.http', }; if (span.kind === SpanKind.SERVER) { From 32befda3a4a0fa196e40edf0a08bd543c74d8070 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Aug 2023 13:52:49 +0200 Subject: [PATCH 06/29] fix(remix): Guard against missing default export for server instrument (#8909) Fixes https://github.com/getsentry/sentry-javascript/issues/8904 I don't know enough about remix to know if/when this can actually happen, but doesn't hurt to guard anyhow! --- packages/remix/src/utils/instrumentServer.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index e588ae74ccad..6fc326aab104 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -421,18 +421,21 @@ export function instrumentBuild(build: ServerBuild): ServerBuild { // Because the build can change between build and runtime. // So if there is a new `loader` or`action` or `documentRequest` after build. // We should be able to wrap them, as they may not be wrapped before. - if (!(wrappedEntry.module.default as WrappedFunction).__sentry_original__) { + const defaultExport = wrappedEntry.module.default as undefined | WrappedFunction; + if (defaultExport && !defaultExport.__sentry_original__) { fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction); } for (const [id, route] of Object.entries(build.routes)) { const wrappedRoute = { ...route, module: { ...route.module } }; - if (wrappedRoute.module.action && !(wrappedRoute.module.action as WrappedFunction).__sentry_original__) { + const routeAction = wrappedRoute.module.action as undefined | WrappedFunction; + if (routeAction && !routeAction.__sentry_original__) { fill(wrappedRoute.module, 'action', makeWrappedAction(id)); } - if (wrappedRoute.module.loader && !(wrappedRoute.module.loader as WrappedFunction).__sentry_original__) { + const routeLoader = wrappedRoute.module.loader as undefined | WrappedFunction; + if (routeLoader && !routeLoader.__sentry_original__) { fill(wrappedRoute.module, 'loader', makeWrappedLoader(id)); } From 5fdaaa7234804fecaa25803ced39f601c6175cf4 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 30 Aug 2023 16:52:30 +0100 Subject: [PATCH 07/29] feat(remix): Add debugid injection and map deletion to sourcemaps script (#8814) Updated `sentry-upload-sourcemaps` script: - Updated `sentry-cli` to: `2.20.4` - Added a script to Inject debugids invoking `sentry-cli` (as this feature is not available in JS SDK of `sentry-cli` yet) - Added a script to delete sourcemaps after uploading them to Sentry. (default is to delete, created a new flag to prevent deletion) --- packages/remix/package.json | 2 +- packages/remix/scripts/createRelease.js | 18 ++++++++++++--- packages/remix/scripts/deleteSourcemaps.js | 22 +++++++++++++++++++ packages/remix/scripts/injectDebugId.js | 19 ++++++++++++++++ .../remix/scripts/sentry-upload-sourcemaps.js | 22 ++++++++++++++++++- .../test/scripts/upload-sourcemaps.test.ts | 11 ++++++---- yarn.lock | 11 +++++----- 7 files changed, 90 insertions(+), 15 deletions(-) create mode 100644 packages/remix/scripts/deleteSourcemaps.js create mode 100644 packages/remix/scripts/injectDebugId.js diff --git a/packages/remix/package.json b/packages/remix/package.json index d2976418cfbc..72f06a7324fb 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -27,7 +27,7 @@ "access": "public" }, "dependencies": { - "@sentry/cli": "2.2.0", + "@sentry/cli": "2.20.5", "@sentry/core": "7.66.0", "@sentry/node": "7.66.0", "@sentry/react": "7.66.0", diff --git a/packages/remix/scripts/createRelease.js b/packages/remix/scripts/createRelease.js index 4b71bb3bfd75..a04fdf67a693 100644 --- a/packages/remix/scripts/createRelease.js +++ b/packages/remix/scripts/createRelease.js @@ -1,19 +1,31 @@ +/* eslint-disable no-console */ const SentryCli = require('@sentry/cli'); + +const { deleteSourcemaps } = require('./deleteSourcemaps'); + const sentry = new SentryCli(); -async function createRelease(argv, DEFAULT_URL_PREFIX, DEFAULT_BUILD_PATH) { +async function createRelease(argv, URL_PREFIX, BUILD_PATH) { const RELEASE = argv.release || (await sentry.releases.proposeVersion()); - const URL_PREFIX = argv.urlPrefix || DEFAULT_URL_PREFIX; - const BUILD_PATH = argv.buildPath || DEFAULT_BUILD_PATH; await sentry.releases.new(RELEASE); await sentry.releases.uploadSourceMaps(RELEASE, { urlPrefix: URL_PREFIX, include: [BUILD_PATH], + useArtifactBundle: !argv.disableDebugIds, }); await sentry.releases.finalize(RELEASE); + + if (argv.deleteAfterUpload) { + try { + deleteSourcemaps(BUILD_PATH); + } catch (error) { + console.warn(`[sentry] Failed to delete sourcemaps in build directory: ${BUILD_PATH}`); + console.error(error); + } + } } module.exports = { diff --git a/packages/remix/scripts/deleteSourcemaps.js b/packages/remix/scripts/deleteSourcemaps.js new file mode 100644 index 000000000000..f73cc678f7df --- /dev/null +++ b/packages/remix/scripts/deleteSourcemaps.js @@ -0,0 +1,22 @@ +/* eslint-disable no-console */ +const fs = require('fs'); +const path = require('path'); + +const glob = require('glob'); + +function deleteSourcemaps(buildPath) { + console.info(`[sentry] Deleting sourcemaps from ${buildPath}`); + + // Delete all .map files in the build folder and its subfolders + const mapFiles = glob.sync('**/*.map', { cwd: buildPath }); + + mapFiles.forEach(file => { + fs.unlinkSync(path.join(buildPath, file)); + + console.info(`[sentry] Deleted ${file}`); + }); +} + +module.exports = { + deleteSourcemaps, +}; diff --git a/packages/remix/scripts/injectDebugId.js b/packages/remix/scripts/injectDebugId.js new file mode 100644 index 000000000000..2eb05ef87ba6 --- /dev/null +++ b/packages/remix/scripts/injectDebugId.js @@ -0,0 +1,19 @@ +/* eslint-disable no-console */ +const { execSync } = require('child_process'); + +const SentryCli = require('@sentry/cli'); + +function injectDebugId(buildPath) { + const cliPath = SentryCli.getPath(); + + try { + execSync(`${cliPath} sourcemaps inject ${buildPath}`); + } catch (error) { + console.warn('[sentry] Failed to inject debug ids.'); + console.error(error); + } +} + +module.exports = { + injectDebugId, +}; diff --git a/packages/remix/scripts/sentry-upload-sourcemaps.js b/packages/remix/scripts/sentry-upload-sourcemaps.js index 0d7a081ddc98..c7eef6b0fa92 100755 --- a/packages/remix/scripts/sentry-upload-sourcemaps.js +++ b/packages/remix/scripts/sentry-upload-sourcemaps.js @@ -2,6 +2,7 @@ const yargs = require('yargs'); const { createRelease } = require('./createRelease'); +const { injectDebugId } = require('./injectDebugId'); const DEFAULT_URL_PREFIX = '~/build/'; const DEFAULT_BUILD_PATH = 'public/build'; @@ -24,11 +25,23 @@ const argv = yargs(process.argv.slice(2)) describe: 'The path to the build directory', default: DEFAULT_BUILD_PATH, }) + .option('disableDebugIds', { + type: 'boolean', + describe: 'Disable the injection and upload of debug ids', + default: false, + }) + .option('deleteAfterUpload', { + type: 'boolean', + describe: 'Delete sourcemaps after uploading', + default: true, + }) .usage( 'Usage: $0\n' + ' [--release RELEASE]\n' + ' [--urlPrefix URL_PREFIX]\n' + ' [--buildPath BUILD_PATH]\n\n' + + ' [--disableDebugIds true|false]\n\n' + + ' [--deleteAfterUpload true|false]\n\n' + 'This CLI tool will upload sourcemaps to Sentry for the given release.\n' + 'It has defaults for URL prefix and build path for Remix builds, but you can override them.\n\n' + 'If you need a more advanced configuration, you can use `sentry-cli` instead.\n' + @@ -36,4 +49,11 @@ const argv = yargs(process.argv.slice(2)) ) .wrap(120).argv; -createRelease(argv, DEFAULT_URL_PREFIX, DEFAULT_BUILD_PATH); +const buildPath = argv.buildPath || DEFAULT_BUILD_PATH; +const urlPrefix = argv.urlPrefix || DEFAULT_URL_PREFIX; + +if (!argv.disableDebugIds) { + injectDebugId(buildPath); +} + +createRelease(argv, urlPrefix, buildPath); diff --git a/packages/remix/test/scripts/upload-sourcemaps.test.ts b/packages/remix/test/scripts/upload-sourcemaps.test.ts index f7886c5380fb..1a4a4acbd5c4 100644 --- a/packages/remix/test/scripts/upload-sourcemaps.test.ts +++ b/packages/remix/test/scripts/upload-sourcemaps.test.ts @@ -36,6 +36,7 @@ describe('createRelease', () => { expect(uploadSourceMapsMock).toHaveBeenCalledWith('0.1.2.3', { urlPrefix: '~/build/', include: ['public/build'], + useArtifactBundle: true, }); expect(finalizeMock).toHaveBeenCalledWith('0.1.2.3'); }); @@ -48,6 +49,7 @@ describe('createRelease', () => { expect(uploadSourceMapsMock).toHaveBeenCalledWith('0.1.2.3.4', { urlPrefix: '~/build/', include: ['public/build'], + useArtifactBundle: true, }); expect(finalizeMock).toHaveBeenCalledWith('0.1.2.3.4'); }); @@ -55,8 +57,8 @@ describe('createRelease', () => { it('should use given buildPath and urlPrefix over the defaults when given.', async () => { await createRelease( { - urlPrefix: '~/build/subfolder', - buildPath: 'public/build/subfolder', + urlPrefix: '~/build/', + buildPath: 'public/build', }, '~/build/', 'public/build', @@ -65,8 +67,9 @@ describe('createRelease', () => { expect(proposeVersionMock).toHaveBeenCalled(); expect(newMock).toHaveBeenCalledWith('0.1.2.3.4'); expect(uploadSourceMapsMock).toHaveBeenCalledWith('0.1.2.3.4', { - urlPrefix: '~/build/subfolder', - include: ['public/build/subfolder'], + urlPrefix: '~/build/', + include: ['public/build'], + useArtifactBundle: true, }); expect(finalizeMock).toHaveBeenCalledWith('0.1.2.3.4'); }); diff --git a/yarn.lock b/yarn.lock index 2f4c50f00895..7228ceab27d3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4428,14 +4428,13 @@ unplugin "1.0.1" webpack-sources "3.2.3" -"@sentry/cli@2.2.0": - version "2.2.0" - resolved "https://registry.yarnpkg.com/@sentry/cli/-/cli-2.2.0.tgz#0cf4d529d87e290dea54d7e58fa5ff87ea200e4e" - integrity sha512-ywFtB8VHyWN248LuM67fsRtdMLif/SOHYY3zyef5WybvnAmRLDmGTWK//hSUCebsHBpehRIkmt4iMiyUXwgd5w== +"@sentry/cli@2.20.5": + version "2.20.5" + resolved "https://registry.yarnpkg.com/@sentry/cli/-/cli-2.20.5.tgz#255a5388ca24c211a0eae01dcc4ad813a7ff335a" + integrity sha512-ZvWb86eF0QXH9C5Mbi87aUmr8SH848yEpXJmlM2AoBowpE9kKDnewCAKvyXUihojUFwCSEEjoJhrRMMgmCZqXA== dependencies: https-proxy-agent "^5.0.0" node-fetch "^2.6.7" - npmlog "^6.0.1" progress "^2.0.3" proxy-from-env "^1.1.0" which "^2.0.2" @@ -20552,7 +20551,7 @@ npmlog@^4.1.2: gauge "~2.7.3" set-blocking "~2.0.0" -npmlog@^6.0.0, npmlog@^6.0.1, npmlog@^6.0.2: +npmlog@^6.0.0, npmlog@^6.0.2: version "6.0.2" resolved "https://registry.yarnpkg.com/npmlog/-/npmlog-6.0.2.tgz#c8166017a42f2dea92d6453168dd865186a70830" integrity sha512-/vBvz5Jfr9dT/aFWd0FIRf+T/Q2WBsLENygUaFUqstqsycmZAP/t5BvFJTK0viFmSUxiUKTUplWy5vt+rvKIxg== From 1d64a0639830c61bbc5c50efe5a06f81c1e3fb0c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 31 Aug 2023 13:54:00 +0200 Subject: [PATCH 08/29] fix(browser): Add replay and profiling options to `BrowserClientOptions` (#8921) Add missing replay and profiling options to the `BrowserClientOptions` interface. Previously, we only exposed these types to `BrowserOptions`. This led to type errors for users who directly create a client, without using `Sentry.init` (as reported in #8857). While one would think that `BrowserClientOptions` is basically a superset of `BrowserOptions` (leaving aside `stackparser`, `integrations` and `transport`) this is in fact not the case, which IMO is the core problem here. `BrowserClientOptions` only inherits the base `Options` (which are shared between browser and node), in addition to transport options. However, changing this so that, `BrowserClientOptions` inherits from `BrowserOptions` is a breaking change, so I opted to just add the missing options to `BrowserClientOptions`. --- packages/browser/src/client.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 60579038a50a..5027c4f0f1a4 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -30,7 +30,9 @@ export type BrowserOptions = Options & * Configuration options for the Sentry Browser SDK Client class * @see BrowserClient for more information. */ -export type BrowserClientOptions = ClientOptions; +export type BrowserClientOptions = ClientOptions & + BrowserClientReplayOptions & + BrowserClientProfilingOptions; /** * The Sentry Browser SDK Client. From 5354ee5a0890df6a88ba608a8e64347f14c06641 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 31 Aug 2023 15:18:38 +0200 Subject: [PATCH 09/29] feat(replay): Allow to configure `maxReplayDuration` (#8769) This defaults to 60min, and is capped at max. 60min (=you cannot specify a longer max duration than 60min). Closes https://github.com/getsentry/sentry-javascript/issues/8758 --- .../replay/errors/errorsInSession/init.js | 1 - .../largeMutations/mutationLimit/init.js | 1 - .../suites/replay/maxReplayDuration/init.js | 2 +- .../suites/replay/maxReplayDuration/test.ts | 6 +- .../suites/replay/minReplayDuration/init.js | 1 - .../suites/replay/sessionExpiry/init.js | 2 - .../suites/replay/sessionInactive/init.js | 2 - .../suites/replay/sessionMaxAge/init.js | 3 +- .../suites/replay/sessionMaxAge/test.ts | 6 +- packages/replay/src/constants.ts | 6 +- packages/replay/src/integration.ts | 3 + packages/replay/src/replay.ts | 27 +++++---- .../replay/src/session/loadOrCreateSession.ts | 10 ++-- .../replay/src/session/maybeRefreshSession.ts | 10 ++-- packages/replay/src/types/replay.ts | 7 ++- packages/replay/src/util/addEvent.ts | 4 +- packages/replay/src/util/isSessionExpired.ts | 15 +++-- .../test/integration/errorSampleRate.test.ts | 16 ++--- .../replay/test/integration/flush.test.ts | 20 +++---- .../replay/test/integration/session.test.ts | 8 +-- .../unit/session/loadOrCreateSession.test.ts | 46 +++++++-------- .../unit/session/maybeRefreshSession.test.ts | 37 ++++++------ .../test/unit/util/isSessionExpired.test.ts | 58 +++++++------------ .../replay/test/utils/setupReplayContainer.ts | 2 + yarn.lock | 18 +++--- 25 files changed, 157 insertions(+), 154 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/errors/errorsInSession/init.js b/packages/browser-integration-tests/suites/replay/errors/errorsInSession/init.js index 29486082ff8a..3bc87bdd0be4 100644 --- a/packages/browser-integration-tests/suites/replay/errors/errorsInSession/init.js +++ b/packages/browser-integration-tests/suites/replay/errors/errorsInSession/init.js @@ -19,5 +19,4 @@ Sentry.init({ return event; }, integrations: [window.Replay], - debug: true, }); diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/init.js b/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/init.js index 35c6feed4df7..ed46fe5974dc 100644 --- a/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/init.js +++ b/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/init.js @@ -13,7 +13,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - debug: true, integrations: [window.Replay], }); diff --git a/packages/browser-integration-tests/suites/replay/maxReplayDuration/init.js b/packages/browser-integration-tests/suites/replay/maxReplayDuration/init.js index e5dfd8115207..140b486c5755 100644 --- a/packages/browser-integration-tests/suites/replay/maxReplayDuration/init.js +++ b/packages/browser-integration-tests/suites/replay/maxReplayDuration/init.js @@ -5,6 +5,7 @@ window.Replay = new Sentry.Replay({ flushMinDelay: 200, flushMaxDelay: 200, minReplayDuration: 0, + maxReplayDuration: 2000, }); Sentry.init({ @@ -19,5 +20,4 @@ Sentry.init({ window.Replay._replay.timeouts = { sessionIdlePause: 1000, // this is usually 5min, but we want to test this with shorter times sessionIdleExpire: 2000, // this is usually 15min, but we want to test this with shorter times - maxSessionLife: 2000, // default: 60min }; diff --git a/packages/browser-integration-tests/suites/replay/maxReplayDuration/test.ts b/packages/browser-integration-tests/suites/replay/maxReplayDuration/test.ts index 1e27aef149e2..a40387159bfc 100644 --- a/packages/browser-integration-tests/suites/replay/maxReplayDuration/test.ts +++ b/packages/browser-integration-tests/suites/replay/maxReplayDuration/test.ts @@ -4,7 +4,7 @@ import { sentryTest } from '../../../utils/fixtures'; import { getExpectedReplayEvent } from '../../../utils/replayEventTemplates'; import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers'; -const SESSION_MAX_AGE = 2000; +const MAX_REPLAY_DURATION = 2000; sentryTest('keeps track of max duration across reloads', async ({ getLocalTestPath, page }) => { if (shouldSkipReplayTest()) { @@ -26,7 +26,7 @@ sentryTest('keeps track of max duration across reloads', async ({ getLocalTestPa await page.goto(url); - await new Promise(resolve => setTimeout(resolve, SESSION_MAX_AGE / 2)); + await new Promise(resolve => setTimeout(resolve, MAX_REPLAY_DURATION / 2)); await page.reload(); await page.click('#button1'); @@ -34,7 +34,7 @@ sentryTest('keeps track of max duration across reloads', async ({ getLocalTestPa // After the second reload, we should have a new session (because we exceeded max age) const reqPromise3 = waitForReplayRequest(page, 0); - await new Promise(resolve => setTimeout(resolve, SESSION_MAX_AGE / 2 + 100)); + await new Promise(resolve => setTimeout(resolve, MAX_REPLAY_DURATION / 2 + 100)); void page.click('#button1'); await page.evaluate(`Object.defineProperty(document, 'visibilityState', { diff --git a/packages/browser-integration-tests/suites/replay/minReplayDuration/init.js b/packages/browser-integration-tests/suites/replay/minReplayDuration/init.js index 429559c5781a..cff168651bea 100644 --- a/packages/browser-integration-tests/suites/replay/minReplayDuration/init.js +++ b/packages/browser-integration-tests/suites/replay/minReplayDuration/init.js @@ -12,7 +12,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - debug: true, integrations: [window.Replay], }); diff --git a/packages/browser-integration-tests/suites/replay/sessionExpiry/init.js b/packages/browser-integration-tests/suites/replay/sessionExpiry/init.js index a3b9726f3103..6fa2c80cbe9c 100644 --- a/packages/browser-integration-tests/suites/replay/sessionExpiry/init.js +++ b/packages/browser-integration-tests/suites/replay/sessionExpiry/init.js @@ -12,7 +12,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - debug: true, integrations: [window.Replay], }); @@ -20,5 +19,4 @@ Sentry.init({ window.Replay._replay.timeouts = { sessionIdlePause: 1000, // this is usually 5min, but we want to test this with shorter times sessionIdleExpire: 2000, // this is usually 15min, but we want to test this with shorter times - maxSessionLife: 3600000, // default: 60min }; diff --git a/packages/browser-integration-tests/suites/replay/sessionInactive/init.js b/packages/browser-integration-tests/suites/replay/sessionInactive/init.js index 781e7b583109..c37968bc654a 100644 --- a/packages/browser-integration-tests/suites/replay/sessionInactive/init.js +++ b/packages/browser-integration-tests/suites/replay/sessionInactive/init.js @@ -12,7 +12,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - debug: true, integrations: [window.Replay], }); @@ -20,5 +19,4 @@ Sentry.init({ window.Replay._replay.timeouts = { sessionIdlePause: 1000, // this is usually 5min, but we want to test this with shorter times sessionIdleExpire: 900000, // defayult: 15min - maxSessionLife: 3600000, // default: 60min }; diff --git a/packages/browser-integration-tests/suites/replay/sessionMaxAge/init.js b/packages/browser-integration-tests/suites/replay/sessionMaxAge/init.js index de8b260647ad..4b1e948f534e 100644 --- a/packages/browser-integration-tests/suites/replay/sessionMaxAge/init.js +++ b/packages/browser-integration-tests/suites/replay/sessionMaxAge/init.js @@ -5,6 +5,7 @@ window.Replay = new Sentry.Replay({ flushMinDelay: 200, flushMaxDelay: 200, minReplayDuration: 0, + maxReplayDuration: 4000, }); Sentry.init({ @@ -12,7 +13,6 @@ Sentry.init({ sampleRate: 0, replaysSessionSampleRate: 1.0, replaysOnErrorSampleRate: 0.0, - debug: true, integrations: [window.Replay], }); @@ -20,5 +20,4 @@ Sentry.init({ window.Replay._replay.timeouts = { sessionIdlePause: 300000, // default: 5min sessionIdleExpire: 900000, // default: 15min - maxSessionLife: 4000, // this is usually 60min, but we want to test this with shorter times }; diff --git a/packages/browser-integration-tests/suites/replay/sessionMaxAge/test.ts b/packages/browser-integration-tests/suites/replay/sessionMaxAge/test.ts index ca50c5a62203..3d5d17507d4f 100644 --- a/packages/browser-integration-tests/suites/replay/sessionMaxAge/test.ts +++ b/packages/browser-integration-tests/suites/replay/sessionMaxAge/test.ts @@ -12,7 +12,7 @@ import { } from '../../../utils/replayHelpers'; // Session should be max. 4s long -const SESSION_MAX_AGE = 4000; +const MAX_REPLAY_DURATION = 4000; /* The main difference between this and sessionExpiry test, is that here we wait for the overall time (4s) @@ -58,7 +58,7 @@ sentryTest('handles session that exceeds max age', async ({ getLocalTestPath, pa // Wait for an incremental snapshot // Wait half of the session max age (after initial flush), but account for potentially slow runners const timePassed1 = Date.now() - startTimestamp; - await new Promise(resolve => setTimeout(resolve, Math.max(SESSION_MAX_AGE / 2 - timePassed1, 0))); + await new Promise(resolve => setTimeout(resolve, Math.max(MAX_REPLAY_DURATION / 2 - timePassed1, 0))); await page.click('#button1'); const req1 = await reqPromise1; @@ -71,7 +71,7 @@ sentryTest('handles session that exceeds max age', async ({ getLocalTestPath, pa // Wait for session to expire const timePassed2 = Date.now() - startTimestamp; - await new Promise(resolve => setTimeout(resolve, Math.max(SESSION_MAX_AGE - timePassed2, 0))); + await new Promise(resolve => setTimeout(resolve, Math.max(MAX_REPLAY_DURATION - timePassed2, 0))); await page.click('#button2'); const req2 = await reqPromise2; diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts index d8d5e792a619..f12127970fed 100644 --- a/packages/replay/src/constants.ts +++ b/packages/replay/src/constants.ts @@ -17,9 +17,6 @@ export const SESSION_IDLE_PAUSE_DURATION = 300_000; // 5 minutes in ms // The idle limit for a session after which the session expires. export const SESSION_IDLE_EXPIRE_DURATION = 900_000; // 15 minutes in ms -// The maximum length of a session -export const MAX_SESSION_LIFE = 3_600_000; // 60 minutes in ms - /** Default flush delays */ export const DEFAULT_FLUSH_MIN_DELAY = 5_000; // XXX: Temp fix for our debounce logic where `maxWait` would never occur if it @@ -50,3 +47,6 @@ export const REPLAY_MAX_EVENT_BUFFER_SIZE = 20_000_000; // ~20MB export const MIN_REPLAY_DURATION = 4_999; /* The max. allowed value that the minReplayDuration can be set to. */ export const MIN_REPLAY_DURATION_LIMIT = 15_000; + +/** The max. length of a replay. */ +export const MAX_REPLAY_DURATION = 3_600_000; // 60 minutes in ms; diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index e4c17ea04ba1..57fb36f66b35 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -5,6 +5,7 @@ import { dropUndefinedKeys } from '@sentry/utils'; import { DEFAULT_FLUSH_MAX_DELAY, DEFAULT_FLUSH_MIN_DELAY, + MAX_REPLAY_DURATION, MIN_REPLAY_DURATION, MIN_REPLAY_DURATION_LIMIT, } from './constants'; @@ -57,6 +58,7 @@ export class Replay implements Integration { flushMinDelay = DEFAULT_FLUSH_MIN_DELAY, flushMaxDelay = DEFAULT_FLUSH_MAX_DELAY, minReplayDuration = MIN_REPLAY_DURATION, + maxReplayDuration = MAX_REPLAY_DURATION, stickySession = true, useCompression = true, _experiments = {}, @@ -136,6 +138,7 @@ export class Replay implements Integration { flushMinDelay, flushMaxDelay, minReplayDuration: Math.min(minReplayDuration, MIN_REPLAY_DURATION_LIMIT), + maxReplayDuration: Math.min(maxReplayDuration, MAX_REPLAY_DURATION), stickySession, sessionSampleRate, errorSampleRate, diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 692a0043844b..72a9dd88d7aa 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -6,7 +6,6 @@ import { logger } from '@sentry/utils'; import { BUFFER_CHECKOUT_TIME, - MAX_SESSION_LIFE, SESSION_IDLE_EXPIRE_DURATION, SESSION_IDLE_PAUSE_DURATION, SLOW_CLICK_SCROLL_TIMEOUT, @@ -150,7 +149,6 @@ export class ReplayContainer implements ReplayContainerInterface { this.timeouts = { sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, sessionIdleExpire: SESSION_IDLE_EXPIRE_DURATION, - maxSessionLife: MAX_SESSION_LIFE, } as const; this._lastActivity = Date.now(); this._isEnabled = false; @@ -277,7 +275,8 @@ export class ReplayContainer implements ReplayContainerInterface { const session = loadOrCreateSession( this.session, { - timeouts: this.timeouts, + maxReplayDuration: this._options.maxReplayDuration, + sessionIdleExpire: this.timeouts.sessionIdleExpire, traceInternals: this._options._experiments.traceInternals, }, { @@ -307,7 +306,8 @@ export class ReplayContainer implements ReplayContainerInterface { const session = loadOrCreateSession( this.session, { - timeouts: this.timeouts, + sessionIdleExpire: this.timeouts.sessionIdleExpire, + maxReplayDuration: this._options.maxReplayDuration, traceInternals: this._options._experiments.traceInternals, }, { @@ -483,7 +483,7 @@ export class ReplayContainer implements ReplayContainerInterface { // `shouldRefresh`, the session could be considered expired due to // lifespan, which is not what we want. Update session start date to be // the current timestamp, so that session is not considered to be - // expired. This means that max replay duration can be MAX_SESSION_LIFE + + // expired. This means that max replay duration can be MAX_REPLAY_DURATION + // (length of buffer), which we are ok with. this._updateUserActivity(activityTime); this._updateSessionActivity(activityTime); @@ -764,7 +764,8 @@ export class ReplayContainer implements ReplayContainerInterface { const session = loadOrCreateSession( this.session, { - timeouts: this.timeouts, + sessionIdleExpire: this.timeouts.sessionIdleExpire, + maxReplayDuration: this._options.maxReplayDuration, traceInternals: this._options._experiments.traceInternals, }, { @@ -793,8 +794,9 @@ export class ReplayContainer implements ReplayContainerInterface { const newSession = maybeRefreshSession( currentSession, { - timeouts: this.timeouts, + sessionIdleExpire: this.timeouts.sessionIdleExpire, traceInternals: this._options._experiments.traceInternals, + maxReplayDuration: this._options.maxReplayDuration, }, { stickySession: Boolean(this._options.stickySession), @@ -929,7 +931,10 @@ export class ReplayContainer implements ReplayContainerInterface { return; } - const expired = isSessionExpired(this.session, this.timeouts); + const expired = isSessionExpired(this.session, { + maxReplayDuration: this._options.maxReplayDuration, + ...this.timeouts, + }); if (breadcrumb && !expired) { this._createCustomBreadcrumb(breadcrumb); @@ -1108,7 +1113,7 @@ export class ReplayContainer implements ReplayContainerInterface { // Check total duration again, to avoid sending outdated stuff // We leave 30s wiggle room to accomodate late flushing etc. // This _could_ happen when the browser is suspended during flushing, in which case we just want to stop - if (timestamp - this._context.initialTimestamp > this.timeouts.maxSessionLife + 30_000) { + if (timestamp - this._context.initialTimestamp > this._options.maxReplayDuration + 30_000) { throw new Error('Session is too long, not sending replay'); } @@ -1181,10 +1186,10 @@ export class ReplayContainer implements ReplayContainerInterface { // A flush is about to happen, cancel any queued flushes this._debouncedFlush.cancel(); - // If session is too short, or too long (allow some wiggle room over maxSessionLife), do not send it + // If session is too short, or too long (allow some wiggle room over maxReplayDuration), do not send it // This _should_ not happen, but it may happen if flush is triggered due to a page activity change or similar const tooShort = duration < this._options.minReplayDuration; - const tooLong = duration > this.timeouts.maxSessionLife + 5_000; + const tooLong = duration > this._options.maxReplayDuration + 5_000; if (tooShort || tooLong) { logInfo( `[Replay] Session duration (${Math.floor(duration / 1000)}s) is too ${ diff --git a/packages/replay/src/session/loadOrCreateSession.ts b/packages/replay/src/session/loadOrCreateSession.ts index 9695eef56102..0766c537a4d2 100644 --- a/packages/replay/src/session/loadOrCreateSession.ts +++ b/packages/replay/src/session/loadOrCreateSession.ts @@ -1,4 +1,4 @@ -import type { Session, SessionOptions, Timeouts } from '../types'; +import type { Session, SessionOptions } from '../types'; import { logInfoNextTick } from '../util/log'; import { createSession } from './createSession'; import { fetchSession } from './fetchSession'; @@ -11,10 +11,12 @@ import { maybeRefreshSession } from './maybeRefreshSession'; export function loadOrCreateSession( currentSession: Session | undefined, { - timeouts, traceInternals, + sessionIdleExpire, + maxReplayDuration, }: { - timeouts: Timeouts; + sessionIdleExpire: number; + maxReplayDuration: number; traceInternals?: boolean; }, sessionOptions: SessionOptions, @@ -28,5 +30,5 @@ export function loadOrCreateSession( return createSession(sessionOptions); } - return maybeRefreshSession(existingSession, { timeouts, traceInternals }, sessionOptions); + return maybeRefreshSession(existingSession, { sessionIdleExpire, traceInternals, maxReplayDuration }, sessionOptions); } diff --git a/packages/replay/src/session/maybeRefreshSession.ts b/packages/replay/src/session/maybeRefreshSession.ts index 51e4925d074d..14bc7f9534fa 100644 --- a/packages/replay/src/session/maybeRefreshSession.ts +++ b/packages/replay/src/session/maybeRefreshSession.ts @@ -1,4 +1,4 @@ -import type { Session, SessionOptions, Timeouts } from '../types'; +import type { Session, SessionOptions } from '../types'; import { isSessionExpired } from '../util/isSessionExpired'; import { logInfoNextTick } from '../util/log'; import { createSession } from './createSession'; @@ -12,16 +12,18 @@ import { makeSession } from './Session'; export function maybeRefreshSession( session: Session, { - timeouts, traceInternals, + maxReplayDuration, + sessionIdleExpire, }: { - timeouts: Timeouts; + sessionIdleExpire: number; + maxReplayDuration: number; traceInternals?: boolean; }, sessionOptions: SessionOptions, ): Session { // If not expired, all good, just keep the session - if (!isSessionExpired(session, timeouts)) { + if (!isSessionExpired(session, { sessionIdleExpire, maxReplayDuration })) { return session; } diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts index 1ae48dfecfc3..b5c2318008a0 100644 --- a/packages/replay/src/types/replay.ts +++ b/packages/replay/src/types/replay.ts @@ -31,7 +31,6 @@ export interface SendReplayData { export interface Timeouts { sessionIdlePause: number; sessionIdleExpire: number; - maxSessionLife: number; } /** @@ -187,6 +186,12 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions { */ minReplayDuration: number; + /** + * The max. duration (in ms) a replay session may be. + * This is capped at max. 60min. + */ + maxReplayDuration: number; + /** * Callback before adding a custom recording event * diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index 9024c3cbb6bd..9c73f6d87ac2 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -41,9 +41,9 @@ export async function addEvent( } // Throw out events that are +60min from the initial timestamp - if (timestampInMs > replay.getContext().initialTimestamp + replay.timeouts.maxSessionLife) { + if (timestampInMs > replay.getContext().initialTimestamp + replay.getOptions().maxReplayDuration) { logInfo( - `[Replay] Skipping event with timestamp ${timestampInMs} because it is after maxSessionLife`, + `[Replay] Skipping event with timestamp ${timestampInMs} because it is after maxReplayDuration`, replay.getOptions()._experiments.traceInternals, ); return null; diff --git a/packages/replay/src/util/isSessionExpired.ts b/packages/replay/src/util/isSessionExpired.ts index a51104fd5a47..b77b84a076b0 100644 --- a/packages/replay/src/util/isSessionExpired.ts +++ b/packages/replay/src/util/isSessionExpired.ts @@ -1,15 +1,22 @@ -import type { Session, Timeouts } from '../types'; +import type { Session } from '../types'; import { isExpired } from './isExpired'; /** * Checks to see if session is expired */ -export function isSessionExpired(session: Session, timeouts: Timeouts, targetTime: number = +new Date()): boolean { +export function isSessionExpired( + session: Session, + { + maxReplayDuration, + sessionIdleExpire, + targetTime = Date.now(), + }: { maxReplayDuration: number; sessionIdleExpire: number; targetTime?: number }, +): boolean { return ( // First, check that maximum session length has not been exceeded - isExpired(session.started, timeouts.maxSessionLife, targetTime) || + isExpired(session.started, maxReplayDuration, targetTime) || // check that the idle timeout has not been exceeded (i.e. user has // performed an action within the last `sessionIdleExpire` ms) - isExpired(session.lastActivity, timeouts.sessionIdleExpire, targetTime) + isExpired(session.lastActivity, sessionIdleExpire, targetTime) ); } diff --git a/packages/replay/test/integration/errorSampleRate.test.ts b/packages/replay/test/integration/errorSampleRate.test.ts index 0db920894291..a2dc7d9db2f4 100644 --- a/packages/replay/test/integration/errorSampleRate.test.ts +++ b/packages/replay/test/integration/errorSampleRate.test.ts @@ -3,7 +3,7 @@ import { captureException, getCurrentHub } from '@sentry/core'; import { BUFFER_CHECKOUT_TIME, DEFAULT_FLUSH_MIN_DELAY, - MAX_SESSION_LIFE, + MAX_REPLAY_DURATION, REPLAY_SESSION_KEY, SESSION_IDLE_EXPIRE_DURATION, WINDOW, @@ -430,7 +430,7 @@ describe('Integration | errorSampleRate', () => { // simply stop the session replay completely and wait for a new page load to // resample. it.each([ - ['MAX_SESSION_LIFE', MAX_SESSION_LIFE], + ['MAX_REPLAY_DURATION', MAX_REPLAY_DURATION], ['SESSION_IDLE_DURATION', SESSION_IDLE_EXPIRE_DURATION], ])( 'stops replay if session had an error and exceeds %s and does not start a new session thereafter', @@ -494,7 +494,7 @@ describe('Integration | errorSampleRate', () => { ); it.each([ - ['MAX_SESSION_LIFE', MAX_SESSION_LIFE], + ['MAX_REPLAY_DURATION', MAX_REPLAY_DURATION], ['SESSION_IDLE_EXPIRE_DURATION', SESSION_IDLE_EXPIRE_DURATION], ])('continues buffering replay if session had no error and exceeds %s', async (_label, waitTime) => { const oldSessionId = replay.session?.id; @@ -760,7 +760,7 @@ describe('Integration | errorSampleRate', () => { jest.runAllTimers(); await new Promise(process.nextTick); - jest.advanceTimersByTime(2 * MAX_SESSION_LIFE); + jest.advanceTimersByTime(2 * MAX_REPLAY_DURATION); // in production, this happens at a time interval, here we mock this mockRecord.takeFullSnapshot(true); @@ -785,7 +785,7 @@ describe('Integration | errorSampleRate', () => { data: { isCheckout: true, }, - timestamp: BASE_TIMESTAMP + 2 * MAX_SESSION_LIFE + DEFAULT_FLUSH_MIN_DELAY + 40, + timestamp: BASE_TIMESTAMP + 2 * MAX_REPLAY_DURATION + DEFAULT_FLUSH_MIN_DELAY + 40, type: 2, }, ]), @@ -795,7 +795,7 @@ describe('Integration | errorSampleRate', () => { mockRecord.takeFullSnapshot.mockClear(); (getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance).mockClear(); - jest.advanceTimersByTime(MAX_SESSION_LIFE); + jest.advanceTimersByTime(MAX_REPLAY_DURATION); await new Promise(process.nextTick); mockRecord._emitter(TEST_EVENT); @@ -960,7 +960,7 @@ it('handles buffer sessions that previously had an error', async () => { // Waiting for max life should eventually stop recording // We simulate a full checkout which would otherwise be done automatically - for (let i = 0; i < MAX_SESSION_LIFE / 60_000; i++) { + for (let i = 0; i < MAX_REPLAY_DURATION / 60_000; i++) { jest.advanceTimersByTime(60_000); await new Promise(process.nextTick); mockRecord.takeFullSnapshot(true); @@ -997,7 +997,7 @@ it('handles buffer sessions that never had an error', async () => { // Waiting for max life should eventually stop recording // We simulate a full checkout which would otherwise be done automatically - for (let i = 0; i < MAX_SESSION_LIFE / 60_000; i++) { + for (let i = 0; i < MAX_REPLAY_DURATION / 60_000; i++) { jest.advanceTimersByTime(60_000); await new Promise(process.nextTick); mockRecord.takeFullSnapshot(true); diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index e78032daaf8f..f29e1fa0390a 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -1,6 +1,6 @@ import * as SentryUtils from '@sentry/utils'; -import { DEFAULT_FLUSH_MIN_DELAY, MAX_SESSION_LIFE, WINDOW } from '../../src/constants'; +import { DEFAULT_FLUSH_MIN_DELAY, MAX_REPLAY_DURATION, WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/replay'; import { clearSession } from '../../src/session/clearSession'; import type { EventBuffer } from '../../src/types'; @@ -305,7 +305,7 @@ describe('Integration | flush', () => { }); it('does not flush if session is too long', async () => { - replay.timeouts.maxSessionLife = 100_000; + replay.getOptions().maxReplayDuration = 100_000; jest.setSystemTime(BASE_TIMESTAMP); sessionStorage.clear(); @@ -335,7 +335,7 @@ describe('Integration | flush', () => { expect(mockFlush).toHaveBeenCalledTimes(1); expect(mockSendReplay).toHaveBeenCalledTimes(0); - replay.timeouts.maxSessionLife = MAX_SESSION_LIFE; + replay.getOptions().maxReplayDuration = MAX_REPLAY_DURATION; replay['_checkSession'] = _tmp; }); @@ -400,7 +400,7 @@ describe('Integration | flush', () => { replay.getOptions()._experiments.traceInternals = false; }); - it('logs warning if adding event that is after maxSessionLife', async () => { + it('logs warning if adding event that is after maxReplayDuration', async () => { replay.getOptions()._experiments.traceInternals = true; sessionStorage.clear(); @@ -416,7 +416,7 @@ describe('Integration | flush', () => { replay.eventBuffer!.hasCheckout = true; // Add event that is too long after session start - const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP + MAX_SESSION_LIFE + 100 }); + const TEST_EVENT = getTestEventCheckout({ timestamp: BASE_TIMESTAMP + MAX_REPLAY_DURATION + 100 }); mockRecord._emitter(TEST_EVENT); // no checkout! @@ -440,8 +440,8 @@ describe('Integration | flush', () => { data: { logger: 'replay' }, level: 'info', message: `[Replay] Skipping event with timestamp ${ - BASE_TIMESTAMP + MAX_SESSION_LIFE + 100 - } because it is after maxSessionLife`, + BASE_TIMESTAMP + MAX_REPLAY_DURATION + 100 + } because it is after maxReplayDuration`, }, }, }, @@ -456,8 +456,8 @@ describe('Integration | flush', () => { * so by the time we actually send the replay it's too late. * In this case, we want to stop the replay. */ - it('stops if flushing after maxSessionLife', async () => { - replay.timeouts.maxSessionLife = 100_000; + it('stops if flushing after maxReplayDuration', async () => { + replay.getOptions().maxReplayDuration = 100_000; sessionStorage.clear(); clearSession(replay); @@ -486,7 +486,7 @@ describe('Integration | flush', () => { expect(mockSendReplay).toHaveBeenCalledTimes(0); expect(replay.isEnabled()).toBe(false); - replay.timeouts.maxSessionLife = MAX_SESSION_LIFE; + replay.getOptions().maxReplayDuration = MAX_REPLAY_DURATION; // Start again for following tests await replay.start(); diff --git a/packages/replay/test/integration/session.test.ts b/packages/replay/test/integration/session.test.ts index b0ea37e4e0f2..ca4e16be8c85 100644 --- a/packages/replay/test/integration/session.test.ts +++ b/packages/replay/test/integration/session.test.ts @@ -3,7 +3,7 @@ import type { Transport } from '@sentry/types'; import { DEFAULT_FLUSH_MIN_DELAY, - MAX_SESSION_LIFE, + MAX_REPLAY_DURATION, REPLAY_SESSION_KEY, SESSION_IDLE_EXPIRE_DURATION, SESSION_IDLE_PAUSE_DURATION, @@ -332,7 +332,7 @@ describe('Integration | session', () => { expect(replay.session).toBe(undefined); }); - it('creates a new session if current session exceeds MAX_SESSION_LIFE', async () => { + it('creates a new session if current session exceeds MAX_REPLAY_DURATION', async () => { jest.clearAllMocks(); const initialSession = { ...replay.session } as Session; @@ -350,8 +350,8 @@ describe('Integration | session', () => { value: new URL(url), }); - // Advanced past MAX_SESSION_LIFE - const ELAPSED = MAX_SESSION_LIFE + 1; + // Advanced past MAX_REPLAY_DURATION + const ELAPSED = MAX_REPLAY_DURATION + 1; jest.advanceTimersByTime(ELAPSED); // Update activity so as to not consider session to be idling replay['_updateUserActivity'](); diff --git a/packages/replay/test/unit/session/loadOrCreateSession.test.ts b/packages/replay/test/unit/session/loadOrCreateSession.test.ts index 907e078c75d3..8f6e7a071c9c 100644 --- a/packages/replay/test/unit/session/loadOrCreateSession.test.ts +++ b/packages/replay/test/unit/session/loadOrCreateSession.test.ts @@ -1,15 +1,10 @@ -import { - MAX_SESSION_LIFE, - SESSION_IDLE_EXPIRE_DURATION, - SESSION_IDLE_PAUSE_DURATION, - WINDOW, -} from '../../../src/constants'; +import { MAX_REPLAY_DURATION, SESSION_IDLE_EXPIRE_DURATION, WINDOW } from '../../../src/constants'; import * as CreateSession from '../../../src/session/createSession'; import * as FetchSession from '../../../src/session/fetchSession'; import { loadOrCreateSession } from '../../../src/session/loadOrCreateSession'; import { saveSession } from '../../../src/session/saveSession'; import { makeSession } from '../../../src/session/Session'; -import type { SessionOptions, Timeouts } from '../../../src/types'; +import type { SessionOptions } from '../../../src/types'; jest.mock('@sentry/utils', () => { return { @@ -24,10 +19,9 @@ const SAMPLE_OPTIONS: SessionOptions = { allowBuffering: false, }; -const timeouts: Timeouts = { - sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, +const DEFAULT_OPTIONS = { sessionIdleExpire: SESSION_IDLE_EXPIRE_DURATION, - maxSessionLife: MAX_SESSION_LIFE, + maxReplayDuration: MAX_REPLAY_DURATION, }; function createMockSession(when: number = Date.now(), id = 'test_session_id') { @@ -59,7 +53,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -90,7 +84,8 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -121,7 +116,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( currentSession, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -141,7 +136,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -174,7 +169,8 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -207,7 +203,8 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts: { ...timeouts, sessionIdleExpire: 5000 }, + sessionIdleExpire: 5000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -236,7 +233,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( currentSession, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -266,7 +263,8 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -293,7 +291,8 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -322,7 +321,8 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 5000 }, + sessionIdleExpire: 5000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -341,7 +341,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -365,7 +365,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -381,7 +381,7 @@ describe('Unit | session | loadOrCreateSession', () => { const session = loadOrCreateSession( undefined, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, diff --git a/packages/replay/test/unit/session/maybeRefreshSession.test.ts b/packages/replay/test/unit/session/maybeRefreshSession.test.ts index 5bcc8bf4481c..c4399a5e1188 100644 --- a/packages/replay/test/unit/session/maybeRefreshSession.test.ts +++ b/packages/replay/test/unit/session/maybeRefreshSession.test.ts @@ -1,13 +1,8 @@ -import { - MAX_SESSION_LIFE, - SESSION_IDLE_EXPIRE_DURATION, - SESSION_IDLE_PAUSE_DURATION, - WINDOW, -} from '../../../src/constants'; +import { MAX_REPLAY_DURATION, SESSION_IDLE_EXPIRE_DURATION, WINDOW } from '../../../src/constants'; import * as CreateSession from '../../../src/session/createSession'; import { maybeRefreshSession } from '../../../src/session/maybeRefreshSession'; import { makeSession } from '../../../src/session/Session'; -import type { SessionOptions, Timeouts } from '../../../src/types'; +import type { SessionOptions } from '../../../src/types'; jest.mock('@sentry/utils', () => { return { @@ -22,10 +17,9 @@ const SAMPLE_OPTIONS: SessionOptions = { allowBuffering: false, }; -const timeouts: Timeouts = { - sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, +const DEFAULT_OPTIONS = { sessionIdleExpire: SESSION_IDLE_EXPIRE_DURATION, - maxSessionLife: MAX_SESSION_LIFE, + maxReplayDuration: MAX_REPLAY_DURATION, }; function createMockSession(when: number = Date.now(), id = 'test_session_id') { @@ -56,7 +50,7 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts, + ...DEFAULT_OPTIONS, }, { ...SAMPLE_OPTIONS, @@ -75,7 +69,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -114,7 +109,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -140,7 +136,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -168,7 +165,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 5000 }, + sessionIdleExpire: 5000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -196,7 +194,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -223,7 +222,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, @@ -250,7 +250,8 @@ describe('Unit | session | maybeRefreshSession', () => { const session = maybeRefreshSession( currentSession, { - timeouts: { ...timeouts, sessionIdleExpire: 1000 }, + sessionIdleExpire: 1000, + maxReplayDuration: MAX_REPLAY_DURATION, }, { ...SAMPLE_OPTIONS, diff --git a/packages/replay/test/unit/util/isSessionExpired.test.ts b/packages/replay/test/unit/util/isSessionExpired.test.ts index 38b24056d36f..2ff7e7ef2989 100644 --- a/packages/replay/test/unit/util/isSessionExpired.test.ts +++ b/packages/replay/test/unit/util/isSessionExpired.test.ts @@ -1,4 +1,4 @@ -import { MAX_SESSION_LIFE, SESSION_IDLE_PAUSE_DURATION } from '../../../src/constants'; +import { MAX_REPLAY_DURATION } from '../../../src/constants'; import { makeSession } from '../../../src/session/Session'; import { isSessionExpired } from '../../../src/util/isSessionExpired'; @@ -16,57 +16,41 @@ function createSession(extra?: Record) { describe('Unit | util | isSessionExpired', () => { it('session last activity is older than expiry time', function () { expect( - isSessionExpired( - createSession(), - { - maxSessionLife: MAX_SESSION_LIFE, - sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, - sessionIdleExpire: 100, - }, - 200, - ), + isSessionExpired(createSession(), { + maxReplayDuration: MAX_REPLAY_DURATION, + sessionIdleExpire: 100, + targetTime: 200, + }), ).toBe(true); // Session expired at ts = 100 }); it('session last activity is not older than expiry time', function () { expect( - isSessionExpired( - createSession({ lastActivity: 100 }), - { - maxSessionLife: MAX_SESSION_LIFE, - sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, - sessionIdleExpire: 150, - }, - 200, - ), + isSessionExpired(createSession({ lastActivity: 100 }), { + maxReplayDuration: MAX_REPLAY_DURATION, + sessionIdleExpire: 150, + targetTime: 200, + }), ).toBe(false); // Session expires at ts >= 250 }); it('session age is not older than max session life', function () { expect( - isSessionExpired( - createSession(), - { - maxSessionLife: MAX_SESSION_LIFE, - sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, - sessionIdleExpire: 1_800_000, - }, - 50_000, - ), + isSessionExpired(createSession(), { + maxReplayDuration: MAX_REPLAY_DURATION, + sessionIdleExpire: 1_800_000, + targetTime: 50_000, + }), ).toBe(false); }); it('session age is older than max session life', function () { expect( - isSessionExpired( - createSession(), - { - maxSessionLife: MAX_SESSION_LIFE, - sessionIdlePause: SESSION_IDLE_PAUSE_DURATION, - sessionIdleExpire: 1_800_000, - }, - 1_800_001, - ), + isSessionExpired(createSession(), { + maxReplayDuration: MAX_REPLAY_DURATION, + sessionIdleExpire: 1_800_000, + targetTime: 1_800_001, + }), ).toBe(true); // Session expires at ts >= 1_800_000 }); }); diff --git a/packages/replay/test/utils/setupReplayContainer.ts b/packages/replay/test/utils/setupReplayContainer.ts index cb70c85bbe54..89bb48210673 100644 --- a/packages/replay/test/utils/setupReplayContainer.ts +++ b/packages/replay/test/utils/setupReplayContainer.ts @@ -1,3 +1,4 @@ +import { MAX_REPLAY_DURATION } from '../../src/constants'; import { createEventBuffer } from '../../src/eventBuffer'; import { ReplayContainer } from '../../src/replay'; import { clearSession } from '../../src/session/clearSession'; @@ -7,6 +8,7 @@ const DEFAULT_OPTIONS = { flushMinDelay: 100, flushMaxDelay: 100, minReplayDuration: 0, + maxReplayDuration: MAX_REPLAY_DURATION, stickySession: false, sessionSampleRate: 0, errorSampleRate: 1, diff --git a/yarn.lock b/yarn.lock index 7228ceab27d3..75861c3324fc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4414,10 +4414,10 @@ fflate "^0.4.4" mitt "^1.1.3" -"@sentry/bundler-plugin-core@0.6.0": - version "0.6.0" - resolved "https://registry.yarnpkg.com/@sentry/bundler-plugin-core/-/bundler-plugin-core-0.6.0.tgz#70ad3740b2f90cdca1aff5fdbcd7306566a2f51e" - integrity sha512-gDPBkFxiOkc525U9pxnGMI5B2DAG0+UCsNuiNgl9+AieDcPSYTwdzfGHytxDZrQgPMvIHEnTAp1VlNB+6UxUGQ== +"@sentry/bundler-plugin-core@0.6.1": + version "0.6.1" + resolved "https://registry.yarnpkg.com/@sentry/bundler-plugin-core/-/bundler-plugin-core-0.6.1.tgz#6c6a2ff3cdc98cd0ff1c30c59408cee9f067adf2" + integrity sha512-EecCJKp9ERM7J93DNDJTvkY78UiD/IfOjBdXWnaUVE0n619O7LfMVjwlXzxRJKl2x05dBE3lDraILLDGxCf6fg== dependencies: "@sentry/cli" "^2.17.0" "@sentry/node" "^7.19.0" @@ -4463,12 +4463,12 @@ proxy-from-env "^1.1.0" which "^2.0.2" -"@sentry/vite-plugin@^0.6.0": - version "0.6.0" - resolved "https://registry.yarnpkg.com/@sentry/vite-plugin/-/vite-plugin-0.6.0.tgz#3902a5224d52b06d753a1deeb6b722bf6523840c" - integrity sha512-3J1ESvbI5okGJaSWm+gTAOOIa96u4ZwfI/C3n+0HSStz3e4vGiGUh59iNyb1/2m5HFgR5OLaHNfAvlyP8GM/ew== +"@sentry/vite-plugin@^0.6.1": + version "0.6.1" + resolved "https://registry.yarnpkg.com/@sentry/vite-plugin/-/vite-plugin-0.6.1.tgz#31eb744e8d87b1528eed8d41433647727a62e7c0" + integrity sha512-qkvKaSOcNhNWcdxRXLSs+8cF3ey0XIRmEzTl8U7sTTcZwuOMHsJB+HsYij6aTGaqsKfP8w1ozVt9szBAiL4//w== dependencies: - "@sentry/bundler-plugin-core" "0.6.0" + "@sentry/bundler-plugin-core" "0.6.1" "@sentry/webpack-plugin@1.19.0": version "1.19.0" From 065ffef715eb0393f10bb158ae68bddec4739bb9 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 31 Aug 2023 15:50:04 +0100 Subject: [PATCH 10/29] chore(remix): Add instructions when a release version can't be proposed. (#8915) --- packages/remix/scripts/createRelease.js | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/remix/scripts/createRelease.js b/packages/remix/scripts/createRelease.js index a04fdf67a693..3e9ca11a931f 100644 --- a/packages/remix/scripts/createRelease.js +++ b/packages/remix/scripts/createRelease.js @@ -6,17 +6,30 @@ const { deleteSourcemaps } = require('./deleteSourcemaps'); const sentry = new SentryCli(); async function createRelease(argv, URL_PREFIX, BUILD_PATH) { - const RELEASE = argv.release || (await sentry.releases.proposeVersion()); + let release; - await sentry.releases.new(RELEASE); + if (!argv.release) { + try { + release = await sentry.releases.proposeVersion(); + } catch (error) { + console.warn('[sentry] Failed to propose a release version.'); + console.warn('[sentry] You can specify a release version with `--release` flag.'); + console.warn('[sentry] For example: `sentry-upload-sourcemaps --release 1.0.0`'); + throw error; + } + } else { + release = argv.release; + } + + await sentry.releases.new(release); - await sentry.releases.uploadSourceMaps(RELEASE, { + await sentry.releases.uploadSourceMaps(release, { urlPrefix: URL_PREFIX, include: [BUILD_PATH], useArtifactBundle: !argv.disableDebugIds, }); - await sentry.releases.finalize(RELEASE); + await sentry.releases.finalize(release); if (argv.deleteAfterUpload) { try { From 91c48c99ead06de91e5815fbcf3648c6e0af4741 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 31 Aug 2023 14:49:45 +0000 Subject: [PATCH 11/29] build(deps): bump apollo-server-core from 3.11.1 to 3.12.1 Bumps [apollo-server-core](https://github.com/apollographql/apollo-server/tree/HEAD/packages/apollo-server-core) from 3.11.1 to 3.12.1. - [Release notes](https://github.com/apollographql/apollo-server/releases) - [Commits](https://github.com/apollographql/apollo-server/commits/apollo-server-core@3.12.1/packages/apollo-server-core) --- updated-dependencies: - dependency-name: apollo-server-core dependency-type: indirect ... Signed-off-by: dependabot[bot] --- yarn.lock | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/yarn.lock b/yarn.lock index 75861c3324fc..40f7682f22b7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6705,10 +6705,17 @@ apollo-reporting-protobuf@^3.3.3: dependencies: "@apollo/protobufjs" "1.2.6" +apollo-reporting-protobuf@^3.4.0: + version "3.4.0" + resolved "https://registry.yarnpkg.com/apollo-reporting-protobuf/-/apollo-reporting-protobuf-3.4.0.tgz#6edd31f09d4a3704d9e808d1db30eca2229ded26" + integrity sha512-h0u3EbC/9RpihWOmcSsvTW2O6RXVaD/mPEjfrPkxRPTEPWqncsgOoRJw+wih4OqfH3PvTJvoEIf4LwKrUaqWog== + dependencies: + "@apollo/protobufjs" "1.2.6" + apollo-server-core@^3.11.1: - version "3.11.1" - resolved "https://registry.yarnpkg.com/apollo-server-core/-/apollo-server-core-3.11.1.tgz#89d83aeaa71a59f760ebfa35bb0cbd31e15474ca" - integrity sha512-t/eCKrRFK1lYZlc5pHD99iG7Np7CEm3SmbDiONA7fckR3EaB/pdsEdIkIwQ5QBBpT5JLp/nwvrZRVwhaWmaRvw== + version "3.12.1" + resolved "https://registry.yarnpkg.com/apollo-server-core/-/apollo-server-core-3.12.1.tgz#ba255c37345db29c48a2e0c064c519a8d62eb5af" + integrity sha512-9SF5WAkkV0FZQ2HVUWI9Jada1U0jg7e8NCN9EklbtvaCeUlOLyXyM+KCWuZ7+dqHxjshbtcwylPHutt3uzoNkw== dependencies: "@apollo/utils.keyvaluecache" "^1.0.1" "@apollo/utils.logger" "^1.0.0" @@ -6719,11 +6726,11 @@ apollo-server-core@^3.11.1: "@graphql-tools/schema" "^8.0.0" "@josephg/resolvable" "^1.0.0" apollo-datasource "^3.3.2" - apollo-reporting-protobuf "^3.3.3" + apollo-reporting-protobuf "^3.4.0" apollo-server-env "^4.2.1" apollo-server-errors "^3.3.1" - apollo-server-plugin-base "^3.7.1" - apollo-server-types "^3.7.1" + apollo-server-plugin-base "^3.7.2" + apollo-server-types "^3.8.0" async-retry "^1.2.1" fast-json-stable-stringify "^2.1.0" graphql-tag "^2.11.0" @@ -6763,12 +6770,12 @@ apollo-server-express@^3.11.1: cors "^2.8.5" parseurl "^1.3.3" -apollo-server-plugin-base@^3.7.1: - version "3.7.1" - resolved "https://registry.yarnpkg.com/apollo-server-plugin-base/-/apollo-server-plugin-base-3.7.1.tgz#aa78ef49bd114e35906ca9cf7493fed2664cbde8" - integrity sha512-g3vJStmQtQvjGI289UkLMfThmOEOddpVgHLHT2bNj0sCD/bbisj4xKbBHETqaURokteqSWyyd4RDTUe0wAUDNQ== +apollo-server-plugin-base@^3.7.2: + version "3.7.2" + resolved "https://registry.yarnpkg.com/apollo-server-plugin-base/-/apollo-server-plugin-base-3.7.2.tgz#c19cd137bc4c993ba2490ba2b571b0f3ce60a0cd" + integrity sha512-wE8dwGDvBOGehSsPTRZ8P/33Jan6/PmL0y0aN/1Z5a5GcbFhDaaJCjK5cav6npbbGL2DPKK0r6MPXi3k3N45aw== dependencies: - apollo-server-types "^3.7.1" + apollo-server-types "^3.8.0" apollo-server-types@^3.7.1: version "3.7.1" @@ -6780,6 +6787,16 @@ apollo-server-types@^3.7.1: apollo-reporting-protobuf "^3.3.3" apollo-server-env "^4.2.1" +apollo-server-types@^3.8.0: + version "3.8.0" + resolved "https://registry.yarnpkg.com/apollo-server-types/-/apollo-server-types-3.8.0.tgz#d976b6967878681f715fe2b9e4dad9ba86b1346f" + integrity sha512-ZI/8rTE4ww8BHktsVpb91Sdq7Cb71rdSkXELSwdSR0eXu600/sY+1UXhTWdiJvk+Eq5ljqoHLwLbY2+Clq2b9A== + dependencies: + "@apollo/utils.keyvaluecache" "^1.0.1" + "@apollo/utils.logger" "^1.0.0" + apollo-reporting-protobuf "^3.4.0" + apollo-server-env "^4.2.1" + apollo-server@^3.11.1: version "3.11.1" resolved "https://registry.yarnpkg.com/apollo-server/-/apollo-server-3.11.1.tgz#831646081323aadf2cb53cdc3401786e41d44d81" From 9666506f893827e5da15ce9c35265ab901364f23 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 1 Sep 2023 09:49:03 +0200 Subject: [PATCH 12/29] ref(node-otel): Avoid exporting internals & refactor attribute adding (#8920) I noticed that we kind of extended the public API by exporting some things from opentelemetry-node that we need in node-experimental. I am deprecating these internals as I'm refactoring some things to be a bit cleaner IMHO. The one export we actually need outside I am exporting as `_INTERNAL_getSentrySpan` to make it clear this is not meant to be used otherwise. The main reason I am not just exporting this is that this _may_ have to change if we change how we internally store/handle spans - e.g. if we move away from the map this is just not possible to have anymore, and I do not want to lock us into having to support this in the future. --- .../src/integrations/express.ts | 6 +- .../src/integrations/fastify.ts | 6 +- .../src/integrations/graphql.ts | 6 +- .../src/integrations/http.ts | 56 +++++++++-------- .../src/integrations/mongo.ts | 6 +- .../src/integrations/mongoose.ts | 6 +- .../src/integrations/mysql2.ts | 6 +- .../src/integrations/postgres.ts | 6 +- .../src/utils/addOriginToSpan.ts | 13 ++++ packages/opentelemetry-node/src/index.ts | 18 +++++- .../opentelemetry-node/src/spanprocessor.ts | 49 +++------------ .../opentelemetry-node/src/utils/spanData.ts | 61 ++++++++++++++++--- 12 files changed, 133 insertions(+), 106 deletions(-) create mode 100644 packages/node-experimental/src/utils/addOriginToSpan.ts diff --git a/packages/node-experimental/src/integrations/express.ts b/packages/node-experimental/src/integrations/express.ts index 9c30d70f4be8..95b9527c8498 100644 --- a/packages/node-experimental/src/integrations/express.ts +++ b/packages/node-experimental/src/integrations/express.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -31,9 +31,7 @@ export class Express extends NodePerformanceIntegration implements Integra return [ new ExpressInstrumentation({ requestHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.http.otel.express', - }); + addOriginToOtelSpan(span, 'auto.http.otel.express'); }, }), ]; diff --git a/packages/node-experimental/src/integrations/fastify.ts b/packages/node-experimental/src/integrations/fastify.ts index e1098d8d89b9..b84301967616 100644 --- a/packages/node-experimental/src/integrations/fastify.ts +++ b/packages/node-experimental/src/integrations/fastify.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { FastifyInstrumentation } from '@opentelemetry/instrumentation-fastify'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -31,9 +31,7 @@ export class Fastify extends NodePerformanceIntegration implements Integra return [ new FastifyInstrumentation({ requestHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.http.otel.fastify', - }); + addOriginToOtelSpan(span, 'auto.http.otel.fastify'); }, }), ]; diff --git a/packages/node-experimental/src/integrations/graphql.ts b/packages/node-experimental/src/integrations/graphql.ts index 1515f69b6516..87749a0f54a2 100644 --- a/packages/node-experimental/src/integrations/graphql.ts +++ b/packages/node-experimental/src/integrations/graphql.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { GraphQLInstrumentation } from '@opentelemetry/instrumentation-graphql'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -32,9 +32,7 @@ export class GraphQL extends NodePerformanceIntegration implements Integra new GraphQLInstrumentation({ ignoreTrivialResolveSpans: true, responseHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.graphql.otel-graphql', - }); + addOriginToOtelSpan(span, 'auto.graphql.otel.graphql'); }, }), ]; diff --git a/packages/node-experimental/src/integrations/http.ts b/packages/node-experimental/src/integrations/http.ts index 6c31f9bce497..5f45334da290 100644 --- a/packages/node-experimental/src/integrations/http.ts +++ b/packages/node-experimental/src/integrations/http.ts @@ -4,10 +4,9 @@ import { registerInstrumentations } from '@opentelemetry/instrumentation'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-node'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import { hasTracingEnabled } from '@sentry/core'; +import { hasTracingEnabled, Transaction } from '@sentry/core'; import { getCurrentHub } from '@sentry/node'; -import type { AdditionalOtelSpanData } from '@sentry/opentelemetry-node'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; +import { _INTERNAL_getSentrySpan } from '@sentry/opentelemetry-node'; import type { EventProcessor, Hub, Integration } from '@sentry/types'; import type { ClientRequest, IncomingMessage, ServerResponse } from 'http'; @@ -146,34 +145,37 @@ export class Http implements Integration { const data = getRequestSpanData(span, request, response); const { attributes } = span; - const additionalData: AdditionalOtelSpanData = { - tags: {}, - data: { + const sentrySpan = _INTERNAL_getSentrySpan(span.spanContext().spanId); + if (sentrySpan) { + sentrySpan.origin = 'auto.http.otel.http'; + + const additionalData: Record = { url: data.url, - }, - contexts: {}, - metadata: {}, - origin: 'auto.http.otel.http', - }; - - if (span.kind === SpanKind.SERVER) { - additionalData.metadata = { request }; - } + }; - if (attributes[SemanticAttributes.HTTP_STATUS_CODE]) { - const statusCode = attributes[SemanticAttributes.HTTP_STATUS_CODE] as string; - additionalData.tags['http.status_code'] = statusCode; - additionalData.data['http.response.status_code'] = statusCode; - } + if (sentrySpan instanceof Transaction && span.kind === SpanKind.SERVER) { + sentrySpan.setMetadata({ request }); + } - if (data['http.query']) { - additionalData.data['http.query'] = data['http.query'].slice(1); - } - if (data['http.fragment']) { - additionalData.data['http.fragment'] = data['http.fragment'].slice(1); - } + if (attributes[SemanticAttributes.HTTP_STATUS_CODE]) { + const statusCode = attributes[SemanticAttributes.HTTP_STATUS_CODE] as string; + additionalData['http.response.status_code'] = statusCode; - addOtelSpanData(span.spanContext().spanId, additionalData); + sentrySpan.setTag('http.status_code', statusCode); + } + + if (data['http.query']) { + additionalData['http.query'] = data['http.query'].slice(1); + } + if (data['http.fragment']) { + additionalData['http.fragment'] = data['http.fragment'].slice(1); + } + + Object.keys(additionalData).forEach(prop => { + const value = additionalData[prop]; + sentrySpan.setData(prop, value); + }); + } if (this._breadcrumbs) { getCurrentHub().addBreadcrumb( diff --git a/packages/node-experimental/src/integrations/mongo.ts b/packages/node-experimental/src/integrations/mongo.ts index 2b8752d770ad..aea5d0a7d3fb 100644 --- a/packages/node-experimental/src/integrations/mongo.ts +++ b/packages/node-experimental/src/integrations/mongo.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { MongoDBInstrumentation } from '@opentelemetry/instrumentation-mongodb'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -31,9 +31,7 @@ export class Mongo extends NodePerformanceIntegration implements Integrati return [ new MongoDBInstrumentation({ responseHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.db.otel-mongo', - }); + addOriginToOtelSpan(span, 'auto.db.otel.mongo'); }, }), ]; diff --git a/packages/node-experimental/src/integrations/mongoose.ts b/packages/node-experimental/src/integrations/mongoose.ts index 42d863966ea3..8f6eb65adb8b 100644 --- a/packages/node-experimental/src/integrations/mongoose.ts +++ b/packages/node-experimental/src/integrations/mongoose.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { MongooseInstrumentation } from '@opentelemetry/instrumentation-mongoose'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -31,9 +31,7 @@ export class Mongoose extends NodePerformanceIntegration implements Integr return [ new MongooseInstrumentation({ responseHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.db.otel-mongoose', - }); + addOriginToOtelSpan(span, 'auto.db.otel.mongoose'); }, }), ]; diff --git a/packages/node-experimental/src/integrations/mysql2.ts b/packages/node-experimental/src/integrations/mysql2.ts index f7d8f2c96fc9..b78b56bdd0ab 100644 --- a/packages/node-experimental/src/integrations/mysql2.ts +++ b/packages/node-experimental/src/integrations/mysql2.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { MySQL2Instrumentation } from '@opentelemetry/instrumentation-mysql2'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -31,9 +31,7 @@ export class Mysql2 extends NodePerformanceIntegration implements Integrat return [ new MySQL2Instrumentation({ responseHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.db.otel-mysql2', - }); + addOriginToOtelSpan(span, 'auto.db.otel.mysql2'); }, }), ]; diff --git a/packages/node-experimental/src/integrations/postgres.ts b/packages/node-experimental/src/integrations/postgres.ts index f6d1414a7fc0..0df7ae31d8ae 100644 --- a/packages/node-experimental/src/integrations/postgres.ts +++ b/packages/node-experimental/src/integrations/postgres.ts @@ -1,8 +1,8 @@ import type { Instrumentation } from '@opentelemetry/instrumentation'; import { PgInstrumentation } from '@opentelemetry/instrumentation-pg'; -import { addOtelSpanData } from '@sentry/opentelemetry-node'; import type { Integration } from '@sentry/types'; +import { addOriginToOtelSpan } from '../utils/addOriginToSpan'; import { NodePerformanceIntegration } from './NodePerformanceIntegration'; /** @@ -31,9 +31,7 @@ export class Postgres extends NodePerformanceIntegration implements Integr return [ new PgInstrumentation({ requestHook(span) { - addOtelSpanData(span.spanContext().spanId, { - origin: 'auto.db.otel-postgres', - }); + addOriginToOtelSpan(span, 'auto.db.otel.postgres'); }, }), ]; diff --git a/packages/node-experimental/src/utils/addOriginToSpan.ts b/packages/node-experimental/src/utils/addOriginToSpan.ts new file mode 100644 index 000000000000..c327043f0816 --- /dev/null +++ b/packages/node-experimental/src/utils/addOriginToSpan.ts @@ -0,0 +1,13 @@ +import type { Span as OtelSpan } from '@opentelemetry/api'; +import { _INTERNAL_getSentrySpan } from '@sentry/opentelemetry-node'; +import type { SpanOrigin } from '@sentry/types'; + +/** Adds an origin to an OTEL Span. */ +export function addOriginToOtelSpan(otelSpan: OtelSpan, origin: SpanOrigin): void { + const sentrySpan = _INTERNAL_getSentrySpan(otelSpan.spanContext().spanId); + if (!sentrySpan) { + return; + } + + sentrySpan.origin = origin; +} diff --git a/packages/opentelemetry-node/src/index.ts b/packages/opentelemetry-node/src/index.ts index 4f68820d23a2..24c477a968fd 100644 --- a/packages/opentelemetry-node/src/index.ts +++ b/packages/opentelemetry-node/src/index.ts @@ -1,3 +1,19 @@ +import { getSentrySpan } from './spanprocessor'; + export { SentrySpanProcessor } from './spanprocessor'; export { SentryPropagator } from './propagator'; -export * from './utils/spanData'; + +/* eslint-disable deprecation/deprecation */ +export { addOtelSpanData, getOtelSpanData, clearOtelSpanData } from './utils/spanData'; +export type { AdditionalOtelSpanData } from './utils/spanData'; +/* eslint-enable deprecation/deprecation */ + +/** + * This is only exported for internal use. + * Semver etc. does not apply here, this is subject to change at any time! + * This is explicitly _NOT_ public because we may have to change the underlying way we store/handle spans, + * which may make this API unusable without further notice. + * + * @private + */ +export { getSentrySpan as _INTERNAL_getSentrySpan }; diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index 0e208b050357..40757160663f 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -10,19 +10,19 @@ import { SENTRY_DYNAMIC_SAMPLING_CONTEXT_KEY, SENTRY_TRACE_PARENT_CONTEXT_KEY } import { isSentryRequestSpan } from './utils/isSentryRequest'; import { mapOtelStatus } from './utils/mapOtelStatus'; import { parseSpanDescription } from './utils/parseOtelSpanDescription'; -import { clearOtelSpanData, getOtelSpanData } from './utils/spanData'; -export const SENTRY_SPAN_PROCESSOR_MAP: Map = new Map< - SentrySpan['spanId'], - SentrySpan ->(); +export const SENTRY_SPAN_PROCESSOR_MAP: Map = new Map(); // make sure to remove references in maps, to ensure this can be GCed function clearSpan(otelSpanId: string): void { - clearOtelSpanData(otelSpanId); SENTRY_SPAN_PROCESSOR_MAP.delete(otelSpanId); } +/** Get a Sentry span for an otel span ID. */ +export function getSentrySpan(otelSpanId: string): SentrySpan | undefined { + return SENTRY_SPAN_PROCESSOR_MAP.get(otelSpanId); +} + /** * Converts OpenTelemetry Spans to Sentry Spans and sends them to Sentry via * the Sentry SDK. @@ -225,18 +225,10 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi const { op, description, data } = parseSpanDescription(otelSpan); - const { data: additionalData, tags, origin } = getOtelSpanData(otelSpan.spanContext().spanId); - sentrySpan.setStatus(mapOtelStatus(otelSpan)); sentrySpan.setData('otel.kind', SpanKind[kind]); - if (tags) { - Object.keys(tags).forEach(prop => { - sentrySpan.setTag(prop, tags[prop]); - }); - } - - const allData = { ...attributes, ...data, ...additionalData }; + const allData = { ...attributes, ...data }; Object.keys(allData).forEach(prop => { const value = allData[prop]; @@ -245,52 +237,27 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi sentrySpan.op = op; sentrySpan.description = description; - - if (origin) { - sentrySpan.origin = origin; - } } function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void { const { op, description, source, data } = parseSpanDescription(otelSpan); - const { data: additionalData, tags, contexts, metadata, origin } = getOtelSpanData(otelSpan.spanContext().spanId); transaction.setContext('otel', { attributes: otelSpan.attributes, resource: otelSpan.resource.attributes, }); - if (tags) { - Object.keys(tags).forEach(prop => { - transaction.setTag(prop, tags[prop]); - }); - } - - if (metadata) { - transaction.setMetadata(metadata); - } - - const allData = { ...data, ...additionalData }; + const allData = data || {}; Object.keys(allData).forEach(prop => { const value = allData[prop]; transaction.setData(prop, value); }); - if (contexts) { - Object.keys(contexts).forEach(prop => { - transaction.setContext(prop, contexts[prop]); - }); - } - transaction.setStatus(mapOtelStatus(otelSpan)); transaction.op = op; transaction.setName(description, source); - - if (origin) { - transaction.origin = origin; - } } function convertOtelTimeToSeconds([seconds, nano]: [number, number]): number { diff --git a/packages/opentelemetry-node/src/utils/spanData.ts b/packages/opentelemetry-node/src/utils/spanData.ts index 74d20b964985..d0e582d5763a 100644 --- a/packages/opentelemetry-node/src/utils/spanData.ts +++ b/packages/opentelemetry-node/src/utils/spanData.ts @@ -1,9 +1,14 @@ +/* eslint-disable deprecation/deprecation */ +import { Transaction } from '@sentry/core'; import type { Context, SpanOrigin } from '@sentry/types'; +import { getSentrySpan } from '../spanprocessor'; + type SentryTags = Record; type SentryData = Record; type Contexts = Record; +/** @deprecated This will be removed in v8. */ export interface AdditionalOtelSpanData { data: SentryData; tags: SentryTags; @@ -14,21 +19,59 @@ export interface AdditionalOtelSpanData { const OTEL_SPAN_DATA_MAP: Map = new Map(); -/** Add data that should be added to the sentry span resulting from the given otel span ID. */ -export function addOtelSpanData(otelSpanId: string, data: Partial): void { - OTEL_SPAN_DATA_MAP.set(otelSpanId, { data: {}, tags: {}, contexts: {}, metadata: {}, ...data }); -} +/** + * Add data that should be added to the sentry span resulting from the given otel span ID. + * @deprecated This will be removed in v8. This was never meant to be public API. + */ +export function addOtelSpanData( + otelSpanId: string, + { data, tags, contexts, metadata, origin }: Partial, +): void { + const sentrySpan = getSentrySpan(otelSpanId); + if (!sentrySpan) { + return; + } -/** Get additional data for a Sentry span. */ -export function getOtelSpanData(otelSpanId: string): AdditionalOtelSpanData { - if (OTEL_SPAN_DATA_MAP.has(otelSpanId)) { - return OTEL_SPAN_DATA_MAP.get(otelSpanId) as AdditionalOtelSpanData; + if (data) { + Object.keys(data).forEach(prop => { + const value = data[prop]; + sentrySpan.setData(prop, value); + }); } + if (tags) { + Object.keys(tags).forEach(prop => { + sentrySpan.setTag(prop, tags[prop]); + }); + } + + if (origin) { + sentrySpan.origin = origin; + } + + if (sentrySpan instanceof Transaction) { + if (metadata) { + sentrySpan.setMetadata(metadata); + } + + if (contexts) { + Object.keys(contexts).forEach(prop => { + sentrySpan.setContext(prop, contexts[prop]); + }); + } + } +} + +/** + * @deprecated This will do nothing and will be removed in v8. + */ +export function getOtelSpanData(_otelSpanId: string): AdditionalOtelSpanData { return { data: {}, tags: {}, contexts: {}, metadata: {} }; } -/** Add data that should be added to the sentry span resulting from the given otel span ID. */ +/** + * @deprecated This will do nothing and will be removed in v8. + */ export function clearOtelSpanData(otelSpanId: string): void { OTEL_SPAN_DATA_MAP.delete(otelSpanId); } From 1ca67d1bb91b0f0462004abf5cc3a8992e0703c8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 1 Sep 2023 09:49:35 +0200 Subject: [PATCH 13/29] fix(node): Improve mysql integration (#8923) 1. Make sure it works consistently when not calling `connection.connect()` (which is optional) - it used to only register the mysql config on the second call, as it calls `connect` under the hood but only after we already tried to fetch it again. 2. Make sure it works without a callback. --- .../mysql/{ => withConnect}/scenario.ts | 0 .../auto-instrument/mysql/withConnect/test.ts | 35 +++++++++++++++++++ .../mysql/withoutCallback/scenario.ts | 31 ++++++++++++++++ .../mysql/withoutCallback/test.ts | 35 +++++++++++++++++++ .../mysql/withoutConnect/scenario.ts | 30 ++++++++++++++++ .../mysql/{ => withoutConnect}/test.ts | 10 ++++-- .../src/node/integrations/mysql.ts | 25 ++++++++++--- 7 files changed, 159 insertions(+), 7 deletions(-) rename packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/{ => withConnect}/scenario.ts (100%) create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withConnect/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/scenario.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/scenario.ts rename packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/{ => withoutConnect}/test.ts (60%) diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withConnect/scenario.ts similarity index 100% rename from packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts rename to packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withConnect/scenario.ts diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withConnect/test.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withConnect/test.ts new file mode 100644 index 000000000000..972c9496216a --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withConnect/test.ts @@ -0,0 +1,35 @@ +import { assertSentryTransaction, TestEnv } from '../../../../../utils'; + +test('should auto-instrument `mysql` package when using connection.connect()', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'Test Transaction', + spans: [ + { + description: 'SELECT 1 + 1 AS solution', + op: 'db', + data: { + 'db.system': 'mysql', + 'server.address': 'localhost', + 'server.port': 3306, + 'db.user': 'root', + }, + }, + + { + description: 'SELECT NOW()', + op: 'db', + data: { + 'db.system': 'mysql', + 'server.address': 'localhost', + 'server.port': 3306, + 'db.user': 'root', + }, + }, + ], + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/scenario.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/scenario.ts new file mode 100644 index 000000000000..a47f05203d35 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/scenario.ts @@ -0,0 +1,31 @@ +import * as Sentry from '@sentry/node'; +import mysql from 'mysql'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations()], +}); + +const connection = mysql.createConnection({ + user: 'root', + password: 'docker', +}); + +const transaction = Sentry.startTransaction({ + op: 'transaction', + name: 'Test Transaction', +}); + +Sentry.configureScope(scope => { + scope.setSpan(transaction); +}); + +connection.query('SELECT 1 + 1 AS solution'); +connection.query('SELECT NOW()', ['1', '2']); + +// Wait a bit to ensure the queries completed +setTimeout(() => { + transaction.finish(); +}, 500); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/test.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/test.ts new file mode 100644 index 000000000000..9e58b59fecad --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutCallback/test.ts @@ -0,0 +1,35 @@ +import { assertSentryTransaction, TestEnv } from '../../../../../utils'; + +test('should auto-instrument `mysql` package when using query without callback', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'Test Transaction', + spans: [ + { + description: 'SELECT 1 + 1 AS solution', + op: 'db', + data: { + 'db.system': 'mysql', + 'server.address': 'localhost', + 'server.port': 3306, + 'db.user': 'root', + }, + }, + + { + description: 'SELECT NOW()', + op: 'db', + data: { + 'db.system': 'mysql', + 'server.address': 'localhost', + 'server.port': 3306, + 'db.user': 'root', + }, + }, + ], + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/scenario.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/scenario.ts new file mode 100644 index 000000000000..c7cc0e660fc4 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/scenario.ts @@ -0,0 +1,30 @@ +import * as Sentry from '@sentry/node'; +import mysql from 'mysql'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations()], +}); + +const connection = mysql.createConnection({ + user: 'root', + password: 'docker', +}); + +const transaction = Sentry.startTransaction({ + op: 'transaction', + name: 'Test Transaction', +}); + +Sentry.configureScope(scope => { + scope.setSpan(transaction); +}); + +connection.query('SELECT 1 + 1 AS solution', function () { + connection.query('SELECT NOW()', ['1', '2'], () => { + if (transaction) transaction.finish(); + connection.end(); + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/test.ts similarity index 60% rename from packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts rename to packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/test.ts index dbdd658f6ef4..85340c2c6fc7 100644 --- a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/withoutConnect/test.ts @@ -1,6 +1,6 @@ -import { assertSentryTransaction, TestEnv } from '../../../../utils'; +import { assertSentryTransaction, TestEnv } from '../../../../../utils'; -test('should auto-instrument `mysql` package.', async () => { +test('should auto-instrument `mysql` package without connection.connect()', async () => { const env = await TestEnv.init(__dirname); const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); @@ -14,6 +14,9 @@ test('should auto-instrument `mysql` package.', async () => { op: 'db', data: { 'db.system': 'mysql', + 'server.address': 'localhost', + 'server.port': 3306, + 'db.user': 'root', }, }, @@ -22,6 +25,9 @@ test('should auto-instrument `mysql` package.', async () => { op: 'db', data: { 'db.system': 'mysql', + 'server.address': 'localhost', + 'server.port': 3306, + 'db.user': 'root', }, }, ], diff --git a/packages/tracing-internal/src/node/integrations/mysql.ts b/packages/tracing-internal/src/node/integrations/mysql.ts index 9c11165c643e..8a3fa0166fd5 100644 --- a/packages/tracing-internal/src/node/integrations/mysql.ts +++ b/packages/tracing-internal/src/node/integrations/mysql.ts @@ -1,5 +1,5 @@ import type { Hub } from '@sentry/core'; -import type { EventProcessor } from '@sentry/types'; +import type { EventProcessor, Span } from '@sentry/types'; import { fill, loadModule, logger } from '@sentry/utils'; import type { LazyLoadedIntegration } from './lazy'; @@ -83,6 +83,19 @@ export class Mysql implements LazyLoadedIntegration { }; } + function finishSpan(span: Span | undefined): void { + if (!span) { + return; + } + + const data = spanDataFromConfig(); + Object.keys(data).forEach(key => { + span.setData(key, data[key]); + }); + + span.finish(); + } + // The original function will have one of these signatures: // function (callback) => void // function (options, callback) => void @@ -91,31 +104,33 @@ export class Mysql implements LazyLoadedIntegration { return function (this: unknown, options: unknown, values: unknown, callback: unknown) { const scope = getCurrentHub().getScope(); const parentSpan = scope?.getSpan(); + const span = parentSpan?.startChild({ description: typeof options === 'string' ? options : (options as { sql: string }).sql, op: 'db', origin: 'auto.db.mysql', data: { - ...spanDataFromConfig(), 'db.system': 'mysql', }, }); if (typeof callback === 'function') { return orig.call(this, options, values, function (err: Error, result: unknown, fields: unknown) { - span?.finish(); + finishSpan(span); callback(err, result, fields); }); } if (typeof values === 'function') { return orig.call(this, options, function (err: Error, result: unknown, fields: unknown) { - span?.finish(); + finishSpan(span); values(err, result, fields); }); } - return orig.call(this, options, values, callback); + return orig.call(this, options, values, function () { + finishSpan(span); + }); }; }); } From 13b47297b42ef5dfc482e6712368e3903174ff85 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 1 Sep 2023 08:52:16 +0100 Subject: [PATCH 14/29] test(e2e): Decode gzipped envelopes in E2E tests (#8926) --- .../test-applications/nextjs-app-dir/.gitignore | 2 ++ .../nextjs-app-dir/event-proxy-server.ts | 14 +++++++++----- .../test-applications/nextjs-app-dir/package.json | 2 +- .../nextjs-app-dir/tests/edge-route.test.ts | 6 ------ .../nextjs-app-dir/tests/middleware.test.ts | 6 ------ .../sveltekit/event-proxy-server.ts | 14 +++++++++----- 6 files changed, 21 insertions(+), 23 deletions(-) diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/.gitignore b/packages/e2e-tests/test-applications/nextjs-app-dir/.gitignore index 35b1048ce099..e799cc33c4e7 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/.gitignore +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/.gitignore @@ -41,3 +41,5 @@ next-env.d.ts .sentryclirc .vscode + +test-results diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/event-proxy-server.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/event-proxy-server.ts index b32910480f38..67cf80b4dabf 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/event-proxy-server.ts +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/event-proxy-server.ts @@ -7,6 +7,7 @@ import type { AddressInfo } from 'net'; import * as os from 'os'; import * as path from 'path'; import * as util from 'util'; +import * as zlib from 'zlib'; const readFile = util.promisify(fs.readFile); const writeFile = util.promisify(fs.writeFile); @@ -44,8 +45,12 @@ export async function startEventProxyServer(options: EventProxyServerOptions): P }); proxyRequest.addListener('end', () => { - const proxyRequestBody = Buffer.concat(proxyRequestChunks).toString(); - const envelopeHeader: { dsn?: string } = JSON.parse(proxyRequestBody.split('\n')[0]); + const proxyRequestBody = + proxyRequest.headers['content-encoding'] === 'gzip' + ? zlib.gunzipSync(Buffer.concat(proxyRequestChunks)).toString() + : Buffer.concat(proxyRequestChunks).toString(); + + let envelopeHeader = JSON.parse(proxyRequestBody.split('\n')[0]); if (!envelopeHeader.dsn) { throw new Error('[event-proxy-server] No dsn on envelope header. Please set tunnel option.'); @@ -71,12 +76,11 @@ export async function startEventProxyServer(options: EventProxyServerOptions): P sentryResponse.addListener('end', () => { eventCallbackListeners.forEach(listener => { - const rawProxyRequestBody = Buffer.concat(proxyRequestChunks).toString(); const rawSentryResponseBody = Buffer.concat(sentryResponseChunks).toString(); const data: SentryRequestCallbackData = { - envelope: parseEnvelope(rawProxyRequestBody, new TextEncoder(), new TextDecoder()), - rawProxyRequestBody, + envelope: parseEnvelope(proxyRequestBody, new TextEncoder(), new TextDecoder()), + rawProxyRequestBody: proxyRequestBody, rawSentryResponseBody, sentryResponseStatusCode: sentryResponse.statusCode, }; diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/package.json b/packages/e2e-tests/test-applications/nextjs-app-dir/package.json index d8701ef8df7c..7edc1429c308 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/package.json +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/package.json @@ -19,7 +19,7 @@ "@types/node": "18.11.17", "@types/react": "18.0.26", "@types/react-dom": "18.0.9", - "next": "13.2.4", + "next": "13.4.19", "react": "18.2.0", "react-dom": "18.2.0", "typescript": "4.9.5", diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts index 7d8d2a2fe97a..794e34973358 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts @@ -2,8 +2,6 @@ import { test, expect } from '@playwright/test'; import { waitForTransaction, waitForError } from '../event-proxy-server'; test('Should create a transaction for edge routes', async ({ request }) => { - test.skip(process.env.TEST_ENV === 'development', "Doesn't work in dev mode."); - const edgerouteTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { return ( transactionEvent?.transaction === 'GET /api/edge-endpoint' && transactionEvent?.contexts?.trace?.status === 'ok' @@ -21,8 +19,6 @@ test('Should create a transaction for edge routes', async ({ request }) => { }); test('Should create a transaction with error status for faulty edge routes', async ({ request }) => { - test.skip(process.env.TEST_ENV === 'development', "Doesn't work in dev mode."); - const edgerouteTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { return ( transactionEvent?.transaction === 'GET /api/error-edge-endpoint' && @@ -42,8 +38,6 @@ test('Should create a transaction with error status for faulty edge routes', asy }); test('Should record exceptions for faulty edge routes', async ({ request }) => { - test.skip(process.env.TEST_ENV === 'development', "Doesn't work in dev mode."); - const errorEventPromise = waitForError('nextjs-13-app-dir', errorEvent => { return errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error'; }); diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/tests/middleware.test.ts b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/middleware.test.ts index 95af8a7a8f29..cef1c30f3f4f 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/tests/middleware.test.ts +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/tests/middleware.test.ts @@ -2,8 +2,6 @@ import { test, expect } from '@playwright/test'; import { waitForTransaction, waitForError } from '../event-proxy-server'; test('Should create a transaction for middleware', async ({ request }) => { - test.skip(process.env.TEST_ENV === 'development', "Doesn't work in dev mode."); - const middlewareTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { return transactionEvent?.transaction === 'middleware' && transactionEvent?.contexts?.trace?.status === 'ok'; }); @@ -19,8 +17,6 @@ test('Should create a transaction for middleware', async ({ request }) => { }); test('Should create a transaction with error status for faulty middleware', async ({ request }) => { - test.skip(process.env.TEST_ENV === 'development', "Doesn't work in dev mode."); - const middlewareTransactionPromise = waitForTransaction('nextjs-13-app-dir', async transactionEvent => { return ( transactionEvent?.transaction === 'middleware' && transactionEvent?.contexts?.trace?.status === 'internal_error' @@ -39,8 +35,6 @@ test('Should create a transaction with error status for faulty middleware', asyn }); test('Records exceptions happening in middleware', async ({ request }) => { - test.skip(process.env.TEST_ENV === 'development', "Doesn't work in dev mode."); - const errorEventPromise = waitForError('nextjs-13-app-dir', errorEvent => { return errorEvent?.exception?.values?.[0]?.value === 'Middleware Error'; }); diff --git a/packages/e2e-tests/test-applications/sveltekit/event-proxy-server.ts b/packages/e2e-tests/test-applications/sveltekit/event-proxy-server.ts index c61e20d4081d..3037181c36cf 100644 --- a/packages/e2e-tests/test-applications/sveltekit/event-proxy-server.ts +++ b/packages/e2e-tests/test-applications/sveltekit/event-proxy-server.ts @@ -7,6 +7,7 @@ import type { AddressInfo } from 'net'; import * as os from 'os'; import * as path from 'path'; import * as util from 'util'; +import * as zlib from 'zlib'; const readFile = util.promisify(fs.readFile); const writeFile = util.promisify(fs.writeFile); @@ -44,8 +45,12 @@ export async function startEventProxyServer(options: EventProxyServerOptions): P }); proxyRequest.addListener('end', () => { - const proxyRequestBody = Buffer.concat(proxyRequestChunks).toString(); - const envelopeHeader: { dsn?: string } = JSON.parse(proxyRequestBody.split('\n')[0]); + const proxyRequestBody = + proxyRequest.headers['content-encoding'] === 'gzip' + ? zlib.gunzipSync(Buffer.concat(proxyRequestChunks)).toString() + : Buffer.concat(proxyRequestChunks).toString(); + + let envelopeHeader = JSON.parse(proxyRequestBody.split('\n')[0]); if (!envelopeHeader.dsn) { throw new Error('[event-proxy-server] No dsn on envelope header. Please set tunnel option.'); @@ -71,12 +76,11 @@ export async function startEventProxyServer(options: EventProxyServerOptions): P sentryResponse.addListener('end', () => { eventCallbackListeners.forEach(listener => { - const rawProxyRequestBody = Buffer.concat(proxyRequestChunks).toString(); const rawSentryResponseBody = Buffer.concat(sentryResponseChunks).toString(); const data: SentryRequestCallbackData = { - envelope: parseEnvelope(rawProxyRequestBody, new TextEncoder(), new TextDecoder()), - rawProxyRequestBody, + envelope: parseEnvelope(proxyRequestBody, new TextEncoder(), new TextDecoder()), + rawProxyRequestBody: proxyRequestBody, rawSentryResponseBody, sentryResponseStatusCode: sentryResponse.statusCode, }; From 67822fb63d25d5e965599592b2003097aa890e70 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 1 Sep 2023 10:49:57 +0200 Subject: [PATCH 15/29] feat(node-experimental): Implement new performance APIs (#8911) This re-implements the new performance APIs: * `startActiveSpan` * `startSpan` * `getActiveSpan` In `@sentry/node-experimental`, where these delegate to OTEL under the hood. I tried to make sure to have _exactly_ the same APIs as we have for these functions without this. There are some caveats with this: * calling `span.finish()` may behave in unexpected ways - it is not supported to manually finish a span. * Calling `scope.getSpan()` will not work * Calling `span.startSpan()` will not work Basically, only the top level exported methods work, nothing else. --- packages/node-experimental/src/index.ts | 1 + packages/node-experimental/src/sdk/trace.ts | 119 ++++++++++++++++++ .../opentelemetry-node/src/spanprocessor.ts | 2 +- 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 packages/node-experimental/src/sdk/trace.ts diff --git a/packages/node-experimental/src/index.ts b/packages/node-experimental/src/index.ts index 3c246bdf2b97..c55a01641da7 100644 --- a/packages/node-experimental/src/index.ts +++ b/packages/node-experimental/src/index.ts @@ -11,6 +11,7 @@ export { init } from './sdk/init'; export { INTEGRATIONS as Integrations }; export { getAutoPerformanceIntegrations } from './integrations/getAutoPerformanceIntegrations'; export * as Handlers from './sdk/handlers'; +export * from './sdk/trace'; export { makeNodeTransport, diff --git a/packages/node-experimental/src/sdk/trace.ts b/packages/node-experimental/src/sdk/trace.ts new file mode 100644 index 000000000000..8585c0fa8597 --- /dev/null +++ b/packages/node-experimental/src/sdk/trace.ts @@ -0,0 +1,119 @@ +import type { Span as OtelSpan, Tracer } from '@opentelemetry/api'; +import { trace } from '@opentelemetry/api'; +import { getCurrentHub, hasTracingEnabled, Transaction } from '@sentry/core'; +import { _INTERNAL_getSentrySpan } from '@sentry/opentelemetry-node'; +import type { Span, TransactionContext } from '@sentry/types'; +import { isThenable } from '@sentry/utils'; + +import type { NodeExperimentalClient } from './client'; + +/** + * Wraps a function with a transaction/span and finishes the span after the function is done. + * The created span is the active span and will be used as parent by other spans created inside the function + * and can be accessed via `Sentry.getSpan()`, as long as the function is executed while the scope is active. + * + * If you want to create a span that is not set as active, use {@link startSpan}. + * + * Note that if you have not enabled tracing extensions via `addTracingExtensions` + * or you didn't set `tracesSampleRate`, this function will not generate spans + * and the `span` returned from the callback will be undefined. + */ +export function startActiveSpan(context: TransactionContext, callback: (span: Span | undefined) => T): T { + const tracer = getTracer(); + if (!tracer) { + return callback(undefined); + } + + const name = context.description || context.op || ''; + + return tracer.startActiveSpan(name, (span: OtelSpan): T => { + const otelSpanId = span.spanContext().spanId; + + const sentrySpan = _INTERNAL_getSentrySpan(otelSpanId); + + if (sentrySpan && isTransaction(sentrySpan) && context.metadata) { + sentrySpan.setMetadata(context.metadata); + } + + function finishSpan(): void { + span.end(); + } + + let maybePromiseResult: T; + try { + maybePromiseResult = callback(sentrySpan); + } catch (e) { + sentrySpan && sentrySpan.setStatus('internal_error'); + finishSpan(); + throw e; + } + + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then( + () => { + finishSpan(); + }, + () => { + sentrySpan && sentrySpan.setStatus('internal_error'); + finishSpan(); + }, + ); + } else { + finishSpan(); + } + + return maybePromiseResult; + }); +} + +/** + * Creates a span. This span is not set as active, so will not get automatic instrumentation spans + * as children or be able to be accessed via `Sentry.getSpan()`. + * + * If you want to create a span that is set as active, use {@link startActiveSpan}. + * + * Note that if you have not enabled tracing extensions via `addTracingExtensions` + * or you didn't set `tracesSampleRate` or `tracesSampler`, this function will not generate spans + * and the `span` returned from the callback will be undefined. + */ +export function startSpan(context: TransactionContext): Span | undefined { + const tracer = getTracer(); + if (!tracer) { + return undefined; + } + + const name = context.description || context.op || ''; + const otelSpan = tracer.startSpan(name); + + const otelSpanId = otelSpan.spanContext().spanId; + + const sentrySpan = _INTERNAL_getSentrySpan(otelSpanId); + + if (sentrySpan && isTransaction(sentrySpan) && context.metadata) { + sentrySpan.setMetadata(context.metadata); + } + + return sentrySpan; +} + +/** + * Returns the currently active span. + */ +export function getActiveSpan(): Span | undefined { + const otelSpan = trace.getActiveSpan(); + const spanId = otelSpan && otelSpan.spanContext().spanId; + return spanId ? _INTERNAL_getSentrySpan(spanId) : undefined; +} + +function getTracer(): Tracer | undefined { + if (!hasTracingEnabled()) { + return undefined; + } + + const client = getCurrentHub().getClient(); + return client && client.tracer; +} + +function isTransaction(span: Span): span is Transaction { + return span instanceof Transaction; +} diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index 40757160663f..7c5873069dbe 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -92,7 +92,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor { */ public onEnd(otelSpan: OtelSpan): void { const otelSpanId = otelSpan.spanContext().spanId; - const sentrySpan = SENTRY_SPAN_PROCESSOR_MAP.get(otelSpanId); + const sentrySpan = getSentrySpan(otelSpanId); if (!sentrySpan) { __DEBUG_BUILD__ && From 2d7a0d3e9539dcf73a007773d15112cad1c14788 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 1 Sep 2023 10:50:21 +0200 Subject: [PATCH 16/29] feat(node-experimental): Sync OTEL context with Sentry AsyncContext (#8797) This PR implements a strategy to sync the OpenTelemetry Context with our own Hub forking for AsyncContext. This works by fully relying on OpenTelemetry to handle async context isolation/forking. 1. We implement a custom OTEL ContextManager that wraps the default AsyncHooks manager, but makes sure to also fork the hub for each context change & puts the hub on the OTEL context so we can retrieve it. 2. Then, we also have a custom Sentry AsyncContextStrategy which just refers to OTEL context and picks the hub from there we put there in 1. This means we do not need to do any context forking ourselves anymore, so no need for e.g. `Sentry.Handlers.requestHandler()` and stuff like this. It _should_ also mean that Sentry & OTEL should be as in sync as possible. Some notes: * Currently only works for AsyncHooks, which should be fine I guess. Could also be exteded to work with other implementations as well. --- packages/node-experimental/package.json | 1 + packages/node-experimental/src/sdk/init.ts | 2 + .../node-experimental/src/sdk/initOtel.ts | 6 +++ .../src/sdk/otelAsyncContextStrategy.ts | 38 +++++++++++++++++++ .../src/sdk/otelContextManager.ts | 38 +++++++++++++++++++ yarn.lock | 5 +++ 6 files changed, 90 insertions(+) create mode 100644 packages/node-experimental/src/sdk/otelAsyncContextStrategy.ts create mode 100644 packages/node-experimental/src/sdk/otelContextManager.ts diff --git a/packages/node-experimental/package.json b/packages/node-experimental/package.json index 24c42b08d607..a20d2d72d69f 100644 --- a/packages/node-experimental/package.json +++ b/packages/node-experimental/package.json @@ -37,6 +37,7 @@ "@opentelemetry/instrumentation-pg": "~0.36.0", "@opentelemetry/sdk-trace-node": "~1.15.0", "@opentelemetry/semantic-conventions": "~1.15.0", + "@opentelemetry/context-async-hooks": "~1.15.0", "@prisma/instrumentation": "~5.0.0", "@sentry/core": "7.66.0", "@sentry/node": "7.66.0", diff --git a/packages/node-experimental/src/sdk/init.ts b/packages/node-experimental/src/sdk/init.ts index ef451bb83c3a..070728367925 100644 --- a/packages/node-experimental/src/sdk/init.ts +++ b/packages/node-experimental/src/sdk/init.ts @@ -6,6 +6,7 @@ import { Http } from '../integrations/http'; import type { NodeExperimentalOptions } from '../types'; import { NodeExperimentalClient } from './client'; import { initOtel } from './initOtel'; +import { setOtelContextAsyncContextStrategy } from './otelAsyncContextStrategy'; const ignoredDefaultIntegrations = ['Http', 'Undici']; @@ -35,4 +36,5 @@ export function init(options: NodeExperimentalOptions | undefined = {}): void { // Always init Otel, even if tracing is disabled, because we need it for trace propagation & the HTTP integration initOtel(); + setOtelContextAsyncContextStrategy(); } diff --git a/packages/node-experimental/src/sdk/initOtel.ts b/packages/node-experimental/src/sdk/initOtel.ts index cd2b27b79c05..92cf794c8b29 100644 --- a/packages/node-experimental/src/sdk/initOtel.ts +++ b/packages/node-experimental/src/sdk/initOtel.ts @@ -4,6 +4,7 @@ import { getCurrentHub } from '@sentry/core'; import { SentryPropagator, SentrySpanProcessor } from '@sentry/opentelemetry-node'; import type { NodeExperimentalClient } from './client'; +import { SentryContextManager } from './otelContextManager'; /** * Initialize OpenTelemetry for Node. @@ -22,9 +23,14 @@ export function initOtel(): () => void { }); provider.addSpanProcessor(new SentrySpanProcessor()); + // We use a custom context manager to keep context in sync with sentry scope + const contextManager = new SentryContextManager(); + contextManager.enable(); + // Initialize the provider provider.register({ propagator: new SentryPropagator(), + contextManager, }); // Cleanup function diff --git a/packages/node-experimental/src/sdk/otelAsyncContextStrategy.ts b/packages/node-experimental/src/sdk/otelAsyncContextStrategy.ts new file mode 100644 index 000000000000..455dc4717422 --- /dev/null +++ b/packages/node-experimental/src/sdk/otelAsyncContextStrategy.ts @@ -0,0 +1,38 @@ +import * as api from '@opentelemetry/api'; +import type { Hub, RunWithAsyncContextOptions } from '@sentry/core'; +import { setAsyncContextStrategy } from '@sentry/core'; + +import { OTEL_CONTEXT_HUB_KEY } from './otelContextManager'; + +/** + * Sets the async context strategy to use follow the OTEL context under the hood. + * We handle forking a hub inside of our custom OTEL Context Manager (./otelContextManager.ts) + */ +export function setOtelContextAsyncContextStrategy(): void { + function getCurrentHub(): Hub | undefined { + const ctx = api.context.active(); + + // Returning undefined means the global hub will be used + return ctx.getValue(OTEL_CONTEXT_HUB_KEY) as Hub | undefined; + } + + /* This is more or less a NOOP - we rely on the OTEL context manager for this */ + function runWithAsyncContext(callback: () => T, options: RunWithAsyncContextOptions): T { + const existingHub = getCurrentHub(); + + if (existingHub && options?.reuseExisting) { + // We're already in an async context, so we don't need to create a new one + // just call the callback with the current hub + return callback(); + } + + const ctx = api.context.active(); + + // We depend on the otelContextManager to handle the context/hub + return api.context.with(ctx, () => { + return callback(); + }); + } + + setAsyncContextStrategy({ getCurrentHub, runWithAsyncContext }); +} diff --git a/packages/node-experimental/src/sdk/otelContextManager.ts b/packages/node-experimental/src/sdk/otelContextManager.ts new file mode 100644 index 000000000000..9110b9e62328 --- /dev/null +++ b/packages/node-experimental/src/sdk/otelContextManager.ts @@ -0,0 +1,38 @@ +import type { Context } from '@opentelemetry/api'; +import * as api from '@opentelemetry/api'; +import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks'; +import type { Carrier, Hub } from '@sentry/core'; +import { ensureHubOnCarrier, getCurrentHub, getHubFromCarrier } from '@sentry/core'; + +export const OTEL_CONTEXT_HUB_KEY = api.createContextKey('sentry_hub'); + +function createNewHub(parent: Hub | undefined): Hub { + const carrier: Carrier = {}; + ensureHubOnCarrier(carrier, parent); + return getHubFromCarrier(carrier); +} + +/** + * This is a custom ContextManager for OpenTelemetry, which extends the default AsyncLocalStorageContextManager. + * It ensures that we create a new hub per context, so that the OTEL Context & the Sentry Hub are always in sync. + * + * Note that we currently only support AsyncHooks with this, + * but since this should work for Node 14+ anyhow that should be good enough. + */ +export class SentryContextManager extends AsyncLocalStorageContextManager { + /** + * Overwrite with() of the original AsyncLocalStorageContextManager + * to ensure we also create a new hub per context. + */ + public with ReturnType>( + context: Context, + fn: F, + thisArg?: ThisParameterType, + ...args: A + ): ReturnType { + const existingHub = getCurrentHub(); + const newHub = createNewHub(existingHub); + + return super.with(context.setValue(OTEL_CONTEXT_HUB_KEY, newHub), fn, thisArg, ...args); + } +} diff --git a/yarn.lock b/yarn.lock index 40f7682f22b7..ec2d72d684e8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3733,6 +3733,11 @@ dependencies: tslib "^2.3.1" +"@opentelemetry/context-async-hooks@~1.15.0": + version "1.15.2" + resolved "https://registry.yarnpkg.com/@opentelemetry/context-async-hooks/-/context-async-hooks-1.15.2.tgz#116bd5fef231137198d5bf551e8c0521fbdfe928" + integrity sha512-VAMHG67srGFQDG/N2ns5AyUT9vUcoKpZ/NpJ5fDQIPfJd7t3ju+aHwvDsMcrYBWuCh03U3Ky6o16+872CZchBg== + "@opentelemetry/context-base@^0.12.0": version "0.12.0" resolved "https://registry.yarnpkg.com/@opentelemetry/context-base/-/context-base-0.12.0.tgz#4906ae27359d3311e3dea1b63770a16f60848550" From 61450cb02dde34b59dedeb3cbd44a690c516e9d5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 1 Sep 2023 10:59:55 +0200 Subject: [PATCH 17/29] chore(vue): Remove unused `index.bundle.ts` file (#8929) Cleaning up a leftover from v6 when we still built Vue CDN bundles. --- packages/vue/src/index.bundle.ts | 68 -------------------------------- 1 file changed, 68 deletions(-) delete mode 100644 packages/vue/src/index.bundle.ts diff --git a/packages/vue/src/index.bundle.ts b/packages/vue/src/index.bundle.ts deleted file mode 100644 index 398208cf2282..000000000000 --- a/packages/vue/src/index.bundle.ts +++ /dev/null @@ -1,68 +0,0 @@ -export type { - Breadcrumb, - Request, - SdkInfo, - Event, - Exception, - SeverityLevel, - StackFrame, - Stacktrace, - Thread, - User, -} from '@sentry/types'; - -export type { BrowserOptions, ReportDialogOptions } from '@sentry/browser'; - -export { - BrowserClient, - defaultIntegrations, - forceLoad, - lastEventId, - onLoad, - showReportDialog, - flush, - close, - wrap, - addGlobalEventProcessor, - addBreadcrumb, - captureException, - captureEvent, - captureMessage, - configureScope, - getHubFromCarrier, - getCurrentHub, - Hub, - Scope, - setContext, - setExtra, - setExtras, - setTag, - setTags, - setUser, - startTransaction, - makeFetchTransport, - makeXHRTransport, - withScope, - SDK_VERSION, -} from '@sentry/browser'; - -import { Integrations as BrowserIntegrations, WINDOW } from '@sentry/browser'; - -export { init } from './sdk'; -export { vueRouterInstrumentation } from './router'; -export { attachErrorHandler } from './errorhandler'; -export { createTracingMixins } from './tracing'; - -let windowIntegrations = {}; - -// This block is needed to add compatibility with the integrations packages when used with a CDN -if (WINDOW.Sentry && WINDOW.Sentry.Integrations) { - windowIntegrations = WINDOW.Sentry.Integrations; -} - -const INTEGRATIONS = { - ...windowIntegrations, - ...BrowserIntegrations, -}; - -export { INTEGRATIONS as Integrations }; From 726a6ad7a0d0a8712cfc0d25d9bc145d6b2bc38f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 10:55:08 +0200 Subject: [PATCH 18/29] feat(browser): Mark errors caught from `TryCatch` integration as unhandled (#8890) Change the mechanisms of errors caught from the `TryCatch` integration to report the error as unhandled. Detailed write-up in https://github.com/getsentry/sentry-javascript/pull/8890 --- .../public-api/instrumentation/eventListener/test.ts | 2 +- .../suites/sessions/update-session/test.ts | 2 +- packages/browser/src/integrations/trycatch.ts | 10 +++++----- packages/browser/test/integration/suites/builtins.js | 8 ++++---- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/test.ts b/packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/test.ts index 8d821a14906e..d7b9f75a13f2 100644 --- a/packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/test.ts +++ b/packages/browser-integration-tests/suites/public-api/instrumentation/eventListener/test.ts @@ -17,7 +17,7 @@ sentryTest( value: 'event_listener_error', mechanism: { type: 'instrument', - handled: true, + handled: false, }, stacktrace: { frames: expect.any(Array), diff --git a/packages/browser-integration-tests/suites/sessions/update-session/test.ts b/packages/browser-integration-tests/suites/sessions/update-session/test.ts index 5ce88e4bdc0e..dfde68dce175 100644 --- a/packages/browser-integration-tests/suites/sessions/update-session/test.ts +++ b/packages/browser-integration-tests/suites/sessions/update-session/test.ts @@ -17,7 +17,7 @@ sentryTest('should update session when an error is thrown.', async ({ getLocalTe expect(updatedSession).toBeDefined(); expect(updatedSession.init).toBe(false); expect(updatedSession.errors).toBe(1); - expect(updatedSession.status).toBe('ok'); + expect(updatedSession.status).toBe('crashed'); expect(pageloadSession.sid).toBe(updatedSession.sid); }); diff --git a/packages/browser/src/integrations/trycatch.ts b/packages/browser/src/integrations/trycatch.ts index cb3e42cfd107..0ec05cc552df 100644 --- a/packages/browser/src/integrations/trycatch.ts +++ b/packages/browser/src/integrations/trycatch.ts @@ -113,7 +113,7 @@ function _wrapTimeFunction(original: () => void): () => number { args[0] = wrap(originalCallback, { mechanism: { data: { function: getFunctionName(original) }, - handled: true, + handled: false, type: 'instrument', }, }); @@ -134,7 +134,7 @@ function _wrapRAF(original: any): (callback: () => void) => any { function: 'requestAnimationFrame', handler: getFunctionName(original), }, - handled: true, + handled: false, type: 'instrument', }, }), @@ -160,7 +160,7 @@ function _wrapXHR(originalSend: () => void): () => void { function: prop, handler: getFunctionName(original), }, - handled: true, + handled: false, type: 'instrument', }, }; @@ -220,7 +220,7 @@ function _wrapEventTarget(target: string): void { handler: getFunctionName(fn), target, }, - handled: true, + handled: false, type: 'instrument', }, }); @@ -239,7 +239,7 @@ function _wrapEventTarget(target: string): void { handler: getFunctionName(fn), target, }, - handled: true, + handled: false, type: 'instrument', }, }), diff --git a/packages/browser/test/integration/suites/builtins.js b/packages/browser/test/integration/suites/builtins.js index 8fa30d120e90..7a7760d9f689 100644 --- a/packages/browser/test/integration/suites/builtins.js +++ b/packages/browser/test/integration/suites/builtins.js @@ -182,7 +182,7 @@ describe('wrapped built-ins', function () { assert.deepEqual(summary.events[0].exception.values[0].mechanism, { type: 'instrument', - handled: true, + handled: false, data: { function: 'onreadystatechange', }, @@ -237,7 +237,7 @@ describe('wrapped built-ins', function () { assert.deepEqual(summary.events[0].exception.values[0].mechanism, { type: 'instrument', - handled: true, + handled: false, }); } }); @@ -277,7 +277,7 @@ describe('wrapped built-ins', function () { assert.oneOf(target, ['Node', 'EventTarget']); assert.deepEqual(summary.events[0].exception.values[0].mechanism, { type: 'instrument', - handled: true, + handled: false, data: { function: 'addEventListener', }, @@ -313,7 +313,7 @@ describe('wrapped built-ins', function () { assert.oneOf(target, ['Node', 'EventTarget']); assert.deepEqual(summary.events[0].exception.values[0].mechanism, { type: 'instrument', - handled: true, + handled: false, data: { function: 'addEventListener', handler: '', From fe375b9b628f8feb8cd1ecbfa614593f8032abe7 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 10:56:47 +0200 Subject: [PATCH 19/29] feat(vue): Mark errors caught by Vue wrappers as unhandled (#8905) Report exceptions caught by our Vue error handler and routing instrumentation as unhandled. Detailed write up in https://github.com/getsentry/sentry-javascript/pull/8890 --- packages/vue/src/errorhandler.ts | 9 +++++++++ packages/vue/src/router.ts | 12 +++++++++++- packages/vue/test/router.test.ts | 3 ++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/vue/src/errorhandler.ts b/packages/vue/src/errorhandler.ts index fd4a1d564343..542d341c322f 100644 --- a/packages/vue/src/errorhandler.ts +++ b/packages/vue/src/errorhandler.ts @@ -1,4 +1,5 @@ import { getCurrentHub } from '@sentry/browser'; +import { addExceptionMechanism } from '@sentry/utils'; import type { Options, ViewModel, Vue } from './types'; import { formatComponentName, generateComponentTrace } from './vendor/components'; @@ -31,6 +32,14 @@ export const attachErrorHandler = (app: Vue, options: Options): void => { setTimeout(() => { getCurrentHub().withScope(scope => { scope.setContext('vue', metadata); + + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + handled: false, + }); + return event; + }); + getCurrentHub().captureException(error); }); }); diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index 641a4344fd4c..2e3fb3476eb1 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -1,5 +1,6 @@ import { captureException, WINDOW } from '@sentry/browser'; import type { Transaction, TransactionContext, TransactionSource } from '@sentry/types'; +import { addExceptionMechanism } from '@sentry/utils'; import { getActiveTransaction } from './tracing'; @@ -78,7 +79,16 @@ export function vueRouterInstrumentation( }); } - router.onError(error => captureException(error)); + router.onError(error => + captureException(error, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { handled: false }); + return event; + }); + + return scope; + }), + ); router.beforeEach((to, from, next) => { // According to docs we could use `from === VueRouter.START_LOCATION` but I couldnt get it working for Vue 2 diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts index a1d6dd4a6713..6e74c7c51251 100644 --- a/packages/vue/test/router.test.ts +++ b/packages/vue/test/router.test.ts @@ -72,7 +72,8 @@ describe('vueRouterInstrumentation()', () => { onErrorCallback(testError); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - expect(captureExceptionSpy).toHaveBeenCalledWith(testError); + // second function is the scope callback + expect(captureExceptionSpy).toHaveBeenCalledWith(testError, expect.any(Function)); }); it.each([ From eb2998196e63c6b9ba9ebc01d9542a1a06b143f0 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 13:55:32 +0200 Subject: [PATCH 20/29] ref(browser): Deprecate top-level `wrap` function (#8927) Deprecate the `wrap` function exported from `@sentry/browser`. This function is documented nowhere, not part of the unified API and afaict not used by any SDKs depending on `@sentry/browser`. --- packages/browser/src/exports.ts | 11 ++++++++++- packages/browser/src/sdk.ts | 5 +++++ packages/browser/test/unit/index.test.ts | 4 ++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/browser/src/exports.ts b/packages/browser/src/exports.ts index 03533bdbc90d..e9050399e641 100644 --- a/packages/browser/src/exports.ts +++ b/packages/browser/src/exports.ts @@ -63,5 +63,14 @@ export { } from './stack-parsers'; export { eventFromException, eventFromMessage, exceptionFromError } from './eventbuilder'; export { createUserFeedbackEnvelope } from './userfeedback'; -export { defaultIntegrations, forceLoad, init, onLoad, showReportDialog, wrap, captureUserFeedback } from './sdk'; +export { + defaultIntegrations, + forceLoad, + init, + onLoad, + showReportDialog, + captureUserFeedback, + // eslint-disable-next-line deprecation/deprecation + wrap, +} from './sdk'; export { GlobalHandlers, TryCatch, Breadcrumbs, LinkedErrors, HttpContext, Dedupe } from './integrations'; diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 45fd26c75745..de352d9db154 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -192,10 +192,15 @@ export function onLoad(callback: () => void): void { /** * Wrap code within a try/catch block so the SDK is able to capture errors. * + * @deprecated This function will be removed in v8. + * It is not part of Sentry's official API and it's easily replaceable by using a try/catch block + * and calling Sentry.captureException. + * * @param fn A function to wrap. * * @returns The result of wrapped function call. */ +// TODO(v8): Remove this function // eslint-disable-next-line @typescript-eslint/no-explicit-any export function wrap(fn: (...args: any) => any): any { return internalWrap(fn)(); diff --git a/packages/browser/test/unit/index.test.ts b/packages/browser/test/unit/index.test.ts index 122637ea91d4..95985a2a69d2 100644 --- a/packages/browser/test/unit/index.test.ts +++ b/packages/browser/test/unit/index.test.ts @@ -355,6 +355,7 @@ describe('wrap()', () => { getCurrentHub().bindClient(new BrowserClient(options)); try { + // eslint-disable-next-line deprecation/deprecation wrap(() => { throw new TypeError('mkey'); }); @@ -364,11 +365,13 @@ describe('wrap()', () => { }); it('should return result of a function call', () => { + // eslint-disable-next-line deprecation/deprecation const result = wrap(() => 2); expect(result).toBe(2); }); it('should allow for passing this and arguments through binding', () => { + // eslint-disable-next-line deprecation/deprecation const result = wrap( function (this: unknown, a: string, b: number): unknown[] { return [this, a, b]; @@ -379,6 +382,7 @@ describe('wrap()', () => { expect((result as unknown[])[1]).toBe('b'); expect((result as unknown[])[2]).toBe(42); + // eslint-disable-next-line deprecation/deprecation const result2 = wrap( function (this: { x: number }): number { return this.x; From 86b3c690e6411c30b61c08e69e13aa77f777f713 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 13:56:46 +0200 Subject: [PATCH 21/29] feat(serverless): Mark errors caught in Serverless handlers as unhandled (#8907) Mark errors caught in * AWS Lambda handler * GCP http, function, cloudEvent handlers as unhandled For more details see #8890 --- packages/serverless/src/awslambda.ts | 6 +-- .../src/gcpfunction/cloud_events.ts | 8 ++-- packages/serverless/src/gcpfunction/events.ts | 8 ++-- packages/serverless/src/gcpfunction/http.ts | 6 +-- packages/serverless/src/utils.ts | 13 ++++++ packages/serverless/test/awslambda.test.ts | 41 ++++++++++++++--- packages/serverless/test/gcpfunction.test.ts | 44 +++++++++++++++---- 7 files changed, 97 insertions(+), 29 deletions(-) diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index b5aa9200b524..e8847fc7b212 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -14,7 +14,7 @@ import { performance } from 'perf_hooks'; import { types } from 'util'; import { AWSServices } from './awsservices'; -import { serverlessEventProcessor } from './utils'; +import { markEventUnhandled, serverlessEventProcessor } from './utils'; export * from '@sentry/node'; @@ -312,11 +312,11 @@ export function wrapHandler( if (options.captureAllSettledReasons && Array.isArray(rv) && isPromiseAllSettledResult(rv)) { const reasons = getRejectedReasons(rv); reasons.forEach(exception => { - captureException(exception); + captureException(exception, scope => markEventUnhandled(scope)); }); } } catch (e) { - captureException(e); + captureException(e, scope => markEventUnhandled(scope)); throw e; } finally { clearTimeout(timeoutWarningTimer); diff --git a/packages/serverless/src/gcpfunction/cloud_events.ts b/packages/serverless/src/gcpfunction/cloud_events.ts index 83725ffbb840..a0d843e71abe 100644 --- a/packages/serverless/src/gcpfunction/cloud_events.ts +++ b/packages/serverless/src/gcpfunction/cloud_events.ts @@ -1,7 +1,7 @@ import { captureException, flush, getCurrentHub } from '@sentry/node'; import { isThenable, logger } from '@sentry/utils'; -import { domainify, proxyFunction } from '../utils'; +import { domainify, markEventUnhandled, proxyFunction } from '../utils'; import type { CloudEventFunction, CloudEventFunctionWithCallback, WrapperOptions } from './general'; export type CloudEventFunctionWrapperOptions = WrapperOptions; @@ -50,7 +50,7 @@ function _wrapCloudEventFunction( const newCallback = domainify((...args: unknown[]) => { if (args[0] !== null && args[0] !== undefined) { - captureException(args[0]); + captureException(args[0], scope => markEventUnhandled(scope)); } transaction?.finish(); @@ -68,13 +68,13 @@ function _wrapCloudEventFunction( try { fnResult = (fn as CloudEventFunctionWithCallback)(context, newCallback); } catch (err) { - captureException(err); + captureException(err, scope => markEventUnhandled(scope)); throw err; } if (isThenable(fnResult)) { fnResult.then(null, err => { - captureException(err); + captureException(err, scope => markEventUnhandled(scope)); throw err; }); } diff --git a/packages/serverless/src/gcpfunction/events.ts b/packages/serverless/src/gcpfunction/events.ts index e2342d1fe905..9c98fcb8c485 100644 --- a/packages/serverless/src/gcpfunction/events.ts +++ b/packages/serverless/src/gcpfunction/events.ts @@ -1,7 +1,7 @@ import { captureException, flush, getCurrentHub } from '@sentry/node'; import { isThenable, logger } from '@sentry/utils'; -import { domainify, proxyFunction } from '../utils'; +import { domainify, markEventUnhandled, proxyFunction } from '../utils'; import type { EventFunction, EventFunctionWithCallback, WrapperOptions } from './general'; export type EventFunctionWrapperOptions = WrapperOptions; @@ -52,7 +52,7 @@ function _wrapEventFunction const newCallback = domainify((...args: unknown[]) => { if (args[0] !== null && args[0] !== undefined) { - captureException(args[0]); + captureException(args[0], scope => markEventUnhandled(scope)); } transaction?.finish(); @@ -72,13 +72,13 @@ function _wrapEventFunction try { fnResult = (fn as EventFunctionWithCallback)(data, context, newCallback); } catch (err) { - captureException(err); + captureException(err, scope => markEventUnhandled(scope)); throw err; } if (isThenable(fnResult)) { fnResult.then(null, err => { - captureException(err); + captureException(err, scope => markEventUnhandled(scope)); throw err; }); } diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 1c265fe9fb64..eea492bb8dab 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -2,7 +2,7 @@ import type { AddRequestDataToEventOptions } from '@sentry/node'; import { captureException, flush, getCurrentHub } from '@sentry/node'; import { isString, isThenable, logger, stripUrlQueryAndFragment, tracingContextFromHeaders } from '@sentry/utils'; -import { domainify, proxyFunction } from './../utils'; +import { domainify, markEventUnhandled, proxyFunction } from './../utils'; import type { HttpFunction, WrapperOptions } from './general'; // TODO (v8 / #5257): Remove this whole old/new business and just use the new stuff @@ -122,13 +122,13 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial markEventUnhandled(scope)); throw err; } if (isThenable(fnResult)) { fnResult.then(null, err => { - captureException(err); + captureException(err, scope => markEventUnhandled(scope)); throw err; }); } diff --git a/packages/serverless/src/utils.ts b/packages/serverless/src/utils.ts index ae1f4b987ffb..69e28ab3a823 100644 --- a/packages/serverless/src/utils.ts +++ b/packages/serverless/src/utils.ts @@ -1,5 +1,6 @@ import { runWithAsyncContext } from '@sentry/core'; import type { Event } from '@sentry/node'; +import type { Scope } from '@sentry/types'; import { addExceptionMechanism } from '@sentry/utils'; /** @@ -55,3 +56,15 @@ export function proxyFunction R>( return new Proxy(source, handler); } + +/** + * Marks an event as unhandled by adding a span processor to the passed scope. + */ +export function markEventUnhandled(scope: Scope): Scope { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { handled: false }); + return event; + }); + + return scope; +} diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts index 53770927c4a5..e03d17bfd14b 100644 --- a/packages/serverless/test/awslambda.test.ts +++ b/packages/serverless/test/awslambda.test.ts @@ -1,6 +1,7 @@ // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil // eslint-disable-next-line import/no-unresolved import * as SentryNode from '@sentry/node'; +import type { Event } from '@sentry/types'; // eslint-disable-next-line import/no-unresolved import type { Callback, Handler } from 'aws-lambda'; @@ -175,8 +176,8 @@ describe('AWSLambda', () => { ]); const wrappedHandler = wrapHandler(handler, { flushTimeout: 1337, captureAllSettledReasons: true }); await wrappedHandler(fakeEvent, fakeContext, fakeCallback); - expect(SentryNode.captureException).toHaveBeenNthCalledWith(1, error); - expect(SentryNode.captureException).toHaveBeenNthCalledWith(2, error2); + expect(SentryNode.captureException).toHaveBeenNthCalledWith(1, error, expect.any(Function)); + expect(SentryNode.captureException).toHaveBeenNthCalledWith(2, error2, expect.any(Function)); expect(SentryNode.captureException).toBeCalledTimes(2); }); }); @@ -229,7 +230,7 @@ describe('AWSLambda', () => { // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); expectScopeSettings(fakeTransactionContext); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalledWith(2000); @@ -308,7 +309,7 @@ describe('AWSLambda', () => { // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); expectScopeSettings(fakeTransactionContext); - expect(SentryNode.captureException).toBeCalledWith(e); + expect(SentryNode.captureException).toBeCalledWith(e, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -375,7 +376,7 @@ describe('AWSLambda', () => { // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); expectScopeSettings(fakeTransactionContext); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -457,7 +458,7 @@ describe('AWSLambda', () => { // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); expectScopeSettings(fakeTransactionContext); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -465,6 +466,34 @@ describe('AWSLambda', () => { }); }); + test('marks the captured error as unhandled', async () => { + expect.assertions(3); + + const error = new Error('wat'); + const handler: Handler = async (_event, _context, _callback) => { + throw error; + }; + const wrappedHandler = wrapHandler(handler); + + try { + await wrappedHandler(fakeEvent, fakeContext, fakeCallback); + } catch (e) { + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); + // @ts-ignore see "Why @ts-ignore" note + const scopeFunction = SentryNode.captureException.mock.calls[0][1]; + const event: Event = { exception: { values: [{}] } }; + let evtProcessor: ((e: Event) => Event) | undefined = undefined; + scopeFunction({ addEventProcessor: jest.fn().mockImplementation(proc => (evtProcessor = proc)) }); + + expect(evtProcessor).toBeInstanceOf(Function); + // @ts-ignore just mocking around... + expect(evtProcessor(event).exception.values[0].mechanism).toEqual({ + handled: false, + type: 'generic', + }); + } + }); + describe('init()', () => { test('calls Sentry.init with correct sdk info metadata', () => { Sentry.AWSLambda.init({}); diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index 74939f1f574a..812447106ad5 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -1,4 +1,5 @@ import * as SentryNode from '@sentry/node'; +import type { Event } from '@sentry/types'; import * as domain from 'domain'; import * as Sentry from '../src'; @@ -12,7 +13,6 @@ import type { Request, Response, } from '../src/gcpfunction/general'; - /** * Why @ts-ignore some Sentry.X calls * @@ -198,7 +198,7 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -317,7 +317,7 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -382,7 +382,7 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -440,7 +440,7 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -469,7 +469,33 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); + }); + }); + + test('marks the captured error as unhandled', async () => { + expect.assertions(4); + + const error = new Error('wat'); + const handler: EventFunctionWithCallback = (_data, _context, _cb) => { + throw error; + }; + const wrappedHandler = wrapEventFunction(handler); + await expect(handleEvent(wrappedHandler)).rejects.toThrowError(error); + + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); + + // @ts-ignore just mocking around... + const scopeFunction = SentryNode.captureException.mock.calls[0][1]; + const event: Event = { exception: { values: [{}] } }; + let evtProcessor: ((e: Event) => Event) | undefined = undefined; + scopeFunction({ addEventProcessor: jest.fn().mockImplementation(proc => (evtProcessor = proc)) }); + + expect(evtProcessor).toBeInstanceOf(Function); + // @ts-ignore just mocking around... + expect(evtProcessor(event).exception.values[0].mechanism).toEqual({ + handled: false, + type: 'generic', }); }); @@ -537,7 +563,7 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -595,7 +621,7 @@ describe('GCPFunction', () => { expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeTransaction.finish).toBeCalled(); expect(SentryNode.flush).toBeCalled(); @@ -625,7 +651,7 @@ describe('GCPFunction', () => { // @ts-ignore see "Why @ts-ignore" note expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction); - expect(SentryNode.captureException).toBeCalledWith(error); + expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function)); }); }); From 3d8b44441a32d5fd141454974465e17d30a5492c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 13:57:56 +0200 Subject: [PATCH 22/29] feat(react): Mark errors captured from ErrorBoundary as unhandled (#8914) For more details see #8890 --- packages/react/src/errorboundary.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/react/src/errorboundary.tsx b/packages/react/src/errorboundary.tsx index d3c831e24357..3d894fc699a8 100644 --- a/packages/react/src/errorboundary.tsx +++ b/packages/react/src/errorboundary.tsx @@ -1,6 +1,6 @@ import type { ReportDialogOptions, Scope } from '@sentry/browser'; import { captureException, getCurrentHub, showReportDialog, withScope } from '@sentry/browser'; -import { isError, logger } from '@sentry/utils'; +import { addExceptionMechanism, isError, logger } from '@sentry/utils'; import hoistNonReactStatics from 'hoist-non-react-statics'; import * as React from 'react'; @@ -138,7 +138,14 @@ class ErrorBoundary extends React.Component { + addExceptionMechanism(event, { handled: false }) + return event; + }) + const eventId = captureException(error, { contexts: { react: { componentStack } } }); + if (onError) { onError(error, componentStack, eventId); } From b1028174c23c7e33f083ecff82bb3bc0aaff4230 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 16:38:27 +0200 Subject: [PATCH 23/29] feat(nextjs): Mark errors caught from NextJS wrappers as unhandled (#8893) Mark errors caught in * `captureUnderscoreException` * `wrapApiHandlerWithSentry` * `callDataFetcherTraced` * `withErrorInstrumentation` * `wrapServerComponentWithSentry` as unhandled. For more details, see #8890, #6073 Co-authored-by: Luca Forstner --- packages/nextjs/src/common/_error.ts | 2 +- .../nextjs/src/common/utils/wrapperUtils.ts | 26 ++++++++++++++++--- .../src/common/wrapApiHandlerWithSentry.ts | 2 +- .../common/wrapServerComponentWithSentry.ts | 14 ++++++++-- 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/packages/nextjs/src/common/_error.ts b/packages/nextjs/src/common/_error.ts index 624aabdb3b98..1ad27cc0b67f 100644 --- a/packages/nextjs/src/common/_error.ts +++ b/packages/nextjs/src/common/_error.ts @@ -45,7 +45,7 @@ export async function captureUnderscoreErrorException(contextOrProps: ContextOrP scope.addEventProcessor(event => { addExceptionMechanism(event, { type: 'instrument', - handled: true, + handled: false, data: { function: '_error.getInitialProps', }, diff --git a/packages/nextjs/src/common/utils/wrapperUtils.ts b/packages/nextjs/src/common/utils/wrapperUtils.ts index 80c9a57b65b5..a6d51ceacebb 100644 --- a/packages/nextjs/src/common/utils/wrapperUtils.ts +++ b/packages/nextjs/src/common/utils/wrapperUtils.ts @@ -6,7 +6,7 @@ import { startTransaction, } from '@sentry/core'; import type { Span, Transaction } from '@sentry/types'; -import { isString, tracingContextFromHeaders } from '@sentry/utils'; +import { addExceptionMechanism, isString, tracingContextFromHeaders } from '@sentry/utils'; import type { IncomingMessage, ServerResponse } from 'http'; import { platformSupportsStreaming } from './platformSupportsStreaming'; @@ -47,7 +47,17 @@ export function withErrorInstrumentation any>( return await origFunction.apply(this, origFunctionArguments); } catch (e) { // TODO: Extract error logic from `withSentry` in here or create a new wrapper with said logic or something like that. - captureException(e); + captureException(e, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + handled: false, + }); + return event; + }); + + return scope; + }); + throw e; } }; @@ -221,7 +231,17 @@ export async function callDataFetcherTraced Promis span.finish(); // TODO Copy more robust error handling over from `withSentry` - captureException(err); + captureException(err, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + handled: false, + }); + return event; + }); + + return scope; + }); + throw err; } } diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index 62ecd952b460..74815e5a209f 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -191,7 +191,7 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri currentScope.addEventProcessor(event => { addExceptionMechanism(event, { type: 'instrument', - handled: true, + handled: false, data: { wrapped_handler: wrappingTarget.name, function: 'withSentry', diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index c5db1c7f217f..31220dcf2c73 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -5,7 +5,7 @@ import { runWithAsyncContext, startTransaction, } from '@sentry/core'; -import { tracingContextFromHeaders } from '@sentry/utils'; +import { addExceptionMechanism, tracingContextFromHeaders } from '@sentry/utils'; import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import type { ServerComponentContext } from '../common/types'; @@ -63,7 +63,17 @@ export function wrapServerComponentWithSentry any> // We don't want to report redirects } else { transaction.setStatus('internal_error'); - captureException(e); + + captureException(e, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + handled: false, + }); + return event; + }); + + return scope; + }); } transaction.finish(); From 39401f010ab9575b51180c08a21ecaa8421e770c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 4 Sep 2023 17:07:35 +0200 Subject: [PATCH 24/29] feat(remix): Mark errors caught from Remix instrumentation as unhandled (#8894) Mark errors caught from remix instrumentation as unhandled: * `captureRemixErrorBoundaryError` * `captureRemixServerException` For more details see #8890, #6073 --- packages/remix/src/client/errors.tsx | 2 +- packages/remix/src/utils/instrumentServer.ts | 2 +- .../integration/test/client/errorboundary.test.ts | 7 +++++-- .../test/integration/test/server/action.test.ts | 12 ++++++------ .../test/integration/test/server/loader.test.ts | 4 ++-- 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/remix/src/client/errors.tsx b/packages/remix/src/client/errors.tsx index 9c9fd5c4b449..ec2bf44a79e1 100644 --- a/packages/remix/src/client/errors.tsx +++ b/packages/remix/src/client/errors.tsx @@ -43,7 +43,7 @@ export function captureRemixErrorBoundaryError(error: unknown): void { scope.addEventProcessor(event => { addExceptionMechanism(event, { type: 'instrument', - handled: true, + handled: false, data: eventData, }); return event; diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 6fc326aab104..780b5e9df121 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -104,7 +104,7 @@ export async function captureRemixServerException(err: unknown, name: string, re scope.addEventProcessor(event => { addExceptionMechanism(event, { type: 'instrument', - handled: true, + handled: false, data: { function: name, }, diff --git a/packages/remix/test/integration/test/client/errorboundary.test.ts b/packages/remix/test/integration/test/client/errorboundary.test.ts index 9c496e3e3040..56b18aa72d12 100644 --- a/packages/remix/test/integration/test/client/errorboundary.test.ts +++ b/packages/remix/test/integration/test/client/errorboundary.test.ts @@ -11,7 +11,7 @@ test('should capture React component errors.', async ({ page }) => { const [pageloadEnvelope, errorEnvelope] = envelopes; - expect(pageloadEnvelope.contexts?.trace.op).toBe('pageload'); + expect(pageloadEnvelope.contexts?.trace?.op).toBe('pageload'); expect(pageloadEnvelope.tags?.['routing.instrumentation']).toBe('remix-router'); expect(pageloadEnvelope.type).toBe('transaction'); expect(pageloadEnvelope.transaction).toBe( @@ -27,6 +27,7 @@ test('should capture React component errors.', async ({ page }) => { type: 'React ErrorBoundary Error', value: 'Sentry React Component Error', stacktrace: { frames: expect.any(Array) }, + mechanism: { type: 'chained', handled: false }, }, ] : []), @@ -34,7 +35,9 @@ test('should capture React component errors.', async ({ page }) => { type: 'Error', value: 'Sentry React Component Error', stacktrace: { frames: expect.any(Array) }, - mechanism: { type: useV2 ? 'instrument' : 'generic', handled: true }, + // In v2 this error will be marked unhandled, in v1 its handled because of LinkedErrors + // This should be fine though because the error boundary's error is marked unhandled + mechanism: { type: useV2 ? 'instrument' : 'generic', handled: !useV2 }, }, ]); }); diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index edf94f20e253..af48c99777ce 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -83,7 +83,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada data: { function: useV2 ? 'remix.server' : 'action', }, - handled: true, + handled: false, type: 'instrument', }, }, @@ -203,7 +203,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada data: { function: useV2 ? 'remix.server' : 'loader', }, - handled: true, + handled: false, type: 'instrument', }, }, @@ -256,7 +256,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada data: { function: useV2 ? 'ErrorResponse' : 'action', }, - handled: true, + handled: false, type: 'instrument', }, }, @@ -311,7 +311,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada data: { function: useV2 ? 'ErrorResponse' : 'action', }, - handled: true, + handled: false, type: 'instrument', }, }, @@ -364,7 +364,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada data: { function: useV2 ? 'ErrorResponse' : 'action', }, - handled: true, + handled: false, type: 'instrument', }, }, @@ -419,7 +419,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada data: { function: useV2 ? 'ErrorResponse' : 'action', }, - handled: true, + handled: false, type: 'instrument', }, }, diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index ccaa93b05e36..c3be005f0c1b 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -41,7 +41,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada data: { function: useV2 ? 'remix.server' : 'loader', }, - handled: true, + handled: false, type: 'instrument', }, }, @@ -140,7 +140,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada data: { function: useV2 ? 'remix.server' : 'loader', }, - handled: true, + handled: false, type: 'instrument', }, }, From 29ef20e2f7b170c98d44f4aca30a4a5af1b606e1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 5 Sep 2023 09:05:04 +0200 Subject: [PATCH 25/29] feat(integrations): Mark errors caught from `HttpClient` and `CaptureConsole` integrations as unhandled (#8891) Mark errors caught from optional integrations * `HttpClient` * `CaptureConsole` as unhandled. For more details see #8890, #6073 --- .../integrations/httpclient/axios/test.ts | 2 +- .../httpclient/fetch/simple/test.ts | 2 +- .../httpclient/fetch/withRequest/test.ts | 2 +- .../withRequestAndBodyAndOptions/test.ts | 2 +- .../fetch/withRequestAndOptions/test.ts | 2 +- .../integrations/httpclient/xhr/test.ts | 2 +- packages/integrations/src/captureconsole.ts | 7 ++++ packages/integrations/src/httpclient.ts | 1 + .../integrations/test/captureconsole.test.ts | 32 +++++++++++++++++++ 9 files changed, 46 insertions(+), 6 deletions(-) diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts index ec0b54653be1..3560a2598160 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts @@ -38,7 +38,7 @@ sentryTest( value: 'HTTP Client Error with status code: 500', mechanism: { type: 'http.client', - handled: true, + handled: false, }, }, ], diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts index 07eafb3185ae..6885de912437 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts @@ -38,7 +38,7 @@ sentryTest( value: 'HTTP Client Error with status code: 500', mechanism: { type: 'http.client', - handled: true, + handled: false, }, }, ], diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts index dd829a2bcc22..3caa6bd19652 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts @@ -36,7 +36,7 @@ sentryTest('works with a Request passed in', async ({ getLocalTestPath, page }) value: 'HTTP Client Error with status code: 500', mechanism: { type: 'http.client', - handled: true, + handled: false, }, }, ], diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts index 208db16c84c9..1ae88e8c5f5b 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts @@ -38,7 +38,7 @@ sentryTest( value: 'HTTP Client Error with status code: 500', mechanism: { type: 'http.client', - handled: true, + handled: false, }, }, ], diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts index a288f6fae1fb..9840f91ba272 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts @@ -36,7 +36,7 @@ sentryTest('works with a Request (without body) & options passed in', async ({ g value: 'HTTP Client Error with status code: 500', mechanism: { type: 'http.client', - handled: true, + handled: false, }, }, ], diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts index 06f6bd4f0217..a3283be9cc00 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts @@ -38,7 +38,7 @@ sentryTest( value: 'HTTP Client Error with status code: 500', mechanism: { type: 'http.client', - handled: true, + handled: false, }, }, ], diff --git a/packages/integrations/src/captureconsole.ts b/packages/integrations/src/captureconsole.ts index 993fb9414052..124985662dd5 100644 --- a/packages/integrations/src/captureconsole.ts +++ b/packages/integrations/src/captureconsole.ts @@ -1,5 +1,6 @@ import type { EventProcessor, Hub, Integration } from '@sentry/types'; import { + addExceptionMechanism, addInstrumentationHandler, CONSOLE_LEVELS, GLOBAL_OBJ, @@ -64,6 +65,12 @@ function consoleHandler(hub: Hub, args: unknown[], level: string): void { scope.setExtra('arguments', args); scope.addEventProcessor(event => { event.logger = 'console'; + + addExceptionMechanism(event, { + handled: false, + type: 'console', + }); + return event; }); diff --git a/packages/integrations/src/httpclient.ts b/packages/integrations/src/httpclient.ts index 98da9d46d3af..5492116d7722 100644 --- a/packages/integrations/src/httpclient.ts +++ b/packages/integrations/src/httpclient.ts @@ -416,6 +416,7 @@ export class HttpClient implements Integration { addExceptionMechanism(event, { type: 'http.client', + handled: false, }); return event; diff --git a/packages/integrations/test/captureconsole.test.ts b/packages/integrations/test/captureconsole.test.ts index ba906f0ea2fd..9a107f8dbd66 100644 --- a/packages/integrations/test/captureconsole.test.ts +++ b/packages/integrations/test/captureconsole.test.ts @@ -397,4 +397,36 @@ describe('CaptureConsole setup', () => { GLOBAL_OBJ.console.log('some message'); }).not.toThrow(); }); + + it("marks captured exception's mechanism as unhandled", () => { + // const addExceptionMechanismSpy = jest.spyOn(utils, 'addExceptionMechanism'); + + const captureConsoleIntegration = new CaptureConsole({ levels: ['error'] }); + const mockHub = getMockHub(captureConsoleIntegration); + captureConsoleIntegration.setupOnce( + () => undefined, + () => mockHub, + ); + + const mockScope = mockHub.getScope(); + + const someError = new Error('some error'); + GLOBAL_OBJ.console.error(someError); + + const addedEventProcessor = (mockScope.addEventProcessor as jest.Mock).mock.calls[0][0]; + const someEvent: Event = { + exception: { + values: [{}], + }, + }; + addedEventProcessor(someEvent); + + expect(mockHub.captureException).toHaveBeenCalledTimes(1); + expect(mockScope.addEventProcessor).toHaveBeenCalledTimes(1); + + expect(someEvent.exception?.values?.[0].mechanism).toEqual({ + handled: false, + type: 'console', + }); + }); }); From b57e1391f0cc825e9b72e1ae142d2c858ae66e51 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 5 Sep 2023 09:33:36 +0100 Subject: [PATCH 26/29] fix(nextjs): Don't rexport default in route handlers (#8924) Co-authored-by: Lukas Stracke --- .../src/config/loaders/wrappingLoader.ts | 169 ++++++++++-------- .../templates/routeHandlerWrapperTemplate.ts | 16 +- .../templates/sentryInitWrapperTemplate.ts | 10 +- 3 files changed, 103 insertions(+), 92 deletions(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 4e19d27fea9b..fc3ced90f384 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -3,6 +3,7 @@ import { stringMatchesSomePattern } from '@sentry/utils'; import * as chalk from 'chalk'; import * as fs from 'fs'; import * as path from 'path'; +import type { RollupBuild, RollupError } from 'rollup'; import { rollup } from 'rollup'; import type { VercelCronsConfig } from '../../common/types'; @@ -277,85 +278,101 @@ async function wrapUserCode( userModuleCode: string, userModuleSourceMap: any, ): Promise<{ code: string; map?: any }> { - const rollupBuild = await rollup({ - input: SENTRY_WRAPPER_MODULE_NAME, - - plugins: [ - // We're using a simple custom plugin that virtualizes our wrapper module and the user module, so we don't have to - // mess around with file paths and so that we can pass the original user module source map to rollup so that - // rollup gives us a bundle with correct source mapping to the original file - { - name: 'virtualize-sentry-wrapper-modules', - resolveId: id => { - if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) { - return id; - } else { - return null; - } - }, - load(id) { - if (id === SENTRY_WRAPPER_MODULE_NAME) { - return wrapperCode; - } else if (id === WRAPPING_TARGET_MODULE_NAME) { - return { - code: userModuleCode, - map: userModuleSourceMap, // give rollup acces to original user module source map - }; - } else { - return null; - } + const wrap = (withDefaultExport: boolean): Promise => + rollup({ + input: SENTRY_WRAPPER_MODULE_NAME, + + plugins: [ + // We're using a simple custom plugin that virtualizes our wrapper module and the user module, so we don't have to + // mess around with file paths and so that we can pass the original user module source map to rollup so that + // rollup gives us a bundle with correct source mapping to the original file + { + name: 'virtualize-sentry-wrapper-modules', + resolveId: id => { + if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) { + return id; + } else { + return null; + } + }, + load(id) { + if (id === SENTRY_WRAPPER_MODULE_NAME) { + return withDefaultExport ? wrapperCode : wrapperCode.replace('export { default } from', 'export {} from'); + } else if (id === WRAPPING_TARGET_MODULE_NAME) { + return { + code: userModuleCode, + map: userModuleSourceMap, // give rollup acces to original user module source map + }; + } else { + return null; + } + }, }, + + // People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to + // handle that correctly so we let a plugin to take care of bundling cjs exports for us. + commonjs({ + sourceMap: true, + strictRequires: true, // Don't hoist require statements that users may define + ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context` + ignore() { + // We basically only want to use this plugin for handling the case where users export their handlers with module.exports. + // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. + // (Also, modifying require may break user code) + return true; + }, + }), + ], + + // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. + external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME, + + // Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the + // user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and + // https://stackoverflow.com/a/60347490.) + context: 'this', + + // Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root + // level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what + // seems to happen is this: + // + // - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'` + // - Rollup converts '../../utils/helper' into an absolute path + // - We mark the helper module as external + // - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is + // the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the + // bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped + // live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the + // root. Unclear why it's not.) + // - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'` + // rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in + // nextjs.. + // + // Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of + // externals entirely, with the result that their paths remain untouched (which is what we want). + makeAbsoluteExternalsRelative: false, + onwarn: (_warning, _warn) => { + // Suppress all warnings - we don't want to bother people with this output + // Might be stuff like "you have unused imports" + // _warn(_warning); // uncomment to debug }, + }); - // People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to - // handle that correctly so we let a plugin to take care of bundling cjs exports for us. - commonjs({ - sourceMap: true, - strictRequires: true, // Don't hoist require statements that users may define - ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context` - ignore() { - // We want basically only want to use this plugin for handling the case where users export their handlers with module.exports. - // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. - // (Also, modifying require may break user code) - return true; - }, - }), - ], - - // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. - external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME, - - // Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the - // user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and - // https://stackoverflow.com/a/60347490.) - context: 'this', - - // Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root - // level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what - // seems to happen is this: - // - // - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'` - // - Rollup converts '../../utils/helper' into an absolute path - // - We mark the helper module as external - // - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is - // the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the - // bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped - // live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the - // root. Unclear why it's not.) - // - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'` - // rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in - // nextjs.. - // - // Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of - // externals entirely, with the result that their paths remain untouched (which is what we want). - makeAbsoluteExternalsRelative: false, - - onwarn: (_warning, _warn) => { - // Suppress all warnings - we don't want to bother people with this output - // Might be stuff like "you have unused imports" - // _warn(_warning); // uncomment to debug - }, - }); + // Next.js sometimes complains if you define a default export (e.g. in route handlers in dev mode). + // This is why we want to avoid unnecessarily creating default exports, even if they're just `undefined`. + // For this reason we try to bundle/wrap the user code once including a re-export of `default`. + // If the user code didn't have a default export, rollup will throw. + // We then try bundling/wrapping agian, but without including a re-export of `default`. + let rollupBuild; + try { + rollupBuild = await wrap(true); + } catch (e) { + if ((e as RollupError)?.code === 'MISSING_EXPORT') { + rollupBuild = await wrap(false); + } else { + throw e; + } + } const finalBundle = await rollupBuild.generate({ format: 'esm', diff --git a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts index 813f860d35de..82fc8b1ad67a 100644 --- a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts @@ -13,7 +13,6 @@ import type { RequestAsyncStorage } from './requestAsyncStorageShim'; declare const requestAsyncStorage: RequestAsyncStorage; declare const routeModule: { - default: unknown; GET?: (...args: unknown[]) => unknown; POST?: (...args: unknown[]) => unknown; PUT?: (...args: unknown[]) => unknown; @@ -59,6 +58,14 @@ function wrapHandler(handler: T, method: 'GET' | 'POST' | 'PUT' | 'PATCH' | ' }); } +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; + +// @ts-ignore This is the file we're wrapping +// eslint-disable-next-line import/no-unresolved +export { default } from '__SENTRY_WRAPPING_TARGET_FILE__'; + export const GET = wrapHandler(routeModule.GET, 'GET'); export const POST = wrapHandler(routeModule.POST, 'POST'); export const PUT = wrapHandler(routeModule.PUT, 'PUT'); @@ -66,10 +73,3 @@ export const PATCH = wrapHandler(routeModule.PATCH, 'PATCH'); export const DELETE = wrapHandler(routeModule.DELETE, 'DELETE'); export const HEAD = wrapHandler(routeModule.HEAD, 'HEAD'); export const OPTIONS = wrapHandler(routeModule.OPTIONS, 'OPTIONS'); - -// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to -// not include anything whose name matchs something we've explicitly exported above. -// @ts-ignore See above -// eslint-disable-next-line import/no-unresolved -export * from '__SENTRY_WRAPPING_TARGET_FILE__'; -export default routeModule.default; diff --git a/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts b/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts index ab38854f090f..1720c3b62672 100644 --- a/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts @@ -4,14 +4,8 @@ import '__SENTRY_CONFIG_IMPORT_PATH__'; // @ts-ignore This is the file we're wrapping // eslint-disable-next-line import/no-unresolved -import * as wrappee from '__SENTRY_WRAPPING_TARGET_FILE__'; - -// @ts-ignore default either exists, or it doesn't - we don't care -// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -const defaultExport = wrappee.default; +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; // @ts-ignore This is the file we're wrapping // eslint-disable-next-line import/no-unresolved -export * from '__SENTRY_WRAPPING_TARGET_FILE__'; - -export default defaultExport; +export { default } from '__SENTRY_WRAPPING_TARGET_FILE__'; From ef5199384783ed2833c23f4f1a59479c599d9008 Mon Sep 17 00:00:00 2001 From: SorsOps <80043879+SorsOps@users.noreply.github.com> Date: Tue, 5 Sep 2023 11:34:35 +0200 Subject: [PATCH 27/29] fix(browser): Check for existence of instrumentation targets (#8939) There are cases when global objects such as the window object are shimmed that they define properties such as `document`, but the actual value is undefined. This exact situation has been occurring and is causing the instrumentDOM function to throw an error as `'document' in WINDOW` is technically true though the value is falsey. We should rather attempt an actual check of the value being truthy rather than if the property is defined Co-authored-by: Luca Forstner --- packages/utils/src/instrument.ts | 9 +++++---- packages/utils/test/instrument.test.ts | 18 +++++++++++++++++- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 157fe8a50c0b..f3364ba92b9c 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -247,8 +247,9 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str } /** JSDoc */ -function instrumentXHR(): void { - if (!('XMLHttpRequest' in WINDOW)) { +export function instrumentXHR(): void { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (!(WINDOW as any).XMLHttpRequest) { return; } @@ -539,8 +540,8 @@ type InstrumentedElement = Element & { }; /** JSDoc */ -function instrumentDOM(): void { - if (!('document' in WINDOW)) { +export function instrumentDOM(): void { + if (!WINDOW.document) { return; } diff --git a/packages/utils/test/instrument.test.ts b/packages/utils/test/instrument.test.ts index f9088ca1257a..f1ec46b93fb1 100644 --- a/packages/utils/test/instrument.test.ts +++ b/packages/utils/test/instrument.test.ts @@ -1,6 +1,22 @@ -import { parseFetchArgs } from '../src/instrument'; +import { instrumentDOM, instrumentXHR, parseFetchArgs } from '../src/instrument'; + +jest.mock('../src/worldwide', () => ({ + // Return an empty object with undefined properties + getGlobalObject: () => ({ + document: undefined, + XMLHttpRequest: undefined, + }), +})); describe('instrument', () => { + it('instrumentXHR() does not throw if XMLHttpRequest is a key on window but not defined', () => { + expect(instrumentXHR).not.toThrow(); + }); + + it('instrumentDOM() does not throw if XMLHttpRequest is a key on window but not defined', () => { + expect(instrumentDOM).not.toThrow(); + }); + describe('parseFetchArgs', () => { it.each([ ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com' }], From 262da82d535f8db4a3f9ea03597c9289360f6d9e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 5 Sep 2023 09:33:11 +0200 Subject: [PATCH 28/29] meta: Update CHANGELOG for 7.67.0 --- CHANGELOG.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95d043c5a6ba..b091aab494c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,39 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 7.67.0 + +### Important Changes + +- **feat: Mark errors caught by the SDK as unhandled** + - feat(browser): Mark errors caught from `TryCatch` integration as unhandled (#8890) + - feat(integrations): Mark errors caught from `HttpClient` and `CaptureConsole` integrations as unhandled (#8891) + - feat(nextjs): Mark errors caught from NextJS wrappers as unhandled (#8893) + - feat(react): Mark errors captured from ErrorBoundary as unhandled (#8914) + - feat(remix): Add debugid injection and map deletion to sourcemaps script (#8814) + - feat(remix): Mark errors caught from Remix instrumentation as unhandled (#8894) + - feat(serverless): Mark errors caught in Serverless handlers as unhandled (#8907) + - feat(vue): Mark errors caught by Vue wrappers as unhandled (#8905) + +This release fixes inconsistent behaviour of when our SDKs classify captured errors as unhandled. +Previously, some of our instrumentations correctly set unhandled, while others set handled. +Going forward, all errors caught automatically from our SDKs will be marked as unhandled. +If you manually capture errors (e.g. by calling `Sentry.captureException`), your errors will continue to be reported as handled. + +This change might lead to a decrease in reported crash-free sessions and consequently in your release health score. +If you have concerns about this, feel free to open an issue. + +### Other Changes + +- feat(node-experimental): Implement new performance APIs (#8911) +- feat(node-experimental): Sync OTEL context with Sentry AsyncContext (#8797) +- feat(replay): Allow to configure `maxReplayDuration` (#8769) +- fix(browser): Add replay and profiling options to `BrowserClientOptions` (#8921) +- fix(node): Improve mysql integration (#8923) +- fix(remix): Guard against missing default export for server instrument (#8909) +- ref(browser): Deprecate top-level `wrap` function (#8927) +- ref(node-otel): Avoid exporting internals & refactor attribute adding (#8920) + ## 7.66.0 - fix: Defer tracing decision to downstream SDKs when using SDK without performance (#8839) From 09f7f7e8d2ba3a6cc54acf3a4f9121451636857d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 5 Sep 2023 11:41:15 +0200 Subject: [PATCH 29/29] update changelog after rebase --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b091aab494c5..29701eff6213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,11 +32,15 @@ If you have concerns about this, feel free to open an issue. - feat(node-experimental): Sync OTEL context with Sentry AsyncContext (#8797) - feat(replay): Allow to configure `maxReplayDuration` (#8769) - fix(browser): Add replay and profiling options to `BrowserClientOptions` (#8921) +- fix(browser): Check for existence of instrumentation targets (#8939) +- fix(nextjs): Don't re-export default in route handlers (#8924) - fix(node): Improve mysql integration (#8923) - fix(remix): Guard against missing default export for server instrument (#8909) - ref(browser): Deprecate top-level `wrap` function (#8927) - ref(node-otel): Avoid exporting internals & refactor attribute adding (#8920) +Work in this release contributed by @SorsOps. Thank you for your contribution! + ## 7.66.0 - fix: Defer tracing decision to downstream SDKs when using SDK without performance (#8839)