From 9d8464b0a6b8d72782d7b74a1a4ad7355abf2d2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kry=C5=A1tof=20Wold=C5=99ich?= <31292499+krystofwoldrich@users.noreply.github.com> Date: Tue, 20 Jun 2023 11:19:18 +0200 Subject: [PATCH 01/20] chore(profiling): Move profiling types to types and cache to utils (#8270) --- packages/browser/src/profiling/cache.ts | 72 +----------------- .../browser/src/profiling/jsSelfProfiling.ts | 60 --------------- packages/browser/src/profiling/utils.ts | 10 +-- packages/hub/package.json | 3 +- packages/types/src/index.ts | 10 +++ packages/types/src/profiling.ts | 76 +++++++++++++++++++ packages/utils/src/cache.ts | 68 +++++++++++++++++ packages/utils/src/index.ts | 1 + 8 files changed, 162 insertions(+), 138 deletions(-) create mode 100644 packages/types/src/profiling.ts create mode 100644 packages/utils/src/cache.ts diff --git a/packages/browser/src/profiling/cache.ts b/packages/browser/src/profiling/cache.ts index ee62538e60cb..34b5da5d12fa 100644 --- a/packages/browser/src/profiling/cache.ts +++ b/packages/browser/src/profiling/cache.ts @@ -1,72 +1,4 @@ import type { Event } from '@sentry/types'; +import { makeFifoCache } from '@sentry/utils'; -/** - * Creates a cache that evicts keys in fifo order - * @param size {Number} - */ -export function makeProfilingCache( - size: number, -): { - get: (key: Key) => Value | undefined; - add: (key: Key, value: Value) => void; - delete: (key: Key) => boolean; - clear: () => void; - size: () => number; -} { - // Maintain a fifo queue of keys, we cannot rely on Object.keys as the browser may not support it. - let evictionOrder: Key[] = []; - let cache: Record = {}; - - return { - add(key: Key, value: Value) { - while (evictionOrder.length >= size) { - // shift is O(n) but this is small size and only happens if we are - // exceeding the cache size so it should be fine. - const evictCandidate = evictionOrder.shift(); - - if (evictCandidate !== undefined) { - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete cache[evictCandidate]; - } - } - - // in case we have a collision, delete the old key. - if (cache[key]) { - this.delete(key); - } - - evictionOrder.push(key); - cache[key] = value; - }, - clear() { - cache = {}; - evictionOrder = []; - }, - get(key: Key): Value | undefined { - return cache[key]; - }, - size() { - return evictionOrder.length; - }, - // Delete cache key and return true if it existed, false otherwise. - delete(key: Key): boolean { - if (!cache[key]) { - return false; - } - - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete cache[key]; - - for (let i = 0; i < evictionOrder.length; i++) { - if (evictionOrder[i] === key) { - evictionOrder.splice(i, 1); - break; - } - } - - return true; - }, - }; -} - -export const PROFILING_EVENT_CACHE = makeProfilingCache(20); +export const PROFILING_EVENT_CACHE = makeFifoCache(20); diff --git a/packages/browser/src/profiling/jsSelfProfiling.ts b/packages/browser/src/profiling/jsSelfProfiling.ts index af9ebada8cbf..efa4a0a0a0bc 100644 --- a/packages/browser/src/profiling/jsSelfProfiling.ts +++ b/packages/browser/src/profiling/jsSelfProfiling.ts @@ -53,63 +53,3 @@ declare global { export interface RawThreadCpuProfile extends JSSelfProfile { profile_id: string; } -export interface ThreadCpuProfile { - samples: { - stack_id: number; - thread_id: string; - elapsed_since_start_ns: string; - }[]; - stacks: number[][]; - frames: { - function: string; - file: string | undefined; - line: number | undefined; - column: number | undefined; - }[]; - thread_metadata: Record; - queue_metadata?: Record; -} - -export interface SentryProfile { - event_id: string; - version: string; - os: { - name: string; - version: string; - build_number: string; - }; - runtime: { - name: string; - version: string; - }; - device: { - architecture: string; - is_emulator: boolean; - locale: string; - manufacturer: string; - model: string; - }; - timestamp: string; - release: string; - environment: string; - platform: string; - profile: ThreadCpuProfile; - debug_meta?: { - images: { - debug_id: string; - image_addr: string; - code_file: string; - type: string; - image_size: number; - image_vmaddr: string; - }[]; - }; - transactions: { - name: string; - trace_id: string; - id: string; - active_thread_id: string; - relative_start_ns: string; - relative_end_ns: string; - }[]; -} diff --git a/packages/browser/src/profiling/utils.ts b/packages/browser/src/profiling/utils.ts index 9fc068f7c0ee..7b2e1b60e848 100644 --- a/packages/browser/src/profiling/utils.ts +++ b/packages/browser/src/profiling/utils.ts @@ -6,19 +6,15 @@ import type { EventEnvelope, EventEnvelopeHeaders, EventItem, + Profile as SentryProfile, SdkInfo, SdkMetadata, + ThreadCpuProfile, } from '@sentry/types'; import { createEnvelope, dropUndefinedKeys, dsnToString, logger, uuid4 } from '@sentry/utils'; import { WINDOW } from '../helpers'; -import type { - JSSelfProfile, - JSSelfProfileStack, - RawThreadCpuProfile, - SentryProfile, - ThreadCpuProfile, -} from './jsSelfProfiling'; +import type { JSSelfProfile, JSSelfProfileStack, RawThreadCpuProfile } from './jsSelfProfiling'; const MS_TO_NS = 1e6; // Use 0 as main thread id which is identical to threadId in node:worker_threads diff --git a/packages/hub/package.json b/packages/hub/package.json index ca34143369db..bc795aa6bee5 100644 --- a/packages/hub/package.json +++ b/packages/hub/package.json @@ -40,7 +40,8 @@ "lint:eslint": "eslint . --format stylish", "lint:prettier": "prettier --check \"{src,test,scripts}/**/**.ts\"", "test": "jest", - "test:watch": "jest --watch" + "test:watch": "jest --watch", + "yalc:publish": "ts-node ../../scripts/prepack.ts && yalc publish build --push" }, "volta": { "extends": "../../package.json" diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 1da1778d2012..ccabae59a995 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -49,6 +49,16 @@ export type { ExtractedNodeRequestData, HttpHeaderValue, Primitive, WorkerLocati export type { ClientOptions, Options } from './options'; export type { Package } from './package'; export type { PolymorphicEvent, PolymorphicRequest } from './polymorphics'; +export type { + ThreadId, + FrameId, + StackId, + ThreadCpuSample, + ThreadCpuStack, + ThreadCpuFrame, + ThreadCpuProfile, + Profile, +} from './profiling'; export type { ReplayEvent, ReplayRecordingData, ReplayRecordingMode } from './replay'; export type { QueryParams, Request, SanitizedRequestData } from './request'; export type { Runtime } from './runtime'; diff --git a/packages/types/src/profiling.ts b/packages/types/src/profiling.ts new file mode 100644 index 000000000000..1b6dac566901 --- /dev/null +++ b/packages/types/src/profiling.ts @@ -0,0 +1,76 @@ +export type ThreadId = string; +export type FrameId = number; +export type StackId = number; + +export interface ThreadCpuSample { + stack_id: StackId; + thread_id: ThreadId; + elapsed_since_start_ns: string; +} + +export type ThreadCpuStack = FrameId[]; + +export type ThreadCpuFrame = { + function: string; + file?: string; + line?: number; + column?: number; +}; + +export interface ThreadCpuProfile { + samples: ThreadCpuSample[]; + stacks: ThreadCpuStack[]; + frames: ThreadCpuFrame[]; + thread_metadata: Record; + queue_metadata?: Record; +} + +export interface Profile { + event_id: string; + version: string; + os: { + name: string; + version: string; + build_number?: string; + }; + runtime: { + name: string; + version: string; + }; + device: { + architecture: string; + is_emulator: boolean; + locale: string; + manufacturer: string; + model: string; + }; + timestamp: string; + release: string; + environment: string; + platform: string; + profile: ThreadCpuProfile; + debug_meta?: { + images: { + debug_id: string; + image_addr: string; + code_file: string; + type: string; + image_size: number; + image_vmaddr: string; + }[]; + }; + transaction?: { + name: string; + id: string; + trace_id: string; + active_thread_id: string; + }; + transactions?: { + name: string; + id: string; + trace_id: string; + active_thread_id: string; + relative_start_ns: string; + relative_end_ns: string; + }[]; +} diff --git a/packages/utils/src/cache.ts b/packages/utils/src/cache.ts new file mode 100644 index 000000000000..412970e77c76 --- /dev/null +++ b/packages/utils/src/cache.ts @@ -0,0 +1,68 @@ +/** + * Creates a cache that evicts keys in fifo order + * @param size {Number} + */ +export function makeFifoCache( + size: number, +): { + get: (key: Key) => Value | undefined; + add: (key: Key, value: Value) => void; + delete: (key: Key) => boolean; + clear: () => void; + size: () => number; +} { + // Maintain a fifo queue of keys, we cannot rely on Object.keys as the browser may not support it. + let evictionOrder: Key[] = []; + let cache: Record = {}; + + return { + add(key: Key, value: Value) { + while (evictionOrder.length >= size) { + // shift is O(n) but this is small size and only happens if we are + // exceeding the cache size so it should be fine. + const evictCandidate = evictionOrder.shift(); + + if (evictCandidate !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete cache[evictCandidate]; + } + } + + // in case we have a collision, delete the old key. + if (cache[key]) { + this.delete(key); + } + + evictionOrder.push(key); + cache[key] = value; + }, + clear() { + cache = {}; + evictionOrder = []; + }, + get(key: Key): Value | undefined { + return cache[key]; + }, + size() { + return evictionOrder.length; + }, + // Delete cache key and return true if it existed, false otherwise. + delete(key: Key): boolean { + if (!cache[key]) { + return false; + } + + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete cache[key]; + + for (let i = 0; i < evictionOrder.length; i++) { + if (evictionOrder[i] === key) { + evictionOrder.splice(i, 1); + break; + } + } + + return true; + }, + }; +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index c5631559a9aa..6b9426c22149 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -28,3 +28,4 @@ export * from './ratelimit'; export * from './baggage'; export * from './url'; export * from './userIntegrations'; +export * from './cache'; From c8686ffaee0844d86d6684df742dd2bf45792aae Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 20 Jun 2023 14:22:36 +0200 Subject: [PATCH 02/20] fix(core): Only start spans in `trace` if tracing is enabled (#8357) In our (still internal) `trace` function, we don't yet check if `hasTracingEnabled` returns `true` before trying to start a span or a transaction. This leads to the following problematic UX path that was discovered [here](https://github.com/getsentry/sentry-javascript/discussions/5838#discussioncomment-6217855): 1. Users don't set `tracesSampleRate` (or `tracesSampler`), causing some SDKs (NextJS, SvelteKit) to **not** add the `BrowserTracing` integration, which in turn means that tracing extension methods are not added/registered. 2. Users or, more likely, other parts of our SDK (e.g. SvelteKit's wrappers/handlers) call `trace` which will still try to start a span/txn. 3. Users will get a console warning that tracing extension methods were not installed and they should manually call `addExtensionMethods` which is completely misleading in this case. This fix makes `trace` check for `hasTracingEnabled()` in which case it will not start a span but just invoke the error callback if an error occurred. --- packages/core/src/tracing/trace.ts | 16 ++++++++--- packages/core/test/lib/tracing/trace.test.ts | 28 ++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 8e7844d23988..2864377bfc04 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -2,13 +2,15 @@ import type { TransactionContext } from '@sentry/types'; import { isThenable } from '@sentry/utils'; import { getCurrentHub } from '../hub'; +import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import type { Span } from './span'; /** * Wraps a function with a transaction/span and finishes the span after the function is done. * - * Note that if you have not enabled tracing extensions via `addTracingExtensions`, this function - * will not generate spans, and the `span` returned from the callback may be undefined. + * 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. * * This function is meant to be used internally and may break at any time. Use at your own risk. * @@ -31,7 +33,15 @@ export function trace( const scope = hub.getScope(); const parentSpan = scope.getSpan(); - const activeSpan = parentSpan ? parentSpan.startChild(ctx) : hub.startTransaction(ctx); + + function getActiveSpan(): Span | undefined { + if (!hasTracingEnabled()) { + return undefined; + } + return parentSpan ? parentSpan.startChild(ctx) : hub.startTransaction(ctx); + } + + const activeSpan = getActiveSpan(); scope.setSpan(activeSpan); function finishAndSetSpan(): void { diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 064c41dc123a..bff1c425c2a0 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -185,5 +185,33 @@ describe('trace', () => { } expect(onError).toHaveBeenCalledTimes(isError ? 1 : 0); }); + + it("doesn't create spans but calls onError if tracing is disabled", async () => { + const options = getDefaultTestClientOptions({ + /* we don't set tracesSampleRate or tracesSampler */ + }); + client = new TestClient(options); + hub = new Hub(client); + makeMain(hub); + + const startTxnSpy = jest.spyOn(hub, 'startTransaction'); + + const onError = jest.fn(); + try { + await trace( + { name: 'GET users/[id]' }, + () => { + return callback(); + }, + onError, + ); + } catch (e) { + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(e); + } + expect(onError).toHaveBeenCalledTimes(isError ? 1 : 0); + + expect(startTxnSpy).not.toHaveBeenCalled(); + }); }); }); From 365c75085e85af615958d28c0202c587fce129bb Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 20 Jun 2023 16:26:55 +0200 Subject: [PATCH 03/20] fix(angular): Stop routing spans on navigation cancel and error events (#8369) Previously, in our Angular routing instrumentation, we only stopped routing spans when a navigation ended successfully. This was because we only listened to [`NavigationEnd`](https://angular.io/api/router/NavigationEnd) router events. However, the Angular router emits other events in unsuccessful routing attempts: * a routing error (e.g. unknown route) emits [`NavigationError`](https://angular.io/api/router/NavigationCancel) * a cancelled navigation (e.g. due to a route guard that rejected/returned false) emits [`NavigationCancel`](https://angular.io/api/router/NavigationCancel) This fix adjusts our instrumentation to also listen to these events and to stop the span accordingly. --- packages/angular/src/tracing.ts | 6 ++- packages/angular/test/tracing.test.ts | 62 ++++++++++++++++++++++++++- packages/angular/test/utils/index.ts | 22 +++++++--- 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index b206d7fe429d..f2e79adfe9b0 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -5,7 +5,7 @@ import type { ActivatedRouteSnapshot, Event, RouterState } from '@angular/router // Duplicated import to work around a TypeScript bug where it'd complain that `Router` isn't imported as a type. // We need to import it as a value to satisfy Angular dependency injection. So: // eslint-disable-next-line @typescript-eslint/consistent-type-imports, import/no-duplicates -import { Router } from '@angular/router'; +import { NavigationCancel, NavigationError, Router } from '@angular/router'; // eslint-disable-next-line import/no-duplicates import { NavigationEnd, NavigationStart, ResolveEnd } from '@angular/router'; import { getCurrentHub, WINDOW } from '@sentry/browser'; @@ -131,7 +131,9 @@ export class TraceService implements OnDestroy { ); public navEnd$: Observable = this._router.events.pipe( - filter(event => event instanceof NavigationEnd), + filter( + event => event instanceof NavigationEnd || event instanceof NavigationCancel || event instanceof NavigationError, + ), tap(() => { if (this._routingSpan) { runOutsideAngular(() => { diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 0afef2771add..a3375518466a 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,5 +1,5 @@ import { Component } from '@angular/core'; -import type { ActivatedRouteSnapshot } from '@angular/router'; +import type { ActivatedRouteSnapshot, CanActivate, RouterStateSnapshot } from '@angular/router'; import type { Hub } from '@sentry/types'; import { instrumentAngularRouting, TraceClassDecorator, TraceDirective, TraceMethodDecorator } from '../src'; @@ -185,6 +185,66 @@ describe('Angular Tracing', () => { env.destroy(); }); + it('finishes routing span on navigation error', async () => { + const customStartTransaction = jest.fn(defaultStartTransaction); + + const env = await TestEnv.setup({ + customStartTransaction, + routes: [ + { + path: '', + component: AppComponent, + }, + ], + useTraceService: true, + }); + + const finishMock = jest.fn(); + transaction.startChild = jest.fn(() => ({ + finish: finishMock, + })); + + await env.navigateInAngular('/somewhere'); + + expect(finishMock).toHaveBeenCalledTimes(1); + + env.destroy(); + }); + + it('finishes routing span on navigation cancel', async () => { + const customStartTransaction = jest.fn(defaultStartTransaction); + + class CanActivateGuard implements CanActivate { + canActivate(_route: ActivatedRouteSnapshot, _state: RouterStateSnapshot): boolean { + return false; + } + } + + const env = await TestEnv.setup({ + customStartTransaction, + routes: [ + { + path: 'cancel', + component: AppComponent, + canActivate: [CanActivateGuard], + }, + ], + useTraceService: true, + additionalProviders: [{ provide: CanActivateGuard, useClass: CanActivateGuard }], + }); + + const finishMock = jest.fn(); + transaction.startChild = jest.fn(() => ({ + finish: finishMock, + })); + + await env.navigateInAngular('/cancel'); + + expect(finishMock).toHaveBeenCalledTimes(1); + + env.destroy(); + }); + describe('URL parameterization', () => { it.each([ [ diff --git a/packages/angular/test/utils/index.ts b/packages/angular/test/utils/index.ts index b15ad2028560..daa23155d931 100644 --- a/packages/angular/test/utils/index.ts +++ b/packages/angular/test/utils/index.ts @@ -1,3 +1,4 @@ +import type { Provider } from '@angular/core'; import { Component, NgModule } from '@angular/core'; import type { ComponentFixture } from '@angular/core/testing'; import { TestBed } from '@angular/core/testing'; @@ -47,6 +48,7 @@ export class TestEnv { startTransactionOnPageLoad?: boolean; startTransactionOnNavigation?: boolean; useTraceService?: boolean; + additionalProviders?: Provider[]; }): Promise { instrumentAngularRouting( conf.customStartTransaction || jest.fn(), @@ -60,14 +62,16 @@ export class TestEnv { TestBed.configureTestingModule({ imports: [AppModule, RouterTestingModule.withRoutes(routes)], declarations: [...(conf.components || []), AppComponent], - providers: useTraceService + providers: (useTraceService ? [ { provide: TraceService, deps: [Router], }, + ...(conf.additionalProviders || []), ] - : [], + : [] + ).concat(...(conf.additionalProviders || [])), }); const router: Router = TestBed.inject(Router); @@ -80,10 +84,16 @@ export class TestEnv { public async navigateInAngular(url: string): Promise { return new Promise(resolve => { return this.fixture.ngZone?.run(() => { - void this.router.navigateByUrl(url).then(() => { - this.fixture.detectChanges(); - resolve(); - }); + void this.router + .navigateByUrl(url) + .then(() => { + this.fixture.detectChanges(); + resolve(); + }) + .catch(() => { + this.fixture.detectChanges(); + resolve(); + }); }); }); } From e5e6a6bd8ab2bbec59879971595a7248fa132826 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 20 Jun 2023 16:27:53 +0200 Subject: [PATCH 04/20] fix(angular): Filter out `TryCatch` integration by default (#8367) The `TryCatch` default integration interferes with the `SentryErrorHander` error handler of the Angular(-Ivy) SDKs by catching certain errors too early, before the Angular SDK-specific error handler can catch them. This caused missing data on the event in some or duplicated errors in other cases. This fix filters out the `TryCatch` by default, as long as users didn't set `defaultIntegrations` in their SDK init. Therefore, it makes the previously provided [workaround](https://github.com/getsentry/sentry-javascript/issues/5417#issuecomment-1453407097) obsolete. --- packages/angular-ivy/src/sdk.ts | 14 +++++++++++++- packages/angular/src/sdk.ts | 14 +++++++++++++- packages/angular/test/sdk.test.ts | 31 ++++++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/packages/angular-ivy/src/sdk.ts b/packages/angular-ivy/src/sdk.ts index d16d16009ca0..fcbbbce399d0 100644 --- a/packages/angular-ivy/src/sdk.ts +++ b/packages/angular-ivy/src/sdk.ts @@ -1,6 +1,6 @@ import { VERSION } from '@angular/core'; import type { BrowserOptions } from '@sentry/browser'; -import { init as browserInit, SDK_VERSION, setContext } from '@sentry/browser'; +import { defaultIntegrations, init as browserInit, SDK_VERSION, setContext } from '@sentry/browser'; import { logger } from '@sentry/utils'; import { IS_DEBUG_BUILD } from './flags'; @@ -21,6 +21,18 @@ export function init(options: BrowserOptions): void { version: SDK_VERSION, }; + // Filter out TryCatch integration as it interferes with our Angular `ErrorHandler`: + // TryCatch would catch certain errors before they reach the `ErrorHandler` and thus provide a + // lower fidelity error than what `SentryErrorHandler` (see errorhandler.ts) would provide. + // see: + // - https://github.com/getsentry/sentry-javascript/issues/5417#issuecomment-1453407097 + // - https://github.com/getsentry/sentry-javascript/issues/2744 + if (options.defaultIntegrations === undefined) { + options.defaultIntegrations = defaultIntegrations.filter(integration => { + return integration.name !== 'TryCatch'; + }); + } + checkAndSetAngularVersion(); browserInit(options); } diff --git a/packages/angular/src/sdk.ts b/packages/angular/src/sdk.ts index 4afce6259c2b..e50cece043d0 100755 --- a/packages/angular/src/sdk.ts +++ b/packages/angular/src/sdk.ts @@ -1,6 +1,6 @@ import { VERSION } from '@angular/core'; import type { BrowserOptions } from '@sentry/browser'; -import { init as browserInit, SDK_VERSION, setContext } from '@sentry/browser'; +import { defaultIntegrations, init as browserInit, SDK_VERSION, setContext } from '@sentry/browser'; import { logger } from '@sentry/utils'; import { IS_DEBUG_BUILD } from './flags'; @@ -21,6 +21,18 @@ export function init(options: BrowserOptions): void { version: SDK_VERSION, }; + // Filter out TryCatch integration as it interferes with our Angular `ErrorHandler`: + // TryCatch would catch certain errors before they reach the `ErrorHandler` and thus provide a + // lower fidelity error than what `SentryErrorHandler` (see errorhandler.ts) would provide. + // see: + // - https://github.com/getsentry/sentry-javascript/issues/5417#issuecomment-1453407097 + // - https://github.com/getsentry/sentry-javascript/issues/2744 + if (options.defaultIntegrations === undefined) { + options.defaultIntegrations = defaultIntegrations.filter(integration => { + return integration.name !== 'TryCatch'; + }); + } + checkAndSetAngularVersion(); browserInit(options); } diff --git a/packages/angular/test/sdk.test.ts b/packages/angular/test/sdk.test.ts index bf5ecabb0ac5..0a7244d424f7 100644 --- a/packages/angular/test/sdk.test.ts +++ b/packages/angular/test/sdk.test.ts @@ -1,6 +1,6 @@ import * as SentryBrowser from '@sentry/browser'; -import { init } from '../src/sdk'; +import { defaultIntegrations, init } from '../src/index'; describe('init', () => { it('sets the Angular version (if available) in the global scope', () => { @@ -13,4 +13,33 @@ describe('init', () => { expect(setContextSpy).toHaveBeenCalledTimes(1); expect(setContextSpy).toHaveBeenCalledWith('angular', { version: 10 }); }); + + describe('filtering out the `TryCatch` integration', () => { + const browserInitSpy = jest.spyOn(SentryBrowser, 'init'); + + beforeEach(() => { + browserInitSpy.mockClear(); + }); + + it('filters if `defaultIntegrations` is not set', () => { + init({}); + + expect(browserInitSpy).toHaveBeenCalledTimes(1); + + const options = browserInitSpy.mock.calls[0][0] || {}; + expect(options.defaultIntegrations).not.toContainEqual(expect.objectContaining({ name: 'TryCatch' })); + }); + + it.each([false as const, defaultIntegrations])( + "doesn't filter if `defaultIntegrations` is set to %s", + defaultIntegrations => { + init({ defaultIntegrations }); + + expect(browserInitSpy).toHaveBeenCalledTimes(1); + + const options = browserInitSpy.mock.calls[0][0] || {}; + expect(options.defaultIntegrations).toEqual(defaultIntegrations); + }, + ); + }); }); From b84c23f6a43014396a8d89de827b1c1ad17bd737 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 Jun 2023 09:16:54 +0200 Subject: [PATCH 05/20] test(e2e): Ensure sveltekit E2E test work with prereleases (#8372) Since @sentry/sveltekit depends on @sentry/vite-plugin, which in turn depens on `@sentry/node@^7.19.0` & `@sentry/tracing@^7.19.0`, this fails to install in E2E tests for pre-release versions (e.g. `7.57.0-beta.0`), as the prerelease does not satisfy the range `^7.19.0`. So we override this to `*` to ensure this works as expected. --- packages/e2e-tests/test-applications/sveltekit/package.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/e2e-tests/test-applications/sveltekit/package.json b/packages/e2e-tests/test-applications/sveltekit/package.json index ab1a2c9cac2e..003868af5ab8 100644 --- a/packages/e2e-tests/test-applications/sveltekit/package.json +++ b/packages/e2e-tests/test-applications/sveltekit/package.json @@ -27,5 +27,11 @@ "vite": "^4.2.0", "wait-port": "1.0.4" }, + "pnpm": { + "overrides": { + "@sentry/node": "*", + "@sentry/tracing": "*" + } + }, "type": "module" } From 4fb043e875a50611f77ff7c1904b8e0c0fce0e2c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 21 Jun 2023 09:49:14 +0200 Subject: [PATCH 06/20] fix(nextjs): Inject init calls via loader instead of via entrypoints (#8368) --- .github/workflows/build.yml | 2 +- .../src/config/loaders/wrappingLoader.ts | 18 +-- packages/nextjs/src/config/webpack.ts | 42 +---- .../webpack/constructWebpackConfig.test.ts | 149 ------------------ 4 files changed, 18 insertions(+), 193 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 990361d6ae7c..3bc95b171d94 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -424,7 +424,7 @@ jobs: name: Nextjs (Node ${{ matrix.node }}) Tests needs: [job_get_metadata, job_build] if: needs.job_get_metadata.outputs.changed_nextjs == 'true' || github.event_name != 'pull_request' - timeout-minutes: 15 + timeout-minutes: 25 runs-on: ubuntu-20.04 strategy: fail-fast: false diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 3157d41df71f..e4d58c579420 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -201,21 +201,21 @@ export default function wrappingLoader( } else { templateCode = templateCode.replace(/__COMPONENT_TYPE__/g, 'Unknown'); } - - // We check whether `this.resourcePath` is absolute because there is no contract by webpack that says it is absolute, - // however we can only create relative paths to the sentry config from absolute paths.Examples where this could possibly be non - absolute are virtual modules. - if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { - const sentryConfigImportPath = path - .relative(path.dirname(this.resourcePath), sentryConfigFilePath) // Absolute paths do not work with Windows: https://github.com/getsentry/sentry-javascript/issues/8133 - .replace(/\\/g, '/'); - templateCode = `import "${sentryConfigImportPath}";\n`.concat(templateCode); - } } else if (wrappingTargetKind === 'middleware') { templateCode = middlewareWrapperTemplateCode; } else { throw new Error(`Invariant: Could not get template code of unknown kind "${wrappingTargetKind}"`); } + // We check whether `this.resourcePath` is absolute because there is no contract by webpack that says it is absolute, + // however we can only create relative paths to the sentry config from absolute paths.Examples where this could possibly be non - absolute are virtual modules. + if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { + const sentryConfigImportPath = path + .relative(path.dirname(this.resourcePath), sentryConfigFilePath) // Absolute paths do not work with Windows: https://github.com/getsentry/sentry-javascript/issues/8133 + .replace(/\\/g, '/'); + templateCode = `import "${sentryConfigImportPath}";\n`.concat(templateCode); + } + // Replace the import path of the wrapping target in the template with a path that the `wrapUserCode` function will understand. templateCode = templateCode.replace(/__SENTRY_WRAPPING_TARGET_FILE__/g, WRAPPING_TARGET_MODULE_NAME); diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 0bb42f98b7ec..7237d8ce24be 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -1,7 +1,7 @@ /* eslint-disable complexity */ /* eslint-disable max-lines */ import { getSentryRelease } from '@sentry/node'; -import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils'; +import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger } from '@sentry/utils'; import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; import * as chalk from 'chalk'; import * as fs from 'fs'; @@ -441,7 +441,7 @@ async function addSentryToEntryProperty( // inject into all entry points which might contain user's code for (const entryPointName in newEntryProperty) { - if (shouldAddSentryToEntryPoint(entryPointName, runtime, userSentryOptions.excludeServerRoutes ?? [])) { + if (shouldAddSentryToEntryPoint(entryPointName, runtime)) { addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject); } else { if ( @@ -589,39 +589,13 @@ function checkWebpackPluginOverrides( * @param excludeServerRoutes A list of excluded serverside entrypoints provided by the user * @returns `true` if sentry code should be injected, and `false` otherwise */ -function shouldAddSentryToEntryPoint( - entryPointName: string, - runtime: 'node' | 'browser' | 'edge', - excludeServerRoutes: Array, -): boolean { - // On the server side, by default we inject the `Sentry.init()` code into every page (with a few exceptions). - if (runtime === 'node') { - // User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes, - // which don't have the `pages` prefix.) - const entryPointRoute = entryPointName.replace(/^pages/, ''); - if (stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true)) { - return false; - } - - // This expression will implicitly include `pages/_app` which is called for all serverside routes and pages - // regardless whether or not the user has a`_app` file. - return entryPointName.startsWith('pages/'); - } else if (runtime === 'browser') { - return ( - // entrypoint for `/pages` pages - this is included on all clientside pages - // It's important that we inject the SDK into this file and not into 'main' because in 'main' - // some important Next.js code (like the setup code for getCongig()) is located and some users - // may need this code inside their Sentry configs - entryPointName === 'pages/_app' || +function shouldAddSentryToEntryPoint(entryPointName: string, runtime: 'node' | 'browser' | 'edge'): boolean { + return ( + runtime === 'browser' && + (entryPointName === 'pages/_app' || // entrypoint for `/app` pages - entryPointName === 'main-app' - ); - } else { - // User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes, - // which don't have the `pages` prefix.) - const entryPointRoute = entryPointName.replace(/^pages/, ''); - return !stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true); - } + entryPointName === 'main-app') + ); } /** diff --git a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts index 01f89bed1077..86bb2d03d3fd 100644 --- a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts +++ b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts @@ -7,10 +7,7 @@ import { CLIENT_SDK_CONFIG_FILE, clientBuildContext, clientWebpackConfig, - EDGE_SDK_CONFIG_FILE, - edgeBuildContext, exportedNextConfig, - SERVER_SDK_CONFIG_FILE, serverBuildContext, serverWebpackConfig, userNextConfig, @@ -88,74 +85,15 @@ describe('constructWebpackConfigFunction()', () => { }); describe('webpack `entry` property config', () => { - const serverConfigFilePath = `./${SERVER_SDK_CONFIG_FILE}`; const clientConfigFilePath = `./${CLIENT_SDK_CONFIG_FILE}`; - const edgeConfigFilePath = `./${EDGE_SDK_CONFIG_FILE}`; - - it('handles various entrypoint shapes', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - // original entrypoint value is a string - // (was 'private-next-pages/_error.js') - 'pages/_error': [serverConfigFilePath, 'private-next-pages/_error.js'], - - // original entrypoint value is a string array - // (was ['./node_modules/smellOVision/index.js', 'private-next-pages/sniffTour.js']) - 'pages/sniffTour': [ - serverConfigFilePath, - './node_modules/smellOVision/index.js', - 'private-next-pages/sniffTour.js', - ], - - // original entrypoint value is an object containing a string `import` value - // (was { import: 'private-next-pages/api/simulator/dogStats/[name].js' }) - 'pages/api/simulator/dogStats/[name]': { - import: [serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'], - }, - - // original entrypoint value is an object containing a string array `import` value - // (was { import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/simulator/leaderboard.js'] }) - 'pages/simulator/leaderboard': { - import: [ - serverConfigFilePath, - './node_modules/dogPoints/converter.js', - 'private-next-pages/simulator/leaderboard.js', - ], - }, - - // original entrypoint value is an object containg properties besides `import` - // (was { import: 'private-next-pages/api/tricks/[trickName].js', dependOn: 'treats', }) - 'pages/api/tricks/[trickName]': { - import: [serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'], - dependOn: 'treats', // untouched - }, - }), - ); - }); it('injects user config file into `_app` in server bundle and in the client bundle', async () => { - const finalServerWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); const finalClientWebpackConfig = await materializeFinalWebpackConfig({ exportedNextConfig, incomingWebpackConfig: clientWebpackConfig, incomingWebpackBuildContext: clientBuildContext, }); - expect(finalServerWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/_app': expect.arrayContaining([serverConfigFilePath]), - }), - ); expect(finalClientWebpackConfig.entry).toEqual( expect.objectContaining({ 'pages/_app': expect.arrayContaining([clientConfigFilePath]), @@ -163,68 +101,6 @@ describe('constructWebpackConfigFunction()', () => { ); }); - it('injects user config file into `_error` in server bundle but not client bundle', async () => { - const finalServerWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - const finalClientWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: clientWebpackConfig, - incomingWebpackBuildContext: clientBuildContext, - }); - - expect(finalServerWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/_error': expect.arrayContaining([serverConfigFilePath]), - }), - ); - expect(finalClientWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/_error': expect.not.arrayContaining([clientConfigFilePath]), - }), - ); - }); - - it('injects user config file into both API routes and non-API routes', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/api/simulator/dogStats/[name]': { - import: expect.arrayContaining([serverConfigFilePath]), - }, - - 'pages/api/tricks/[trickName]': expect.objectContaining({ - import: expect.arrayContaining([serverConfigFilePath]), - }), - - 'pages/simulator/leaderboard': { - import: expect.arrayContaining([serverConfigFilePath]), - }, - }), - ); - }); - - it('injects user config file into API middleware', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: edgeBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - middleware: [edgeConfigFilePath, 'private-next-pages/middleware.js'], - }), - ); - }); - it('does not inject anything into non-_app pages during client build', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ exportedNextConfig, @@ -244,30 +120,5 @@ describe('constructWebpackConfigFunction()', () => { simulatorBundle: './src/simulator/index.ts', }); }); - - it('does not inject into routes included in `excludeServerRoutes`', async () => { - const nextConfigWithExcludedRoutes = { - ...exportedNextConfig, - sentry: { - excludeServerRoutes: [/simulator/], - }, - }; - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig: nextConfigWithExcludedRoutes, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/simulator/leaderboard': { - import: expect.not.arrayContaining([serverConfigFilePath]), - }, - 'pages/api/simulator/dogStats/[name]': { - import: expect.not.arrayContaining([serverConfigFilePath]), - }, - }), - ); - }); }); }); From da2487eec0466b8db5cada2575e6f53297a7dcf6 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Wed, 21 Jun 2023 11:44:52 -0700 Subject: [PATCH 07/20] fix(replay): Mark ui.slowClickDetected `clickCount` as optional (#8376) This field was added as part of v7.56.0, but `SlowClickFrameData` was created before that. Having `clickCount` be required breaks backwards compatibility, it should be optional. --- packages/replay/src/types/replayFrame.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/replay/src/types/replayFrame.ts b/packages/replay/src/types/replayFrame.ts index 379dbab91605..f3fb594829d3 100644 --- a/packages/replay/src/types/replayFrame.ts +++ b/packages/replay/src/types/replayFrame.ts @@ -91,7 +91,7 @@ interface SlowClickFrameData extends ClickFrameData { route?: string; timeAfterClickMs: number; endReason: string; - clickCount: number; + clickCount?: number; } export interface SlowClickFrame extends BaseBreadcrumbFrame { category: 'ui.slowClickDetected'; From ee4e37ef4dc2549daa8accfcd4a4a00221f8f4ea Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 22 Jun 2023 13:30:48 +0200 Subject: [PATCH 08/20] fix(sveltekit): Only instrument SvelteKit `fetch` if the SDK client is valid (#8381) In the client-side SvelteKit fetch instrumentation, our previous type cast when retrieving the SDK client was wrong, causing us to not guard the fetch instrumentation correctly if the client was undefined. This fix adds an undefined check. fixes #8290 --- packages/sveltekit/src/client/load.ts | 25 +++++++++++---- packages/sveltekit/test/client/load.test.ts | 34 ++++++++++++++++++--- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index 1996523346e2..bbc184b6d3a0 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -3,7 +3,7 @@ import type { BaseClient } from '@sentry/core'; import { getCurrentHub, trace } from '@sentry/core'; import type { Breadcrumbs, BrowserTracing } from '@sentry/svelte'; import { captureException } from '@sentry/svelte'; -import type { ClientOptions, SanitizedRequestData } from '@sentry/types'; +import type { Client, ClientOptions, SanitizedRequestData } from '@sentry/types'; import { addExceptionMechanism, addNonEnumerableProperty, @@ -122,12 +122,14 @@ type SvelteKitFetch = LoadEvent['fetch']; * @returns a proxy of SvelteKit's fetch implementation */ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch { - const client = getCurrentHub().getClient() as BaseClient; + const client = getCurrentHub().getClient(); - const browserTracingIntegration = - client.getIntegrationById && (client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined); - const breadcrumbsIntegration = - client.getIntegrationById && (client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined); + if (!isValidClient(client)) { + return originalFetch; + } + + const browserTracingIntegration = client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined; + const breadcrumbsIntegration = client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined; const browserTracingOptions = browserTracingIntegration && browserTracingIntegration.options; @@ -270,3 +272,14 @@ function addFetchBreadcrumb( }, ); } + +type MaybeClientWithGetIntegrationsById = + | (Client & { getIntegrationById?: BaseClient['getIntegrationById'] }) + | undefined; + +type ClientWithGetIntegrationById = Required & + Exclude; + +function isValidClient(client: MaybeClientWithGetIntegrationsById): client is ClientWithGetIntegrationById { + return !!client && typeof client.getIntegrationById === 'function'; +} diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index b07e0c12108f..65b4cc1da3b1 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -52,6 +52,12 @@ const mockedGetIntegrationById = vi.fn(id => { return undefined; }); +const mockedGetClient = vi.fn(() => { + return { + getIntegrationById: mockedGetIntegrationById, + }; +}); + vi.mock('@sentry/core', async () => { const original = (await vi.importActual('@sentry/core')) as any; return { @@ -62,11 +68,7 @@ vi.mock('@sentry/core', async () => { }, getCurrentHub: () => { return { - getClient: () => { - return { - getIntegrationById: mockedGetIntegrationById, - }; - }, + getClient: mockedGetClient, getScope: () => { return { getSpan: () => { @@ -427,6 +429,28 @@ describe('wrapLoadWithSentry', () => { }); }); + it.each([ + ['is undefined', undefined], + ["doesn't have a `getClientById` method", {}], + ])("doesn't instrument fetch if the client %s", async (_, client) => { + // @ts-expect-error: we're mocking the client + mockedGetClient.mockImplementationOnce(() => client); + + async function load(_event: Parameters[0]): Promise> { + return { + msg: 'hi', + }; + } + const wrappedLoad = wrapLoadWithSentry(load); + + const originalFetch = MOCK_LOAD_ARGS.fetch; + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(MOCK_LOAD_ARGS.fetch).toStrictEqual(originalFetch); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); + it('adds an exception mechanism', async () => { const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { void callback({}, { event_id: 'fake-event-id' }); From 54e091e7004c6b79cd94acc2d8a32c65632dbbd6 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 22 Jun 2023 14:37:44 +0200 Subject: [PATCH 09/20] fix(serverless): Export `autoDiscoverNodePerformanceMonitoringIntegrations` from SDK (#8382) When refactoring our `@sentry/tracing` package and exporting tracing related integrations from SDK packages, we apparently forgot to export `autoDiscoverNodePerformanceMonitoringIntegrations` from the Serverless SDK. This fix re-adds the export. --- packages/serverless/src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/serverless/src/index.ts b/packages/serverless/src/index.ts index 284aef1331af..f6ae3a2b9184 100644 --- a/packages/serverless/src/index.ts +++ b/packages/serverless/src/index.ts @@ -15,6 +15,7 @@ export { Scope, addBreadcrumb, addGlobalEventProcessor, + autoDiscoverNodePerformanceMonitoringIntegrations, captureEvent, captureException, captureMessage, From 9fd50169b5286c56340e08b7d075c15baae01833 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 22 Jun 2023 17:04:24 +0100 Subject: [PATCH 10/20] test(react): Add tests for `react-router` `createHashRouter` usage. (#8362) --- .../react-create-hash-router/.gitignore | 29 ++ .../react-create-hash-router/.npmrc | 2 + .../react-create-hash-router/package.json | 53 ++++ .../playwright.config.ts | 70 +++++ .../public/index.html | 24 ++ .../react-create-hash-router/src/globals.d.ts | 5 + .../react-create-hash-router/src/index.tsx | 78 ++++++ .../src/pages/Index.tsx | 24 ++ .../src/pages/User.tsx | 7 + .../src/react-app-env.d.ts | 1 + .../react-create-hash-router/test-recipe.json | 19 ++ .../tests/behaviour-test.spec.ts | 256 ++++++++++++++++++ .../tests/fixtures/ReplayRecordingData.ts | 243 +++++++++++++++++ .../react-create-hash-router/tsconfig.json | 20 ++ 14 files changed, 831 insertions(+) create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/.gitignore create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/.npmrc create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/package.json create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/playwright.config.ts create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/public/index.html create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/src/globals.d.ts create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/src/index.tsx create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/src/pages/Index.tsx create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/src/pages/User.tsx create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/src/react-app-env.d.ts create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/test-recipe.json create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/tests/behaviour-test.spec.ts create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/tests/fixtures/ReplayRecordingData.ts create mode 100644 packages/e2e-tests/test-applications/react-create-hash-router/tsconfig.json diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/.gitignore b/packages/e2e-tests/test-applications/react-create-hash-router/.gitignore new file mode 100644 index 000000000000..84634c973eeb --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/.gitignore @@ -0,0 +1,29 @@ +# See https://help.github.com/articles/ignoring-files/ for more about ignoring files. + +# dependencies +/node_modules +/.pnp +.pnp.js + +# testing +/coverage + +# production +/build + +# misc +.DS_Store +.env.local +.env.development.local +.env.test.local +.env.production.local + +npm-debug.log* +yarn-debug.log* +yarn-error.log* + +/test-results/ +/playwright-report/ +/playwright/.cache/ + +!*.d.ts diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/.npmrc b/packages/e2e-tests/test-applications/react-create-hash-router/.npmrc new file mode 100644 index 000000000000..c6b3ef9b3eaa --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/.npmrc @@ -0,0 +1,2 @@ +@sentry:registry=http://localhost:4873 +@sentry-internal:registry=http://localhost:4873 diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/package.json b/packages/e2e-tests/test-applications/react-create-hash-router/package.json new file mode 100644 index 000000000000..bac46c9562d0 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/package.json @@ -0,0 +1,53 @@ +{ + "name": "react-create-hash-router-test", + "version": "0.1.0", + "private": true, + "dependencies": { + "@sentry/react": "*", + "@testing-library/jest-dom": "5.14.1", + "@testing-library/react": "13.0.0", + "@testing-library/user-event": "13.2.1", + "@types/jest": "27.0.1", + "@types/node": "16.7.13", + "@types/react": "18.0.0", + "@types/react-dom": "18.0.0", + "react": "18.2.0", + "react-dom": "18.2.0", + "react-router-dom": "^6.4.1", + "react-scripts": "5.0.1", + "typescript": "4.4.2", + "web-vitals": "2.1.0" + }, + "scripts": { + "build": "react-scripts build", + "start": "serve -s build", + "test": "playwright test" + }, + "eslintConfig": { + "extends": [ + "react-app", + "react-app/jest" + ] + }, + "browserslist": { + "production": [ + ">0.2%", + "not dead", + "not op_mini all" + ], + "development": [ + "last 1 chrome version", + "last 1 firefox version", + "last 1 safari version" + ] + }, + "devDependencies": { + "@playwright/test": "1.26.1", + "axios": "1.1.2", + "serve": "14.0.1" + }, + "volta": { + "node": "16.19.0", + "yarn": "1.22.19" + } +} diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/playwright.config.ts b/packages/e2e-tests/test-applications/react-create-hash-router/playwright.config.ts new file mode 100644 index 000000000000..a24d7bc1c742 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/playwright.config.ts @@ -0,0 +1,70 @@ +import type { PlaywrightTestConfig } from '@playwright/test'; +import { devices } from '@playwright/test'; + +/** + * See https://playwright.dev/docs/test-configuration. + */ +const config: PlaywrightTestConfig = { + testDir: './tests', + /* Maximum time one test can run for. */ + timeout: 60 * 1000, + expect: { + /** + * Maximum time expect() should wait for the condition to be met. + * For example in `await expect(locator).toHaveText();` + */ + timeout: 5000, + }, + /* Run tests in files in parallel */ + fullyParallel: true, + /* Fail the build on CI if you accidentally left test.only in the source code. */ + forbidOnly: !!process.env.CI, + /* Retry on CI only */ + retries: 0, + /* Opt out of parallel tests on CI. */ + workers: 1, + /* Reporter to use. See https://playwright.dev/docs/test-reporters */ + reporter: 'list', + /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ + use: { + /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ + actionTimeout: 0, + + /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */ + trace: 'on-first-retry', + }, + + /* Configure projects for major browsers */ + projects: [ + { + name: 'chromium', + use: { + ...devices['Desktop Chrome'], + }, + }, + // For now we only test Chrome! + // { + // name: 'firefox', + // use: { + // ...devices['Desktop Firefox'], + // }, + // }, + // { + // name: 'webkit', + // use: { + // ...devices['Desktop Safari'], + // }, + // }, + ], + + /* Run your local dev server before starting the tests */ + webServer: { + command: 'pnpm start', + port: Number(process.env.BASE_PORT) + Number(process.env.PORT_MODULO), + env: { + PORT: String(Number(process.env.BASE_PORT) + Number(process.env.PORT_MODULO)), + }, + }, +}; + +export default config; diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/public/index.html b/packages/e2e-tests/test-applications/react-create-hash-router/public/index.html new file mode 100644 index 000000000000..39da76522bea --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/public/index.html @@ -0,0 +1,24 @@ + + + + + + + + React App + + + +
+ + + diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/src/globals.d.ts b/packages/e2e-tests/test-applications/react-create-hash-router/src/globals.d.ts new file mode 100644 index 000000000000..ffa61ca49acc --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/src/globals.d.ts @@ -0,0 +1,5 @@ +interface Window { + recordedTransactions?: string[]; + capturedExceptionId?: string; + sentryReplayId?: string; +} diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/src/index.tsx b/packages/e2e-tests/test-applications/react-create-hash-router/src/index.tsx new file mode 100644 index 000000000000..aef574bce3c4 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/src/index.tsx @@ -0,0 +1,78 @@ +import React from 'react'; +import ReactDOM from 'react-dom/client'; +import * as Sentry from '@sentry/react'; +import { + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + RouterProvider, + createHashRouter, +} from 'react-router-dom'; +import Index from './pages/Index'; +import User from './pages/User'; + +const replay = new Sentry.Replay(); + +Sentry.init({ + // environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.REACT_APP_E2E_TEST_DSN, + integrations: [ + new Sentry.BrowserTracing({ + routingInstrumentation: Sentry.reactRouterV6Instrumentation( + React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + ), + }), + replay, + ], + // We recommend adjusting this value in production, or using tracesSampler + // for finer control + tracesSampleRate: 1.0, + release: 'e2e-test', + + // Always capture replays, so we can test this properly + replaysSessionSampleRate: 1.0, + replaysOnErrorSampleRate: 0.0, +}); + +Object.defineProperty(window, 'sentryReplayId', { + get() { + return replay['_replay'].session.id; + }, +}); + +Sentry.addGlobalEventProcessor(event => { + if ( + event.type === 'transaction' && + (event.contexts?.trace?.op === 'pageload' || event.contexts?.trace?.op === 'navigation') + ) { + const eventId = event.event_id; + if (eventId) { + window.recordedTransactions = window.recordedTransactions || []; + window.recordedTransactions.push(eventId); + } + } + + return event; +}); + +const sentryCreateHashRouter = Sentry.wrapCreateBrowserRouter(createHashRouter); + +const router = sentryCreateHashRouter([ + { + path: '/', + element: , + }, + { + path: '/user/:id', + element: , + }, +]); + +const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement); + +root.render(); diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/src/pages/Index.tsx b/packages/e2e-tests/test-applications/react-create-hash-router/src/pages/Index.tsx new file mode 100644 index 000000000000..2f683c63ed84 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/src/pages/Index.tsx @@ -0,0 +1,24 @@ +import * as React from 'react'; +import * as Sentry from '@sentry/react'; +import { Link } from 'react-router-dom'; + +const Index = () => { + return ( + <> + { + const eventId = Sentry.captureException(new Error('I am an error!')); + window.capturedExceptionId = eventId; + }} + /> + + navigate + + + ); +}; + +export default Index; diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/src/pages/User.tsx b/packages/e2e-tests/test-applications/react-create-hash-router/src/pages/User.tsx new file mode 100644 index 000000000000..671455a92fff --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/src/pages/User.tsx @@ -0,0 +1,7 @@ +import * as React from 'react'; + +const User = () => { + return

I am a blank page :)

; +}; + +export default User; diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/src/react-app-env.d.ts b/packages/e2e-tests/test-applications/react-create-hash-router/src/react-app-env.d.ts new file mode 100644 index 000000000000..6431bc5fc6b2 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/src/react-app-env.d.ts @@ -0,0 +1 @@ +/// diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/test-recipe.json b/packages/e2e-tests/test-applications/react-create-hash-router/test-recipe.json new file mode 100644 index 000000000000..7955a96ea1d0 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/test-recipe.json @@ -0,0 +1,19 @@ +{ + "$schema": "../../test-recipe-schema.json", + "testApplicationName": "react-create-hash-router", + "buildCommand": "pnpm install && npx playwright install && pnpm build", + "tests": [ + { + "testName": "Playwright tests", + "testCommand": "pnpm test" + } + ], + "canaryVersions": [ + { + "dependencyOverrides": { + "react": "latest", + "react-dom": "latest" + } + } + ] +} diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/tests/behaviour-test.spec.ts b/packages/e2e-tests/test-applications/react-create-hash-router/tests/behaviour-test.spec.ts new file mode 100644 index 000000000000..c57beb61c8db --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/tests/behaviour-test.spec.ts @@ -0,0 +1,256 @@ +import { test, expect } from '@playwright/test'; +import axios, { AxiosError } from 'axios'; +import { ReplayRecordingData } from './fixtures/ReplayRecordingData'; + +const EVENT_POLLING_TIMEOUT = 30_000; + +const authToken = process.env.E2E_TEST_AUTH_TOKEN; +const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG; +const sentryTestProject = process.env.E2E_TEST_SENTRY_TEST_PROJECT; + +test('Sends an exception to Sentry', async ({ page }) => { + await page.goto('/'); + + const exceptionButton = page.locator('id=exception-button'); + await exceptionButton.click(); + + const exceptionIdHandle = await page.waitForFunction(() => window.capturedExceptionId); + const exceptionEventId = await exceptionIdHandle.jsonValue(); + + console.log(`Polling for error eventId: ${exceptionEventId}`); + + await expect + .poll( + async () => { + try { + const response = await axios.get( + `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${exceptionEventId}/`, + { headers: { Authorization: `Bearer ${authToken}` } }, + ); + return response.status; + } catch (e) { + if (e instanceof AxiosError && e.response) { + if (e.response.status !== 404) { + throw e; + } else { + return e.response.status; + } + } else { + throw e; + } + } + }, + { + timeout: EVENT_POLLING_TIMEOUT, + }, + ) + .toBe(200); +}); + +test('Sends a pageload transaction to Sentry', async ({ page }) => { + await page.goto('/'); + + const recordedTransactionsHandle = await page.waitForFunction(() => { + if (window.recordedTransactions && window.recordedTransactions?.length >= 1) { + return window.recordedTransactions; + } else { + return undefined; + } + }); + const recordedTransactionEventIds = await recordedTransactionsHandle.jsonValue(); + + if (recordedTransactionEventIds === undefined) { + throw new Error("Application didn't record any transaction event IDs."); + } + + let hadPageLoadTransaction = false; + + console.log(`Polling for transaction eventIds: ${JSON.stringify(recordedTransactionEventIds)}`); + + await Promise.all( + recordedTransactionEventIds.map(async transactionEventId => { + await expect + .poll( + async () => { + try { + const response = await axios.get( + `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionEventId}/`, + { headers: { Authorization: `Bearer ${authToken}` } }, + ); + + if (response.data.contexts.trace.op === 'pageload') { + expect(response.data.title).toBe('/'); + hadPageLoadTransaction = true; + } + + return response.status; + } catch (e) { + if (e instanceof AxiosError && e.response) { + if (e.response.status !== 404) { + throw e; + } else { + return e.response.status; + } + } else { + throw e; + } + } + }, + { + timeout: EVENT_POLLING_TIMEOUT, + }, + ) + .toBe(200); + }), + ); + + expect(hadPageLoadTransaction).toBe(true); +}); + +test('Sends a navigation transaction to Sentry', async ({ page }) => { + await page.goto('/'); + + // Give pageload transaction time to finish + page.waitForTimeout(4000); + + const linkElement = page.locator('id=navigation'); + await linkElement.click(); + + const recordedTransactionsHandle = await page.waitForFunction(() => { + if (window.recordedTransactions && window.recordedTransactions?.length >= 2) { + return window.recordedTransactions; + } else { + return undefined; + } + }); + const recordedTransactionEventIds = await recordedTransactionsHandle.jsonValue(); + + if (recordedTransactionEventIds === undefined) { + throw new Error("Application didn't record any transaction event IDs."); + } + + let hadPageNavigationTransaction = false; + + console.log(`Polling for transaction eventIds: ${JSON.stringify(recordedTransactionEventIds)}`); + + await Promise.all( + recordedTransactionEventIds.map(async transactionEventId => { + await expect + .poll( + async () => { + try { + const response = await axios.get( + `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionEventId}/`, + { headers: { Authorization: `Bearer ${authToken}` } }, + ); + + if (response.data.contexts.trace.op === 'navigation') { + expect(response.data.title).toBe('/user/:id'); + hadPageNavigationTransaction = true; + } + + return response.status; + } catch (e) { + if (e instanceof AxiosError && e.response) { + if (e.response.status !== 404) { + throw e; + } else { + return e.response.status; + } + } else { + throw e; + } + } + }, + { + timeout: EVENT_POLLING_TIMEOUT, + }, + ) + .toBe(200); + }), + ); + + expect(hadPageNavigationTransaction).toBe(true); +}); + +test('Sends a Replay recording to Sentry', async ({ browser }) => { + const context = await browser.newContext(); + const page = await context.newPage(); + + await page.goto('/'); + + const replayId = await page.waitForFunction(() => { + return window.sentryReplayId; + }); + + // Wait for replay to be sent + + if (replayId === undefined) { + throw new Error("Application didn't set a replayId"); + } + + console.log(`Polling for replay with ID: ${replayId}`); + + await expect + .poll( + async () => { + try { + const response = await axios.get( + `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/replays/${replayId}/`, + { headers: { Authorization: `Bearer ${authToken}` } }, + ); + + return response.status; + } catch (e) { + if (e instanceof AxiosError && e.response) { + if (e.response.status !== 404) { + throw e; + } else { + return e.response.status; + } + } else { + throw e; + } + } + }, + { + timeout: EVENT_POLLING_TIMEOUT, + }, + ) + .toBe(200); + + // now fetch the first recording segment + await expect + .poll( + async () => { + try { + const response = await axios.get( + `https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/replays/${replayId}/recording-segments/?cursor=100%3A0%3A1`, + { headers: { Authorization: `Bearer ${authToken}` } }, + ); + + return { + status: response.status, + data: response.data, + }; + } catch (e) { + if (e instanceof AxiosError && e.response) { + if (e.response.status !== 404) { + throw e; + } else { + return e.response.status; + } + } else { + throw e; + } + } + }, + { + timeout: EVENT_POLLING_TIMEOUT, + }, + ) + .toEqual({ + status: 200, + data: ReplayRecordingData, + }); +}); diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/tests/fixtures/ReplayRecordingData.ts b/packages/e2e-tests/test-applications/react-create-hash-router/tests/fixtures/ReplayRecordingData.ts new file mode 100644 index 000000000000..0da2e1b2e327 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/tests/fixtures/ReplayRecordingData.ts @@ -0,0 +1,243 @@ +import { expect } from '@playwright/test'; + +export const ReplayRecordingData = [ + [ + { + type: 4, + data: { href: expect.stringMatching(/http:\/\/localhost:\d+\//), width: 1280, height: 720 }, + timestamp: expect.any(Number), + }, + { + data: { + payload: { + blockAllMedia: true, + errorSampleRate: 0, + maskAllInputs: true, + maskAllText: true, + networkCaptureBodies: true, + networkDetailHasUrls: false, + networkRequestHasHeaders: true, + networkResponseHasHeaders: true, + sessionSampleRate: 1, + useCompression: false, + useCompressionOption: true, + }, + tag: 'options', + }, + timestamp: expect.any(Number), + type: 5, + }, + { + type: 2, + data: { + node: { + type: 0, + childNodes: [ + { type: 1, name: 'html', publicId: '', systemId: '', id: 2 }, + { + type: 2, + tagName: 'html', + attributes: { lang: 'en' }, + childNodes: [ + { + type: 2, + tagName: 'head', + attributes: {}, + childNodes: [ + { type: 2, tagName: 'meta', attributes: { charset: 'utf-8' }, childNodes: [], id: 5 }, + { + type: 2, + tagName: 'meta', + attributes: { name: 'viewport', content: 'width=device-width,initial-scale=1' }, + childNodes: [], + id: 6, + }, + { + type: 2, + tagName: 'meta', + attributes: { name: 'theme-color', content: '#000000' }, + childNodes: [], + id: 7, + }, + { + type: 2, + tagName: 'title', + attributes: {}, + childNodes: [{ type: 3, textContent: '***** ***', id: 9 }], + id: 8, + }, + ], + id: 4, + }, + { + type: 2, + tagName: 'body', + attributes: {}, + childNodes: [ + { + type: 2, + tagName: 'noscript', + attributes: {}, + childNodes: [{ type: 3, textContent: '*** **** ** ****** ********** ** *** **** ****', id: 12 }], + id: 11, + }, + { type: 2, tagName: 'div', attributes: { id: 'root' }, childNodes: [], id: 13 }, + ], + id: 10, + }, + ], + id: 3, + }, + ], + id: 1, + }, + initialOffset: { left: 0, top: 0 }, + }, + timestamp: expect.any(Number), + }, + { + type: 3, + data: { + source: 0, + texts: [], + attributes: [], + removes: [], + adds: [ + { + parentId: 13, + nextId: null, + node: { + type: 2, + tagName: 'a', + attributes: { id: 'navigation', href: expect.stringMatching(/http:\/\/localhost:\d+\/user\/5/) }, + childNodes: [], + id: 14, + }, + }, + { parentId: 14, nextId: null, node: { type: 3, textContent: '********', id: 15 } }, + { + parentId: 13, + nextId: 14, + node: { + type: 2, + tagName: 'input', + attributes: { type: 'button', id: 'exception-button', value: '******* *********' }, + childNodes: [], + id: 16, + }, + }, + ], + }, + timestamp: expect.any(Number), + }, + { + type: 3, + data: { source: 5, text: 'Capture Exception', isChecked: false, id: 16 }, + timestamp: expect.any(Number), + }, + { + type: 5, + timestamp: expect.any(Number), + data: { + tag: 'performanceSpan', + payload: { + op: 'navigation.navigate', + description: expect.stringMatching(/http:\/\/localhost:\d+\//), + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + data: { + decodedBodySize: expect.any(Number), + encodedBodySize: expect.any(Number), + duration: expect.any(Number), + domInteractive: expect.any(Number), + domContentLoadedEventEnd: expect.any(Number), + domContentLoadedEventStart: expect.any(Number), + loadEventStart: expect.any(Number), + loadEventEnd: expect.any(Number), + domComplete: expect.any(Number), + redirectCount: expect.any(Number), + size: expect.any(Number), + }, + }, + }, + }, + { + type: 5, + timestamp: expect.any(Number), + data: { + tag: 'performanceSpan', + payload: { + op: 'resource.script', + description: expect.stringMatching(/http:\/\/localhost:\d+\/static\/js\/main.(\w+).js/), + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + data: { + decodedBodySize: expect.any(Number), + encodedBodySize: expect.any(Number), + size: expect.any(Number), + }, + }, + }, + }, + { + type: 5, + timestamp: expect.any(Number), + data: { + tag: 'performanceSpan', + payload: { + op: 'largest-contentful-paint', + description: 'largest-contentful-paint', + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + data: { value: expect.any(Number), size: expect.any(Number), nodeId: 16 }, + }, + }, + }, + { + type: 5, + timestamp: expect.any(Number), + data: { + tag: 'performanceSpan', + payload: { + op: 'paint', + description: 'first-paint', + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + }, + }, + }, + { + type: 5, + timestamp: expect.any(Number), + data: { + tag: 'performanceSpan', + payload: { + op: 'paint', + description: 'first-contentful-paint', + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + }, + }, + }, + { + type: 5, + timestamp: expect.any(Number), + data: { + tag: 'performanceSpan', + payload: { + op: 'memory', + description: 'memory', + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + data: { + memory: { + jsHeapSizeLimit: expect.any(Number), + totalJSHeapSize: expect.any(Number), + usedJSHeapSize: expect.any(Number), + }, + }, + }, + }, + }, + ], +]; diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/tsconfig.json b/packages/e2e-tests/test-applications/react-create-hash-router/tsconfig.json new file mode 100644 index 000000000000..c8df41dcf4b5 --- /dev/null +++ b/packages/e2e-tests/test-applications/react-create-hash-router/tsconfig.json @@ -0,0 +1,20 @@ +{ + "compilerOptions": { + "target": "es5", + "lib": ["dom", "dom.iterable", "esnext"], + "allowJs": true, + "skipLibCheck": true, + "esModuleInterop": true, + "allowSyntheticDefaultImports": true, + "strict": true, + "forceConsistentCasingInFileNames": true, + "noFallthroughCasesInSwitch": true, + "module": "esnext", + "moduleResolution": "node", + "resolveJsonModule": true, + "isolatedModules": true, + "noEmit": true, + "jsx": "react" + }, + "include": ["src", "tests"] +} From fc7ef13f359a0e4bb0b96a1762d68ff4ea30c3d5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 23 Jun 2023 10:06:31 +0200 Subject: [PATCH 11/20] fix(tracing): Instrument Prisma client in constructor of integration (#8383) --- .github/workflows/build.yml | 1 + .../src/node/integrations/prisma.ts | 56 +++++++++---------- .../test/integrations/node/prisma.test.ts | 35 +++++------- 3 files changed, 41 insertions(+), 51 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3bc95b171d94..b8e3fcafcb34 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -89,6 +89,7 @@ jobs: - 'scripts/**' - 'packages/core/**' - 'packages/tracing/**' + - 'packages/tracing-internal/**' - 'packages/utils/**' - 'packages/types/**' - 'packages/integrations/**' diff --git a/packages/tracing-internal/src/node/integrations/prisma.ts b/packages/tracing-internal/src/node/integrations/prisma.ts index 20a84aa36aaf..ab6fd1f1ae5b 100644 --- a/packages/tracing-internal/src/node/integrations/prisma.ts +++ b/packages/tracing-internal/src/node/integrations/prisma.ts @@ -1,7 +1,6 @@ -import type { Hub } from '@sentry/core'; -import { trace } from '@sentry/core'; -import type { EventProcessor, Integration } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import { getCurrentHub, trace } from '@sentry/core'; +import type { Integration } from '@sentry/types'; +import { addNonEnumerableProperty, logger } from '@sentry/utils'; import { shouldDisableAutoInstrumentation } from './utils/node-utils'; @@ -36,6 +35,7 @@ type PrismaMiddleware = ( ) => Promise; interface PrismaClient { + _sentryInstrumented?: boolean; $use: (cb: PrismaMiddleware) => void; } @@ -55,17 +55,30 @@ export class Prisma implements Integration { */ public name: string = Prisma.id; - /** - * Prisma ORM Client Instance - */ - private readonly _client?: PrismaClient; - /** * @inheritDoc */ public constructor(options: { client?: unknown } = {}) { - if (isValidPrismaClient(options.client)) { - this._client = options.client; + // We instrument the PrismaClient inside the constructor and not inside `setupOnce` because in some cases of server-side + // bundling (Next.js) multiple Prisma clients can be instantiated, even though users don't intend to. When instrumenting + // in setupOnce we can only ever instrument one client. + // https://github.com/getsentry/sentry-javascript/issues/7216#issuecomment-1602375012 + // In the future we might explore providing a dedicated PrismaClient middleware instead of this hack. + if (isValidPrismaClient(options.client) && !options.client._sentryInstrumented) { + addNonEnumerableProperty(options.client as any, '_sentryInstrumented', true); + + options.client.$use((params, next: (params: PrismaMiddlewareParams) => Promise) => { + if (shouldDisableAutoInstrumentation(getCurrentHub)) { + return next(params); + } + + const action = params.action; + const model = params.model; + return trace( + { name: model ? `${model} ${action}` : action, op: 'db.sql.prisma', data: { 'db.system': 'prisma' } }, + () => next(params), + ); + }); } else { __DEBUG_BUILD__ && logger.warn( @@ -77,24 +90,7 @@ export class Prisma implements Integration { /** * @inheritDoc */ - public setupOnce(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { - if (!this._client) { - __DEBUG_BUILD__ && logger.error('PrismaIntegration is missing a Prisma Client Instance'); - return; - } - - if (shouldDisableAutoInstrumentation(getCurrentHub)) { - __DEBUG_BUILD__ && logger.log('Prisma Integration is skipped because of instrumenter configuration.'); - return; - } - - this._client.$use((params, next: (params: PrismaMiddlewareParams) => Promise) => { - const action = params.action; - const model = params.model; - return trace( - { name: model ? `${model} ${action}` : action, op: 'db.sql.prisma', data: { 'db.system': 'prisma' } }, - () => next(params), - ); - }); + public setupOnce(): void { + // Noop - here for backwards compatibility } } diff --git a/packages/tracing/test/integrations/node/prisma.test.ts b/packages/tracing/test/integrations/node/prisma.test.ts index 3096401ec43a..61c0e5fb07f6 100644 --- a/packages/tracing/test/integrations/node/prisma.test.ts +++ b/packages/tracing/test/integrations/node/prisma.test.ts @@ -1,7 +1,6 @@ /* eslint-disable deprecation/deprecation */ -/* eslint-disable @typescript-eslint/unbound-method */ -import { Hub, Scope } from '@sentry/core'; -import { logger } from '@sentry/utils'; +import * as sentryCore from '@sentry/core'; +import { Hub } from '@sentry/core'; import { Integrations } from '../../../src'; import { getTestClient } from '../../testutils'; @@ -38,21 +37,15 @@ class PrismaClient { } describe('setupOnce', function () { - const Client: PrismaClient = new PrismaClient(); - - beforeAll(() => { - new Integrations.Prisma({ client: Client }).setupOnce( - () => undefined, - () => new Hub(undefined, new Scope()), - ); - }); - beforeEach(() => { mockTrace.mockClear(); + mockTrace.mockReset(); }); it('should add middleware with $use method correctly', done => { - void Client.user.create()?.then(() => { + const prismaClient = new PrismaClient(); + new Integrations.Prisma({ client: prismaClient }); + void prismaClient.user.create()?.then(() => { expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenLastCalledWith( { name: 'user create', op: 'db.sql.prisma', data: { 'db.system': 'prisma' } }, @@ -62,18 +55,18 @@ describe('setupOnce', function () { }); }); - it("doesn't attach when using otel instrumenter", () => { - const loggerLogSpy = jest.spyOn(logger, 'log'); + it("doesn't trace when using otel instrumenter", done => { + const prismaClient = new PrismaClient(); + new Integrations.Prisma({ client: prismaClient }); const client = getTestClient({ instrumenter: 'otel' }); const hub = new Hub(client); - const integration = new Integrations.Prisma({ client: Client }); - integration.setupOnce( - () => {}, - () => hub, - ); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); - expect(loggerLogSpy).toBeCalledWith('Prisma Integration is skipped because of instrumenter configuration.'); + void prismaClient.user.create()?.then(() => { + expect(mockTrace).not.toHaveBeenCalled(); + done(); + }); }); }); From 00641e2a5be791f1ee4e935e30015a6bf00959e7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 23 Jun 2023 13:09:05 +0200 Subject: [PATCH 12/20] ref(replay): More graceful `sessionStorage` check (#8394) It seems in some environments `sessionStorage` my be unset to `null` or similar. To be extra careful, we can guard for existence as well there. Fixes https://github.com/getsentry/sentry-javascript/issues/8392 --- packages/replay/src/session/clearSession.ts | 5 ++--- packages/replay/src/session/fetchSession.ts | 5 ++--- packages/replay/src/session/saveSession.ts | 4 ++-- packages/replay/src/util/hasSessionStorage.ts | 6 ++++++ 4 files changed, 12 insertions(+), 8 deletions(-) create mode 100644 packages/replay/src/util/hasSessionStorage.ts diff --git a/packages/replay/src/session/clearSession.ts b/packages/replay/src/session/clearSession.ts index d084764c2fb9..78f50255e363 100644 --- a/packages/replay/src/session/clearSession.ts +++ b/packages/replay/src/session/clearSession.ts @@ -1,5 +1,6 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../../src/constants'; import type { ReplayContainer } from '../../src/types'; +import { hasSessionStorage } from '../util/hasSessionStorage'; /** * Removes the session from Session Storage and unsets session in replay instance @@ -13,9 +14,7 @@ export function clearSession(replay: ReplayContainer): void { * Deletes a session from storage */ function deleteSession(): void { - const hasSessionStorage = 'sessionStorage' in WINDOW; - - if (!hasSessionStorage) { + if (!hasSessionStorage()) { return; } diff --git a/packages/replay/src/session/fetchSession.ts b/packages/replay/src/session/fetchSession.ts index 4b4b1eccf530..3e89a9cbd049 100644 --- a/packages/replay/src/session/fetchSession.ts +++ b/packages/replay/src/session/fetchSession.ts @@ -1,14 +1,13 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../constants'; import type { Session } from '../types'; +import { hasSessionStorage } from '../util/hasSessionStorage'; import { makeSession } from './Session'; /** * Fetches a session from storage */ export function fetchSession(): Session | null { - const hasSessionStorage = 'sessionStorage' in WINDOW; - - if (!hasSessionStorage) { + if (!hasSessionStorage()) { return null; } diff --git a/packages/replay/src/session/saveSession.ts b/packages/replay/src/session/saveSession.ts index 8f75d0ab50ed..d868fd6ea8a1 100644 --- a/packages/replay/src/session/saveSession.ts +++ b/packages/replay/src/session/saveSession.ts @@ -1,12 +1,12 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../constants'; import type { Session } from '../types'; +import { hasSessionStorage } from '../util/hasSessionStorage'; /** * Save a session to session storage. */ export function saveSession(session: Session): void { - const hasSessionStorage = 'sessionStorage' in WINDOW; - if (!hasSessionStorage) { + if (!hasSessionStorage()) { return; } diff --git a/packages/replay/src/util/hasSessionStorage.ts b/packages/replay/src/util/hasSessionStorage.ts new file mode 100644 index 000000000000..f242df101c25 --- /dev/null +++ b/packages/replay/src/util/hasSessionStorage.ts @@ -0,0 +1,6 @@ +import { WINDOW } from '../constants'; + +/** If sessionStorage is available. */ +export function hasSessionStorage(): boolean { + return 'sessionStorage' in WINDOW && !!WINDOW.sessionStorage; +} From 52174851d7a8fe612268302c6bb8bdf44a31de44 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 23 Jun 2023 14:13:22 +0200 Subject: [PATCH 13/20] feat(browser): Better event name handling for non-Error objects (#8374) This PR adjusts the exception name generation for non-error objects in the browser SDK. ref https://github.com/getsentry/sentry-javascript/issues/7941 --- .../captureException/classInstance/subject.js | 6 ++++ .../captureException/classInstance/test.ts | 21 +++++++++++ .../{empty_obj => emptyObj}/subject.js | 0 .../{empty_obj => emptyObj}/test.ts | 2 +- .../captureException/errorEvent/init.js | 8 +++++ .../captureException/errorEvent/subject.js | 5 +++ .../captureException/errorEvent/test.ts | 25 +++++++++++++ .../captureException/event/subject.js | 1 + .../public-api/captureException/event/test.ts | 21 +++++++++++ .../captureException/plainObject/subject.js | 4 +++ .../captureException/plainObject/test.ts | 21 +++++++++++ .../{simple_error => simpleError}/subject.js | 0 .../{simple_error => simpleError}/test.ts | 0 .../subject.js | 0 .../{undefined_arg => undefinedArg}/test.ts | 0 packages/browser/src/eventbuilder.ts | 36 +++++++++++++++++-- .../test/integration/suites/onerror.js | 4 +-- .../suites/onunhandledrejection.js | 6 ++-- .../browser/test/unit/eventbuilder.test.ts | 19 ++++++++++ .../browser/tsconfig.test-integration.json | 12 +++++++ packages/browser/tsconfig.test.json | 3 +- 21 files changed, 184 insertions(+), 10 deletions(-) create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts rename packages/browser-integration-tests/suites/public-api/captureException/{empty_obj => emptyObj}/subject.js (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{empty_obj => emptyObj}/test.ts (91%) create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/event/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/event/test.ts create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts rename packages/browser-integration-tests/suites/public-api/captureException/{simple_error => simpleError}/subject.js (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{simple_error => simpleError}/test.ts (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{undefined_arg => undefinedArg}/subject.js (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{undefined_arg => undefinedArg}/test.ts (100%) create mode 100644 packages/browser/tsconfig.test-integration.json diff --git a/packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js new file mode 100644 index 000000000000..d2d2b96a87fe --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js @@ -0,0 +1,6 @@ +class MyTestClass { + prop1 = 'value1'; + prop2 = 2; +} + +Sentry.captureException(new MyTestClass()); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts new file mode 100644 index 000000000000..3a8865ec3672 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts @@ -0,0 +1,21 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should capture an POJO', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: 'Object captured as exception with keys: prop1, prop2', + mechanism: { + type: 'generic', + handled: true, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/empty_obj/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/subject.js similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/empty_obj/subject.js rename to packages/browser-integration-tests/suites/public-api/captureException/emptyObj/subject.js diff --git a/packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts similarity index 91% rename from packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts rename to packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts index 6ce86bfe7aeb..fa6b1dcb1562 100644 --- a/packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts +++ b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts @@ -12,7 +12,7 @@ sentryTest('should capture an empty object', async ({ getLocalTestPath, page }) expect(eventData.exception?.values).toHaveLength(1); expect(eventData.exception?.values?.[0]).toMatchObject({ type: 'Error', - value: 'Non-Error exception captured with keys: [object has no keys]', + value: 'Object captured as exception with keys: [object has no keys]', mechanism: { type: 'generic', handled: true, diff --git a/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js new file mode 100644 index 000000000000..3796a084234a --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/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', + defaultIntegrations: false, +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js new file mode 100644 index 000000000000..207f9d1d58f6 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js @@ -0,0 +1,5 @@ +window.addEventListener('error', function (event) { + Sentry.captureException(event); +}); + +window.thisDoesNotExist(); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts new file mode 100644 index 000000000000..dbcaaf24a1cf --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts @@ -0,0 +1,25 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should capture an ErrorEvent', async ({ getLocalTestPath, page, browserName }) => { + // On Firefox, the ErrorEvent has the `error` property and thus is handled separately + if (browserName === 'firefox') { + sentryTest.skip(); + } + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'ErrorEvent', + value: 'Event `ErrorEvent` captured as exception with message `Script error.`', + mechanism: { + type: 'generic', + handled: true, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/event/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/event/subject.js new file mode 100644 index 000000000000..b5855af22829 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/event/subject.js @@ -0,0 +1 @@ +Sentry.captureException(new Event('custom')); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/event/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/event/test.ts new file mode 100644 index 000000000000..65c46a776731 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/event/test.ts @@ -0,0 +1,21 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should capture an Event', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Event', + value: 'Event `Event` (type=custom) captured as exception', + mechanism: { + type: 'generic', + handled: true, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js new file mode 100644 index 000000000000..ea827971bed4 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js @@ -0,0 +1,4 @@ +Sentry.captureException({ + prop1: 'value1', + prop2: 2, +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts new file mode 100644 index 000000000000..e81fe0125906 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts @@ -0,0 +1,21 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should capture an class instance', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: 'Object captured as exception with keys: prop1, prop2', + mechanism: { + type: 'generic', + handled: true, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/simple_error/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/simpleError/subject.js similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/simple_error/subject.js rename to packages/browser-integration-tests/suites/public-api/captureException/simpleError/subject.js diff --git a/packages/browser-integration-tests/suites/public-api/captureException/simple_error/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/simpleError/test.ts similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/simple_error/test.ts rename to packages/browser-integration-tests/suites/public-api/captureException/simpleError/test.ts diff --git a/packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/subject.js similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/subject.js rename to packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/subject.js diff --git a/packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/test.ts similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/test.ts rename to packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/test.ts diff --git a/packages/browser/src/eventbuilder.ts b/packages/browser/src/eventbuilder.ts index 4db58b9b7b43..6acc1080c56f 100644 --- a/packages/browser/src/eventbuilder.ts +++ b/packages/browser/src/eventbuilder.ts @@ -14,6 +14,8 @@ import { resolvedSyncPromise, } from '@sentry/utils'; +type Prototype = { constructor: (...args: unknown[]) => unknown }; + /** * This function creates an exception from a JavaScript Error */ @@ -55,9 +57,7 @@ export function eventFromPlainObject( values: [ { type: isEvent(exception) ? exception.constructor.name : isUnhandledRejection ? 'UnhandledRejection' : 'Error', - value: `Non-Error ${ - isUnhandledRejection ? 'promise rejection' : 'exception' - } captured with keys: ${extractExceptionKeysForMessage(exception)}`, + value: getNonErrorObjectExceptionValue(exception, { isUnhandledRejection }), }, ], }, @@ -283,3 +283,33 @@ export function eventFromString( return event; } + +function getNonErrorObjectExceptionValue( + exception: Record, + { isUnhandledRejection }: { isUnhandledRejection?: boolean }, +): string { + const keys = extractExceptionKeysForMessage(exception); + const captureType = isUnhandledRejection ? 'promise rejection' : 'exception'; + + // Some ErrorEvent instances do not have an `error` property, which is why they are not handled before + // We still want to try to get a decent message for these cases + if (isErrorEvent(exception)) { + return `Event \`ErrorEvent\` captured as ${captureType} with message \`${exception.message}\``; + } + + if (isEvent(exception)) { + const className = getObjectClassName(exception); + return `Event \`${className}\` (type=${exception.type}) captured as ${captureType}`; + } + + return `Object captured as ${captureType} with keys: ${keys}`; +} + +function getObjectClassName(obj: unknown): string | undefined | void { + try { + const prototype: Prototype | null = Object.getPrototypeOf(obj); + return prototype ? prototype.constructor.name : undefined; + } catch (e) { + // ignore errors here + } +} diff --git a/packages/browser/test/integration/suites/onerror.js b/packages/browser/test/integration/suites/onerror.js index d5a7f155d723..3db1a755061a 100644 --- a/packages/browser/test/integration/suites/onerror.js +++ b/packages/browser/test/integration/suites/onerror.js @@ -61,7 +61,7 @@ describe('window.onerror', function () { } else { assert.equal( summary.events[0].exception.values[0].value, - 'Non-Error exception captured with keys: error, somekey' + 'Object captured as exception with keys: error, somekey' ); } assert.equal(summary.events[0].exception.values[0].stacktrace.frames.length, 1); // always 1 because thrown objects can't provide > 1 frame @@ -119,7 +119,7 @@ describe('window.onerror', function () { assert.equal(summary.events[0].exception.values[0].type, 'Error'); assert.equal( summary.events[0].exception.values[0].value, - 'Non-Error exception captured with keys: otherKey, type' + 'Object captured as exception with keys: otherKey, type' ); assert.deepEqual(summary.events[0].extra.__serialized__, { type: 'error', diff --git a/packages/browser/test/integration/suites/onunhandledrejection.js b/packages/browser/test/integration/suites/onunhandledrejection.js index d32708eb88cd..f9095d7c7333 100644 --- a/packages/browser/test/integration/suites/onunhandledrejection.js +++ b/packages/browser/test/integration/suites/onunhandledrejection.js @@ -77,7 +77,7 @@ describe('window.onunhandledrejection', function () { // non-error rejections don't provide stacktraces so we can skip that assertion assert.equal( summary.events[0].exception.values[0].value, - 'Non-Error promise rejection captured with keys: currentTarget, isTrusted, target, type' + 'Event `Event` (type=unhandledrejection) captured as promise rejection' ); assert.equal(summary.events[0].exception.values[0].type, 'Event'); assert.equal(summary.events[0].exception.values[0].mechanism.handled, false); @@ -144,7 +144,7 @@ describe('window.onunhandledrejection', function () { // non-error rejections don't provide stacktraces so we can skip that assertion assert.equal( summary.events[0].exception.values[0].value, - 'Non-Error promise rejection captured with keys: a, b, c' + 'Object captured as promise rejection with keys: a, b, c' ); assert.equal(summary.events[0].exception.values[0].type, 'UnhandledRejection'); assert.equal(summary.events[0].exception.values[0].mechanism.handled, false); @@ -172,7 +172,7 @@ describe('window.onunhandledrejection', function () { // non-error rejections don't provide stacktraces so we can skip that assertion assert.equal( summary.events[0].exception.values[0].value, - 'Non-Error promise rejection captured with keys: a, b, c, d, e' + 'Object captured as promise rejection with keys: a, b, c, d, e' ); assert.equal(summary.events[0].exception.values[0].type, 'UnhandledRejection'); assert.equal(summary.events[0].exception.values[0].mechanism.handled, false); diff --git a/packages/browser/test/unit/eventbuilder.test.ts b/packages/browser/test/unit/eventbuilder.test.ts index ac9b564e99e0..d7a2ab712959 100644 --- a/packages/browser/test/unit/eventbuilder.test.ts +++ b/packages/browser/test/unit/eventbuilder.test.ts @@ -23,6 +23,11 @@ jest.mock('@sentry/core', () => { }; }); +class MyTestClass { + prop1 = 'hello'; + prop2 = 2; +} + afterEach(() => { jest.resetAllMocks(); }); @@ -61,4 +66,18 @@ describe('eventFromPlainObject', () => { }, }); }); + + it.each([ + ['empty object', {}, 'Object captured as exception with keys: [object has no keys]'], + ['pojo', { prop1: 'hello', prop2: 2 }, 'Object captured as exception with keys: prop1, prop2'], + ['Custom Class', new MyTestClass(), 'Object captured as exception with keys: prop1, prop2'], + ['Event', new Event('custom'), 'Event `Event` (type=custom) captured as exception'], + ['MouseEvent', new MouseEvent('click'), 'Event `MouseEvent` (type=click) captured as exception'], + ] as [string, Record, string][])( + 'has correct exception value for %s', + (_name, exception, expected) => { + const actual = eventFromPlainObject(defaultStackParser, exception); + expect(actual.exception?.values?.[0]?.value).toEqual(expected); + }, + ); }); diff --git a/packages/browser/tsconfig.test-integration.json b/packages/browser/tsconfig.test-integration.json new file mode 100644 index 000000000000..06a946007c8b --- /dev/null +++ b/packages/browser/tsconfig.test-integration.json @@ -0,0 +1,12 @@ +{ + "extends": "./tsconfig.json", + + "include": ["test/integration/**/*"], + + "compilerOptions": { + // should include all types from `./tsconfig.json` plus types for all test frameworks used + "types": ["node", "mocha", "chai", "sinon"] + + // other package-specific, test-specific options + } +} diff --git a/packages/browser/tsconfig.test.json b/packages/browser/tsconfig.test.json index 03bd27bbfdda..9bdd2aa76dab 100644 --- a/packages/browser/tsconfig.test.json +++ b/packages/browser/tsconfig.test.json @@ -2,10 +2,11 @@ "extends": "./tsconfig.json", "include": ["test/**/*"], + "exclude": ["test/integration/**/*"], "compilerOptions": { // should include all types from `./tsconfig.json` plus types for all test frameworks used - "types": ["node", "mocha", "chai", "sinon", "jest"] + "types": ["node", "jest"] // other package-specific, test-specific options } From a34581d2893fb96624bac7f023c89c8c79e962c6 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 23 Jun 2023 08:21:41 -0400 Subject: [PATCH 14/20] feat(eslint): Add `no-return-await` rule to eslint config (#8388) Add https://eslint.org/docs/latest/rules/no-return-await to eslint config to remove usage of uneeded async/await calls. This helps reduce microtasks being generated, which can help reduce memory pressure caused by the SDK. The downside of removing `return await` is that stacktraces get slightly worse for async errors that use these methods, as we no longer pause execution on return for the engine to grab context on, but instead just pass through the promise, but I think it's worth it for this to be the default, and for us to opt-in to the better stacktraces if need be. --- packages/browser-integration-tests/utils/helpers.ts | 2 +- packages/browser-integration-tests/utils/replayHelpers.ts | 4 ++-- packages/e2e-tests/test-utils/event-proxy-server.ts | 4 ++-- packages/eslint-config-sdk/src/index.js | 3 +++ .../nextjs/src/client/wrapAppGetInitialPropsWithSentry.ts | 4 ++-- .../src/client/wrapDocumentGetInitialPropsWithSentry.ts | 4 ++-- .../nextjs/src/client/wrapErrorGetInitialPropsWithSentry.ts | 4 ++-- packages/nextjs/src/client/wrapGetInitialPropsWithSentry.ts | 4 ++-- .../nextjs/src/client/wrapGetServerSidePropsWithSentry.ts | 4 ++-- packages/nextjs/src/client/wrapGetStaticPropsWithSentry.ts | 4 ++-- .../nextjs/src/common/devErrorSymbolicationEventProcessor.ts | 2 +- packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts | 4 ++-- packages/nextjs/src/edge/wrapMiddlewareWithSentry.ts | 2 +- packages/nextjs/src/server/wrapApiHandlerWithSentry.ts | 4 ++-- .../src/server/wrapDocumentGetInitialPropsWithSentry.ts | 4 ++-- packages/nextjs/src/server/wrapGetStaticPropsWithSentry.ts | 2 +- packages/replay/src/util/sendReplay.ts | 2 +- packages/replay/test/integration/flush.test.ts | 4 ++-- packages/sveltekit/src/vite/svelteConfig.ts | 2 +- packages/utils/test/buildPolyfills/originals.js | 4 ++-- 20 files changed, 35 insertions(+), 32 deletions(-) diff --git a/packages/browser-integration-tests/utils/helpers.ts b/packages/browser-integration-tests/utils/helpers.ts index 296b2fcdba91..525877e9763f 100644 --- a/packages/browser-integration-tests/utils/helpers.ts +++ b/packages/browser-integration-tests/utils/helpers.ts @@ -268,7 +268,7 @@ async function injectScriptAndGetEvents(page: Page, url: string, scriptPath: str await page.goto(url); await runScriptInSandbox(page, scriptPath); - return await getSentryEvents(page); + return getSentryEvents(page); } export { diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index ec332edea74d..bf070b395cf6 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -132,14 +132,14 @@ export async function waitForReplayRunning(page: Page): Promise { * Note that due to how this works with playwright, this is a POJO copy of replay. * This means that we cannot access any methods on it, and also not mutate it in any way. */ -export async function getReplaySnapshot(page: Page): Promise<{ +export function getReplaySnapshot(page: Page): Promise<{ _isPaused: boolean; _isEnabled: boolean; _context: InternalEventContext; session: Session | undefined; recordingMode: ReplayRecordingMode; }> { - return await page.evaluate(() => { + return page.evaluate(() => { const replayIntegration = (window as unknown as Window & { Replay: { _replay: ReplayContainer } }).Replay; const replay = replayIntegration._replay; diff --git a/packages/e2e-tests/test-utils/event-proxy-server.ts b/packages/e2e-tests/test-utils/event-proxy-server.ts index c61e20d4081d..b32910480f38 100644 --- a/packages/e2e-tests/test-utils/event-proxy-server.ts +++ b/packages/e2e-tests/test-utils/event-proxy-server.ts @@ -243,7 +243,7 @@ async function registerCallbackServerPort(serverName: string, port: string): Pro await writeFile(tmpFilePath, port, { encoding: 'utf8' }); } -async function retrieveCallbackServerPort(serverName: string): Promise { +function retrieveCallbackServerPort(serverName: string): Promise { const tmpFilePath = path.join(os.tmpdir(), `${TEMP_FILE_PREFIX}${serverName}`); - return await readFile(tmpFilePath, 'utf8'); + return readFile(tmpFilePath, 'utf8'); } diff --git a/packages/eslint-config-sdk/src/index.js b/packages/eslint-config-sdk/src/index.js index 05ec68cff509..c070195ed083 100644 --- a/packages/eslint-config-sdk/src/index.js +++ b/packages/eslint-config-sdk/src/index.js @@ -261,5 +261,8 @@ module.exports = { 'array-callback-return': ['error', { allowImplicit: true }], quotes: ['error', 'single', { avoidEscape: true }], + + // Remove uncessary usages of async await to prevent extra micro-tasks + 'no-return-await': 'error', }, }; diff --git a/packages/nextjs/src/client/wrapAppGetInitialPropsWithSentry.ts b/packages/nextjs/src/client/wrapAppGetInitialPropsWithSentry.ts index 8c757be6a3c8..e5f8c40847ff 100644 --- a/packages/nextjs/src/client/wrapAppGetInitialPropsWithSentry.ts +++ b/packages/nextjs/src/client/wrapAppGetInitialPropsWithSentry.ts @@ -8,8 +8,8 @@ type AppGetInitialProps = (typeof App)['getInitialProps']; */ export function wrapAppGetInitialPropsWithSentry(origAppGetInitialProps: AppGetInitialProps): AppGetInitialProps { return new Proxy(origAppGetInitialProps, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { - return await wrappingTarget.apply(thisArg, args); + apply: (wrappingTarget, thisArg, args: Parameters) => { + return wrappingTarget.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/client/wrapDocumentGetInitialPropsWithSentry.ts b/packages/nextjs/src/client/wrapDocumentGetInitialPropsWithSentry.ts index 0af40a1f3f84..20669a0af9f6 100644 --- a/packages/nextjs/src/client/wrapDocumentGetInitialPropsWithSentry.ts +++ b/packages/nextjs/src/client/wrapDocumentGetInitialPropsWithSentry.ts @@ -10,8 +10,8 @@ export function wrapDocumentGetInitialPropsWithSentry( origDocumentGetInitialProps: DocumentGetInitialProps, ): DocumentGetInitialProps { return new Proxy(origDocumentGetInitialProps, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { - return await wrappingTarget.apply(thisArg, args); + apply: (wrappingTarget, thisArg, args: Parameters) => { + return wrappingTarget.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/client/wrapErrorGetInitialPropsWithSentry.ts b/packages/nextjs/src/client/wrapErrorGetInitialPropsWithSentry.ts index 605efa58eff9..ab32a2bf93cc 100644 --- a/packages/nextjs/src/client/wrapErrorGetInitialPropsWithSentry.ts +++ b/packages/nextjs/src/client/wrapErrorGetInitialPropsWithSentry.ts @@ -11,8 +11,8 @@ export function wrapErrorGetInitialPropsWithSentry( origErrorGetInitialProps: ErrorGetInitialProps, ): ErrorGetInitialProps { return new Proxy(origErrorGetInitialProps, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { - return await wrappingTarget.apply(thisArg, args); + apply: (wrappingTarget, thisArg, args: Parameters) => { + return wrappingTarget.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/client/wrapGetInitialPropsWithSentry.ts b/packages/nextjs/src/client/wrapGetInitialPropsWithSentry.ts index 1fbbd8707063..37004f04bc6e 100644 --- a/packages/nextjs/src/client/wrapGetInitialPropsWithSentry.ts +++ b/packages/nextjs/src/client/wrapGetInitialPropsWithSentry.ts @@ -8,8 +8,8 @@ type GetInitialProps = Required['getInitialProps']; */ export function wrapGetInitialPropsWithSentry(origGetInitialProps: GetInitialProps): GetInitialProps { return new Proxy(origGetInitialProps, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { - return await wrappingTarget.apply(thisArg, args); + apply: (wrappingTarget, thisArg, args: Parameters) => { + return wrappingTarget.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/client/wrapGetServerSidePropsWithSentry.ts b/packages/nextjs/src/client/wrapGetServerSidePropsWithSentry.ts index 2235016856f4..50450c053a15 100644 --- a/packages/nextjs/src/client/wrapGetServerSidePropsWithSentry.ts +++ b/packages/nextjs/src/client/wrapGetServerSidePropsWithSentry.ts @@ -6,8 +6,8 @@ import type { GetServerSideProps } from 'next'; */ export function wrapGetServerSidePropsWithSentry(origGetServerSideProps: GetServerSideProps): GetServerSideProps { return new Proxy(origGetServerSideProps, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { - return await wrappingTarget.apply(thisArg, args); + apply: (wrappingTarget, thisArg, args: Parameters) => { + return wrappingTarget.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/client/wrapGetStaticPropsWithSentry.ts b/packages/nextjs/src/client/wrapGetStaticPropsWithSentry.ts index 735a3cd8a936..3b99737bcf20 100644 --- a/packages/nextjs/src/client/wrapGetStaticPropsWithSentry.ts +++ b/packages/nextjs/src/client/wrapGetStaticPropsWithSentry.ts @@ -8,8 +8,8 @@ type Props = { [key: string]: unknown }; */ export function wrapGetStaticPropsWithSentry(origGetStaticProps: GetStaticProps): GetStaticProps { return new Proxy(origGetStaticProps, { - apply: async (wrappingTarget, thisArg, args: Parameters>) => { - return await wrappingTarget.apply(thisArg, args); + apply: (wrappingTarget, thisArg, args: Parameters>) => { + return wrappingTarget.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/common/devErrorSymbolicationEventProcessor.ts b/packages/nextjs/src/common/devErrorSymbolicationEventProcessor.ts index 7e3fd8baae24..d516780c0257 100644 --- a/packages/nextjs/src/common/devErrorSymbolicationEventProcessor.ts +++ b/packages/nextjs/src/common/devErrorSymbolicationEventProcessor.ts @@ -116,7 +116,7 @@ export async function devErrorSymbolicationEventProcessor(event: Event, hint: Ev const frames = stackTraceParser.parse(hint.originalException.stack); const resolvedFrames = await Promise.all( - frames.map(async frame => await resolveStackFrame(frame, hint.originalException as Error)), + frames.map(frame => resolveStackFrame(frame, hint.originalException as Error)), ); if (event.exception?.values?.[0].stacktrace?.frames) { diff --git a/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts index ef228abc40e9..ebfdbf5fdf74 100644 --- a/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts @@ -11,7 +11,7 @@ export function wrapApiHandlerWithSentry( parameterizedRoute: string, ): (...params: Parameters) => Promise> { return new Proxy(handler, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { const req = args[0]; const activeSpan = !!getCurrentHub().getScope()?.getSpan(); @@ -25,7 +25,7 @@ export function wrapApiHandlerWithSentry( mechanismFunctionName: 'wrapApiHandlerWithSentry', }); - return await wrappedHandler.apply(thisArg, args); + return wrappedHandler.apply(thisArg, args); }, }); } diff --git a/packages/nextjs/src/edge/wrapMiddlewareWithSentry.ts b/packages/nextjs/src/edge/wrapMiddlewareWithSentry.ts index 18c16f1a4198..831a50eb8629 100644 --- a/packages/nextjs/src/edge/wrapMiddlewareWithSentry.ts +++ b/packages/nextjs/src/edge/wrapMiddlewareWithSentry.ts @@ -11,7 +11,7 @@ export function wrapMiddlewareWithSentry( middleware: H, ): (...params: Parameters) => Promise> { return new Proxy(middleware, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { return withEdgeWrapping(wrappingTarget, { spanDescription: 'middleware', spanOp: 'middleware.nextjs', diff --git a/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts index 3ebf97d4b614..9aecbd9a6c6b 100644 --- a/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts @@ -26,7 +26,7 @@ import { autoEndTransactionOnResponseEnd, finishTransaction, flushQueue } from ' */ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler { return new Proxy(apiHandler, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { // eslint-disable-next-line deprecation/deprecation return withSentry(wrappingTarget, parameterizedRoute).apply(thisArg, args); }, @@ -49,7 +49,7 @@ export const withSentryAPI = wrapApiHandlerWithSentry; */ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: string): NextApiHandler { return new Proxy(apiHandler, { - apply: async (wrappingTarget, thisArg, args: [AugmentedNextApiRequest, AugmentedNextApiResponse]) => { + apply: (wrappingTarget, thisArg, args: [AugmentedNextApiRequest, AugmentedNextApiResponse]) => { const [req, res] = args; // We're now auto-wrapping API route handlers using `wrapApiHandlerWithSentry` (which uses `withSentry` under the hood), but diff --git a/packages/nextjs/src/server/wrapDocumentGetInitialPropsWithSentry.ts b/packages/nextjs/src/server/wrapDocumentGetInitialPropsWithSentry.ts index e7d0d6eac621..1d821d86dcda 100644 --- a/packages/nextjs/src/server/wrapDocumentGetInitialPropsWithSentry.ts +++ b/packages/nextjs/src/server/wrapDocumentGetInitialPropsWithSentry.ts @@ -19,7 +19,7 @@ export function wrapDocumentGetInitialPropsWithSentry( origDocumentGetInitialProps: DocumentGetInitialProps, ): DocumentGetInitialProps { return new Proxy(origDocumentGetInitialProps, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { if (isBuild()) { return wrappingTarget.apply(thisArg, args); } @@ -41,7 +41,7 @@ export function wrapDocumentGetInitialPropsWithSentry( dataFetchingMethodName: 'getInitialProps', }); - return await tracedGetInitialProps.apply(thisArg, args); + return tracedGetInitialProps.apply(thisArg, args); } else { return errorWrappedGetInitialProps.apply(thisArg, args); } diff --git a/packages/nextjs/src/server/wrapGetStaticPropsWithSentry.ts b/packages/nextjs/src/server/wrapGetStaticPropsWithSentry.ts index d8c7cc8f68ab..78f910dfb0e4 100644 --- a/packages/nextjs/src/server/wrapGetStaticPropsWithSentry.ts +++ b/packages/nextjs/src/server/wrapGetStaticPropsWithSentry.ts @@ -19,7 +19,7 @@ export function wrapGetStaticPropsWithSentry( parameterizedRoute: string, ): GetStaticProps { return new Proxy(origGetStaticPropsa, { - apply: async (wrappingTarget, thisArg, args: Parameters>) => { + apply: (wrappingTarget, thisArg, args: Parameters>) => { if (isBuild()) { return wrappingTarget.apply(thisArg, args); } diff --git a/packages/replay/src/util/sendReplay.ts b/packages/replay/src/util/sendReplay.ts index f10bb223d3d9..5f10011182c6 100644 --- a/packages/replay/src/util/sendReplay.ts +++ b/packages/replay/src/util/sendReplay.ts @@ -57,7 +57,7 @@ export async function sendReplay( // will retry in intervals of 5, 10, 30 retryConfig.interval *= ++retryConfig.count; - return await new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { setTimeout(async () => { try { await sendReplay(replayData, retryConfig); diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index 18c7a86ca188..cf142ae8c45c 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -135,8 +135,8 @@ describe('Integration | flush', () => { it('long first flush enqueues following events', async () => { // Mock this to resolve after 20 seconds so that we can queue up following flushes - mockAddPerformanceEntries.mockImplementationOnce(async () => { - return await new Promise(resolve => setTimeout(resolve, 20000)); + mockAddPerformanceEntries.mockImplementationOnce(() => { + return new Promise(resolve => setTimeout(resolve, 20000)); }); expect(mockAddPerformanceEntries).not.toHaveBeenCalled(); diff --git a/packages/sveltekit/src/vite/svelteConfig.ts b/packages/sveltekit/src/vite/svelteConfig.ts index 07c701e912f1..4e69ad8ef3b0 100644 --- a/packages/sveltekit/src/vite/svelteConfig.ts +++ b/packages/sveltekit/src/vite/svelteConfig.ts @@ -52,7 +52,7 @@ export function getHooksFileName(svelteConfig: Config, hookType: 'client' | 'ser */ export async function getAdapterOutputDir(svelteConfig: Config, adapter: SupportedSvelteKitAdapters): Promise { if (adapter === 'node') { - return await getNodeAdapterOutputDir(svelteConfig); + return getNodeAdapterOutputDir(svelteConfig); } // Auto and Vercel adapters simply use config.kit.outDir diff --git a/packages/utils/test/buildPolyfills/originals.js b/packages/utils/test/buildPolyfills/originals.js index d3dcb22e8082..969591755367 100644 --- a/packages/utils/test/buildPolyfills/originals.js +++ b/packages/utils/test/buildPolyfills/originals.js @@ -2,11 +2,11 @@ // the modified versions do the same thing the originals do. // From Sucrase -export async function _asyncNullishCoalesce(lhs, rhsFn) { +export function _asyncNullishCoalesce(lhs, rhsFn) { if (lhs != null) { return lhs; } else { - return await rhsFn(); + return rhsFn(); } } From c77555e84c0df7abbe9b5806f6e8ea160f2fcaa9 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 23 Jun 2023 09:24:48 -0400 Subject: [PATCH 15/20] ref(replay): Remove circular dep in replay eventBuffer (#8389) When running `yarn build:watch` I found some circular deps: ``` src/eventBuffer/index.ts -> src/eventBuffer/EventBufferArray.ts -> src/eventBuffer/index.ts src/eventBuffer/index.ts -> src/eventBuffer/EventBufferProxy.ts -> src/eventBuffer/EventBufferCompressionWorker.ts -> src/eventBuffer/index.ts ``` This refactors the `EventBufferSizeExceededError` export to remove that. --- packages/replay/src/eventBuffer/EventBufferArray.ts | 2 +- .../src/eventBuffer/EventBufferCompressionWorker.ts | 2 +- packages/replay/src/eventBuffer/error.ts | 8 ++++++++ packages/replay/src/eventBuffer/index.ts | 8 -------- packages/replay/src/util/addEvent.ts | 2 +- .../replay/test/unit/eventBuffer/EventBufferArray.test.ts | 5 +++-- .../unit/eventBuffer/EventBufferCompressionWorker.test.ts | 3 ++- 7 files changed, 16 insertions(+), 14 deletions(-) create mode 100644 packages/replay/src/eventBuffer/error.ts diff --git a/packages/replay/src/eventBuffer/EventBufferArray.ts b/packages/replay/src/eventBuffer/EventBufferArray.ts index a7b363891026..a4a823269ece 100644 --- a/packages/replay/src/eventBuffer/EventBufferArray.ts +++ b/packages/replay/src/eventBuffer/EventBufferArray.ts @@ -1,7 +1,7 @@ import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; import type { AddEventResult, EventBuffer, EventBufferType, RecordingEvent } from '../types'; import { timestampToMs } from '../util/timestampToMs'; -import { EventBufferSizeExceededError } from '.'; +import { EventBufferSizeExceededError } from './error'; /** * A basic event buffer that does not do any compression. diff --git a/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts b/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts index 5b4c0eb4487a..695114ebec77 100644 --- a/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts +++ b/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts @@ -3,7 +3,7 @@ import type { ReplayRecordingData } from '@sentry/types'; import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; import type { AddEventResult, EventBuffer, EventBufferType, RecordingEvent } from '../types'; import { timestampToMs } from '../util/timestampToMs'; -import { EventBufferSizeExceededError } from '.'; +import { EventBufferSizeExceededError } from './error'; import { WorkerHandler } from './WorkerHandler'; /** diff --git a/packages/replay/src/eventBuffer/error.ts b/packages/replay/src/eventBuffer/error.ts new file mode 100644 index 000000000000..1d60388d42d7 --- /dev/null +++ b/packages/replay/src/eventBuffer/error.ts @@ -0,0 +1,8 @@ +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; + +/** This error indicates that the event buffer size exceeded the limit.. */ +export class EventBufferSizeExceededError extends Error { + public constructor() { + super(`Event buffer exceeded maximum size of ${REPLAY_MAX_EVENT_BUFFER_SIZE}.`); + } +} diff --git a/packages/replay/src/eventBuffer/index.ts b/packages/replay/src/eventBuffer/index.ts index fe58b76f3c7b..f0eb83c68243 100644 --- a/packages/replay/src/eventBuffer/index.ts +++ b/packages/replay/src/eventBuffer/index.ts @@ -1,7 +1,6 @@ import { getWorkerURL } from '@sentry-internal/replay-worker'; import { logger } from '@sentry/utils'; -import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; import type { EventBuffer } from '../types'; import { EventBufferArray } from './EventBufferArray'; import { EventBufferProxy } from './EventBufferProxy'; @@ -31,10 +30,3 @@ export function createEventBuffer({ useCompression }: CreateEventBufferParams): __DEBUG_BUILD__ && logger.log('[Replay] Using simple buffer'); return new EventBufferArray(); } - -/** This error indicates that the event buffer size exceeded the limit.. */ -export class EventBufferSizeExceededError extends Error { - public constructor() { - super(`Event buffer exceeded maximum size of ${REPLAY_MAX_EVENT_BUFFER_SIZE}.`); - } -} diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index 79bf4ff2f362..9533c1690dd8 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -2,7 +2,7 @@ import { EventType } from '@sentry-internal/rrweb'; import { getCurrentHub } from '@sentry/core'; import { logger } from '@sentry/utils'; -import { EventBufferSizeExceededError } from '../eventBuffer'; +import { EventBufferSizeExceededError } from '../eventBuffer/error'; import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayFrameEvent } from '../types'; import { timestampToMs } from './timestampToMs'; diff --git a/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts index c7b0a4bd7e90..494d03e9572f 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts @@ -1,6 +1,7 @@ import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; -import { createEventBuffer, EventBufferSizeExceededError } from './../../../src/eventBuffer'; -import { BASE_TIMESTAMP } from './../../index'; +import { createEventBuffer } from '../../../src/eventBuffer'; +import { EventBufferSizeExceededError } from '../../../src/eventBuffer/error'; +import { BASE_TIMESTAMP } from '../../index'; const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; diff --git a/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts index 6c3e5948fac1..cab6855e411d 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts @@ -4,8 +4,9 @@ import pako from 'pako'; import { BASE_TIMESTAMP } from '../..'; 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 { createEventBuffer, EventBufferSizeExceededError } from './../../../src/eventBuffer'; const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; From 98d3916351a3d5849c94328568a48dc2dbe13c34 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 23 Jun 2023 12:22:26 -0400 Subject: [PATCH 16/20] feat(types): Add tracePropagationTargets to top level options (#8395) --- packages/node/src/integrations/http.ts | 8 ++++++++ packages/node/src/types.ts | 20 +------------------- packages/types/src/options.ts | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 5e2ce9e253a2..970c60f75839 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -20,6 +20,12 @@ interface TracingOptions { * requests. If this option is provided, the SDK will match the * request URL of outgoing requests against the items in this * array, and only attach tracing headers if a match was found. + * + * @deprecated Use top level `tracePropagationTargets` option instead. + * ``` + * Sentry.init({ + * tracePropagationTargets: ['api.site.com'], + * }) */ tracePropagationTargets?: TracePropagationTargets; @@ -156,6 +162,7 @@ function _createWrappedRequestMethodFactory( }; const shouldAttachTraceData = (url: string): boolean => { + // eslint-disable-next-line deprecation/deprecation if (tracingOptions?.tracePropagationTargets === undefined) { return true; } @@ -165,6 +172,7 @@ function _createWrappedRequestMethodFactory( return cachedDecision; } + // eslint-disable-next-line deprecation/deprecation const decision = stringMatchesSomePattern(url, tracingOptions.tracePropagationTargets); headersUrlMap.set(url, decision); return decision; diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 3e464d1c6457..b0ecb354dd82 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -1,4 +1,4 @@ -import type { ClientOptions, Options, SamplingContext, TracePropagationTargets } from '@sentry/types'; +import type { ClientOptions, Options, SamplingContext } from '@sentry/types'; import type { NodeTransportOptions } from './transports'; @@ -32,24 +32,6 @@ export interface BaseNodeOptions { */ includeLocalVariables?: boolean; - /** - * List of strings/regex controlling to which outgoing requests - * the SDK will attach tracing headers. - * - * By default the SDK will attach those headers to all outgoing - * requests. If this option is provided, the SDK will match the - * request URL of outgoing requests against the items in this - * array, and only attach tracing headers if a match was found. - * - * @example - * ```js - * Sentry.init({ - * tracePropagationTargets: ['api.site.com'], - * }); - * ``` - */ - tracePropagationTargets?: TracePropagationTargets; - // TODO (v8): Remove this in v8 /** * @deprecated Moved to constructor options of the `Http` and `Undici` integration. diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 0f8a163b2615..60d747136d90 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -5,6 +5,7 @@ import type { Integration } from './integration'; import type { CaptureContext } from './scope'; import type { SdkMetadata } from './sdkmetadata'; import type { StackLineParser, StackParser } from './stacktrace'; +import type { TracePropagationTargets } from './tracing'; import type { SamplingContext } from './transaction'; import type { BaseTransportOptions, Transport } from './transport'; @@ -221,6 +222,24 @@ export interface ClientOptions; + /** + * List of strings/regex controlling to which outgoing requests + * the SDK will attach tracing headers. + * + * By default the SDK will attach those headers to all outgoing + * requests. If this option is provided, the SDK will match the + * request URL of outgoing requests against the items in this + * array, and only attach tracing headers if a match was found. + * + * @example + * ```js + * Sentry.init({ + * tracePropagationTargets: ['api.site.com'], + * }); + * ``` + */ + tracePropagationTargets?: TracePropagationTargets; + /** * Function to compute tracing sample rate dynamically and filter unwanted traces. * From e583fe9db2ee32a52b0e873b5ea89ee7c6a40a76 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 26 Jun 2023 10:46:13 +0200 Subject: [PATCH 17/20] fix(sveltekit): Check for cached requests in client-side fetch instrumentation (#8391) As outlined in https://github.com/getsentry/sentry-javascript/issues/8174#issuecomment-1557042801, our current SvelteKit fetch instrumentation breaks SvelteKit's request caching mechanism. This is problematic as `fetch` requests from universal `load` functions were made again on the client side during hydration although the response was already cached from the initial server-side request. The reason for the cache miss is that in the instrumentation we add our tracing headers to the requests, which lead to a different cache key than the one produced on the server side. This fix vendors in code from [the SvelteKit repo](https://github.com/sveltejs/kit) so that we can perform the same cache lookup in our instrumentation. If the lookup was successful (--> cache hit), we won't attach any headers or create breadcrumbs to 1. let Kit's fetch return the cached response and 2. not add spans for a fetch request that didn't even happen. --- packages/sveltekit/src/client/load.ts | 7 ++ .../src/client/vendor/buildSelector.ts | 57 +++++++++++++ packages/sveltekit/src/client/vendor/hash.ts | 51 ++++++++++++ .../src/client/vendor/lookUpCache.ts | 79 +++++++++++++++++++ packages/sveltekit/test/client/load.test.ts | 7 +- .../test/client/vendor/lookUpCache.test.ts | 45 +++++++++++ packages/sveltekit/test/vitest.setup.ts | 5 ++ 7 files changed, 250 insertions(+), 1 deletion(-) create mode 100644 packages/sveltekit/src/client/vendor/buildSelector.ts create mode 100644 packages/sveltekit/src/client/vendor/hash.ts create mode 100644 packages/sveltekit/src/client/vendor/lookUpCache.ts create mode 100644 packages/sveltekit/test/client/vendor/lookUpCache.test.ts diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index bbc184b6d3a0..e56d33b2e23c 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -17,6 +17,7 @@ import type { LoadEvent } from '@sveltejs/kit'; import type { SentryWrappedFlag } from '../common/utils'; import { isRedirect } from '../common/utils'; +import { isRequestCached } from './vendor/lookUpCache'; type PatchedLoadEvent = LoadEvent & Partial; @@ -153,6 +154,11 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch return new Proxy(originalFetch, { apply: (wrappingTarget, thisArg, args: Parameters) => { const [input, init] = args; + + if (isRequestCached(input, init)) { + return wrappingTarget.apply(thisArg, args); + } + const { url: rawUrl, method } = parseFetchArgs(args); // TODO: extract this to a util function (and use it in breadcrumbs integration as well) @@ -196,6 +202,7 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch patchedInit.headers = headers; } + let fetchPromise: Promise; const patchedFetchArgs = [input, patchedInit]; diff --git a/packages/sveltekit/src/client/vendor/buildSelector.ts b/packages/sveltekit/src/client/vendor/buildSelector.ts new file mode 100644 index 000000000000..9ff0ddebe7c7 --- /dev/null +++ b/packages/sveltekit/src/client/vendor/buildSelector.ts @@ -0,0 +1,57 @@ +/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ + +// Vendored from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js +// with types only changes. + +// The MIT License (MIT) + +// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors) + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: + +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. + +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +import { hash } from './hash'; + +/** + * Build the cache key for a given request + * @param {URL | RequestInfo} resource + * @param {RequestInit} [opts] + */ +export function build_selector(resource: URL | RequestInfo, opts: RequestInit | undefined): string { + const url = JSON.stringify(resource instanceof Request ? resource.url : resource); + + let selector = `script[data-sveltekit-fetched][data-url=${url}]`; + + if (opts?.headers || opts?.body) { + /** @type {import('types').StrictBody[]} */ + const values = []; + + if (opts.headers) { + // @ts-ignore - TS complains but this is a 1:1 copy of the original code and apparently it works + values.push([...new Headers(opts.headers)].join(',')); + } + + if (opts.body && (typeof opts.body === 'string' || ArrayBuffer.isView(opts.body))) { + values.push(opts.body); + } + + selector += `[data-hash="${hash(...values)}"]`; + } + + return selector; +} diff --git a/packages/sveltekit/src/client/vendor/hash.ts b/packages/sveltekit/src/client/vendor/hash.ts new file mode 100644 index 000000000000..1723dac703a6 --- /dev/null +++ b/packages/sveltekit/src/client/vendor/hash.ts @@ -0,0 +1,51 @@ +/* eslint-disable no-bitwise */ + +// Vendored from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/hash.js +// with types only changes. + +// The MIT License (MIT) + +// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors) + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: + +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. + +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +import type { StrictBody } from '@sveltejs/kit/types/internal'; + +/** + * Hash using djb2 + * @param {import('types').StrictBody[]} values + */ +export function hash(...values: StrictBody[]): string { + let hash = 5381; + + for (const value of values) { + if (typeof value === 'string') { + let i = value.length; + while (i) hash = (hash * 33) ^ value.charCodeAt(--i); + } else if (ArrayBuffer.isView(value)) { + const buffer = new Uint8Array(value.buffer, value.byteOffset, value.byteLength); + let i = buffer.length; + while (i) hash = (hash * 33) ^ buffer[--i]; + } else { + throw new TypeError('value must be a string or TypedArray'); + } + } + + return (hash >>> 0).toString(36); +} diff --git a/packages/sveltekit/src/client/vendor/lookUpCache.ts b/packages/sveltekit/src/client/vendor/lookUpCache.ts new file mode 100644 index 000000000000..afcaf676b40d --- /dev/null +++ b/packages/sveltekit/src/client/vendor/lookUpCache.ts @@ -0,0 +1,79 @@ +/* eslint-disable no-bitwise */ + +// Parts of this code are taken from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js +// Attribution given directly in the function code below + +// The MIT License (MIT) + +// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors) + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: + +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. + +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +import { WINDOW } from '@sentry/svelte'; +import { getDomElement } from '@sentry/utils'; + +import { build_selector } from './buildSelector'; + +/** + * Checks if a request is cached by looking for a script tag with the same selector as the constructed selector of the request. + * + * This function is a combination of the cache lookups in sveltekit's internal client-side fetch functions + * - initial_fetch (used during hydration) https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L76 + * - subsequent_fetch (used afterwards) https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L98 + * + * Parts of this function's logic is taken from SvelteKit source code. + * These lines are annotated with attribution in comments above them. + * + * @param input first fetch param + * @param init second fetch param + * @returns true if a cache hit was encountered, false otherwise + */ +export function isRequestCached(input: URL | RequestInfo, init: RequestInit | undefined): boolean { + // build_selector call copied from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L77 + const selector = build_selector(input, init); + + const script = getDomElement(selector); + + if (!script) { + return false; + } + + // If the script has a data-ttl attribute, we check if we're still in the TTL window: + try { + // ttl retrieval taken from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L83-L84 + const ttl = Number(script.getAttribute('data-ttl')) * 1000; + + if (isNaN(ttl)) { + return false; + } + + if (ttl) { + // cache hit determination taken from: https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L105-L106 + return ( + WINDOW.performance.now() < ttl && + ['default', 'force-cache', 'only-if-cached', undefined].includes(init && init.cache) + ); + } + } catch { + return false; + } + + // Otherwise, we check if the script has a content and return true in that case + return !!script.textContent; +} diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 65b4cc1da3b1..07608bb9845a 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -27,6 +27,12 @@ vi.mock('@sentry/svelte', async () => { }; }); +vi.mock('../../src/client/vendor/lookUpCache', () => { + return { + isRequestCached: () => false, + }; +}); + const mockTrace = vi.fn(); const mockedBrowserTracing = { @@ -433,7 +439,6 @@ describe('wrapLoadWithSentry', () => { ['is undefined', undefined], ["doesn't have a `getClientById` method", {}], ])("doesn't instrument fetch if the client %s", async (_, client) => { - // @ts-expect-error: we're mocking the client mockedGetClient.mockImplementationOnce(() => client); async function load(_event: Parameters[0]): Promise> { diff --git a/packages/sveltekit/test/client/vendor/lookUpCache.test.ts b/packages/sveltekit/test/client/vendor/lookUpCache.test.ts new file mode 100644 index 000000000000..29b13494be12 --- /dev/null +++ b/packages/sveltekit/test/client/vendor/lookUpCache.test.ts @@ -0,0 +1,45 @@ +import { JSDOM } from 'jsdom'; +import { vi } from 'vitest'; + +import { isRequestCached } from '../../../src/client/vendor/lookUpCache'; + +globalThis.document = new JSDOM().window.document; + +vi.useFakeTimers().setSystemTime(new Date('2023-06-22')); +vi.spyOn(performance, 'now').mockReturnValue(1000); + +describe('isRequestCached', () => { + it('should return true if a script tag with the same selector as the constructed request selector is found', () => { + globalThis.document.body.innerHTML = + ''; + + expect(isRequestCached('/api/todos/1', undefined)).toBe(true); + }); + + it('should return false if a script with the same selector as the constructed request selector is not found', () => { + globalThis.document.body.innerHTML = ''; + + expect(isRequestCached('/api/todos/1', undefined)).toBe(false); + }); + + it('should return true if a script with the same selector as the constructed request selector is found and its TTL is valid', () => { + globalThis.document.body.innerHTML = + ''; + + expect(isRequestCached('/api/todos/1', undefined)).toBe(true); + }); + + it('should return false if a script with the same selector as the constructed request selector is found and its TTL is expired', () => { + globalThis.document.body.innerHTML = + ''; + + expect(isRequestCached('/api/todos/1', undefined)).toBe(false); + }); + + it("should return false if the TTL is set but can't be parsed as a number", () => { + globalThis.document.body.innerHTML = + ''; + + expect(isRequestCached('/api/todos/1', undefined)).toBe(false); + }); +}); diff --git a/packages/sveltekit/test/vitest.setup.ts b/packages/sveltekit/test/vitest.setup.ts index 48c9b0e33528..af2810a98a96 100644 --- a/packages/sveltekit/test/vitest.setup.ts +++ b/packages/sveltekit/test/vitest.setup.ts @@ -11,3 +11,8 @@ export function setup() { }; }); } + +if (!globalThis.fetch) { + // @ts-ignore - Needed for vitest to work with SvelteKit fetch instrumentation + globalThis.Request = class Request {}; +} From 22b5887411f924058c33d875123fb526ac609f4e Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 26 Jun 2023 08:43:43 -0400 Subject: [PATCH 18/20] ref: Remove undefined checks for `hub.getScope()` (#8401) --- .../nextjs-app-dir/components/transaction-context.tsx | 2 +- packages/ember/addon/index.ts | 2 +- packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts | 2 +- packages/nextjs/test/edge/withSentryAPI.test.ts | 4 ++-- .../express/sentry-trace/baggage-header-out/server.ts | 2 +- .../sentry-trace/baggage-transaction-name/server.ts | 2 +- packages/node/src/handlers.ts | 4 ++-- packages/node/src/integrations/http.ts | 8 ++------ packages/node/src/sdk.ts | 2 +- packages/node/test/handlers.test.ts | 4 ++-- packages/node/test/integrations/http.test.ts | 4 ++-- packages/node/test/integrations/requestdata.test.ts | 2 +- packages/opentelemetry-node/test/spanprocessor.test.ts | 2 +- packages/remix/src/utils/instrumentServer.ts | 2 +- packages/replay/src/util/sendReplayRequest.ts | 2 +- .../test/integration/coreHandlers/handleScope.test.ts | 4 ++-- packages/svelte/src/performance.ts | 4 +--- packages/tracing-internal/src/browser/request.ts | 6 ++---- packages/tracing/test/hub.test.ts | 6 +++--- packages/vue/src/tracing.ts | 3 +-- 20 files changed, 29 insertions(+), 38 deletions(-) diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/components/transaction-context.tsx b/packages/e2e-tests/test-applications/nextjs-app-dir/components/transaction-context.tsx index a357439bee1f..384b06ab5528 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/components/transaction-context.tsx +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/components/transaction-context.tsx @@ -29,7 +29,7 @@ export function TransactionContextProvider({ children }: PropsWithChildren) { transactionActive: false, start: (transactionName: string) => { const t = startTransaction({ name: transactionName }); - getCurrentHub().getScope()?.setSpan(t); + getCurrentHub().getScope().setSpan(t); setTransaction(t); }, } diff --git a/packages/ember/addon/index.ts b/packages/ember/addon/index.ts index cf1f7e2f23b9..2db7ac4192f6 100644 --- a/packages/ember/addon/index.ts +++ b/packages/ember/addon/index.ts @@ -63,7 +63,7 @@ export function InitSentryForEmber(_runtimeConfig?: BrowserOptions) { } export const getActiveTransaction = () => { - return Sentry.getCurrentHub()?.getScope()?.getTransaction(); + return Sentry.getCurrentHub().getScope().getTransaction(); }; export const instrumentRoutePerformance = (BaseRoute: any) => { diff --git a/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts index ebfdbf5fdf74..f903d77f46c4 100644 --- a/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/edge/wrapApiHandlerWithSentry.ts @@ -14,7 +14,7 @@ export function wrapApiHandlerWithSentry( apply: (wrappingTarget, thisArg, args: Parameters) => { const req = args[0]; - const activeSpan = !!getCurrentHub().getScope()?.getSpan(); + const activeSpan = getCurrentHub().getScope().getSpan(); const wrappedHandler = withEdgeWrapping(wrappingTarget, { spanDescription: diff --git a/packages/nextjs/test/edge/withSentryAPI.test.ts b/packages/nextjs/test/edge/withSentryAPI.test.ts index 08a91e0c5e11..a991ecf88e6b 100644 --- a/packages/nextjs/test/edge/withSentryAPI.test.ts +++ b/packages/nextjs/test/edge/withSentryAPI.test.ts @@ -73,7 +73,7 @@ describe('wrapApiHandlerWithSentry', () => { it('should return a function that starts a span on the current transaction with the correct description when there is an active transaction and no request is being passed', async () => { const testTransaction = coreSdk.startTransaction({ name: 'testTransaction' }); - coreSdk.getCurrentHub().getScope()?.setSpan(testTransaction); + coreSdk.getCurrentHub().getScope().setSpan(testTransaction); const startChildSpy = jest.spyOn(testTransaction, 'startChild'); @@ -92,6 +92,6 @@ describe('wrapApiHandlerWithSentry', () => { ); testTransaction.finish(); - coreSdk.getCurrentHub().getScope()?.setSpan(undefined); + coreSdk.getCurrentHub().getScope().setSpan(undefined); }); }); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts index 16f5371bc1ee..f582288f8314 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts @@ -25,7 +25,7 @@ app.use(Sentry.Handlers.tracingHandler()); app.use(cors()); app.get('/test/express', (_req, res) => { - const transaction = Sentry.getCurrentHub().getScope()?.getTransaction(); + const transaction = Sentry.getCurrentHub().getScope().getTransaction(); if (transaction) { transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd'; } diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts index 3d8afeaed2f8..78e24ffa9f10 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts @@ -27,7 +27,7 @@ app.use(Sentry.Handlers.tracingHandler()); app.use(cors()); app.get('/test/express', (_req, res) => { - const transaction = Sentry.getCurrentHub().getScope()?.getTransaction(); + const transaction = Sentry.getCurrentHub().getScope().getTransaction(); if (transaction) { transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd'; transaction.setMetadata({ source: 'route' }); diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 3e10c09eb903..95a4cfe65e38 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -163,7 +163,7 @@ export function requestHandler( // If Scope contains a Single mode Session, it is removed in favor of using Session Aggregates mode const scope = currentHub.getScope(); - if (scope && scope.getSession()) { + if (scope.getSession()) { scope.setSession(); } } @@ -339,7 +339,7 @@ export function trpcMiddleware(options: SentryTrpcMiddlewareOptions = {}) { return function ({ path, type, next, rawInput }: TrpcMiddlewareArguments): T { const hub = getCurrentHub(); const clientOptions = hub.getClient()?.getOptions(); - const sentryTransaction = hub.getScope()?.getTransaction(); + const sentryTransaction = hub.getScope().getTransaction(); if (sentryTransaction) { sentryTransaction.setName(`trpc/${path}`, 'route'); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 970c60f75839..54d761861348 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -192,9 +192,7 @@ function _createWrappedRequestMethodFactory( } let requestSpan: Span | undefined; - let parentSpan: Span | undefined; - - const scope = getCurrentHub().getScope(); + const parentSpan = getCurrentHub().getScope().getSpan(); const method = requestOptions.method || 'GET'; const requestSpanData: SanitizedRequestData = { @@ -210,9 +208,7 @@ function _createWrappedRequestMethodFactory( requestSpanData['http.query'] = requestOptions.search.substring(1); } - if (scope && tracingOptions && shouldCreateSpan(rawRequestUrl)) { - parentSpan = scope.getSpan(); - + if (tracingOptions && shouldCreateSpan(rawRequestUrl)) { if (parentSpan) { requestSpan = parentSpan.startChild({ description: `${method} ${requestSpanData.url}`, diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index d0a02c746247..9e55fd4b5a84 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -276,7 +276,7 @@ function startSessionTracking(): void { // such as calling process.exit() or uncaught exceptions. // Ref: https://nodejs.org/api/process.html#process_event_beforeexit process.on('beforeExit', () => { - const session = hub.getScope()?.getSession(); + const session = hub.getScope().getSession(); const terminalStates: SessionStatus[] = ['exited', 'crashed']; // Only call endSession, if the Session exists on Scope and SessionStatus is not a // Terminal Status i.e. Exited or Crashed because diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 3cf5127ce848..046bdf0bb31f 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -319,7 +319,7 @@ describe('tracingHandler', () => { sentryTracingMiddleware(req, res, next); - const transaction = sentryCore.getCurrentHub().getScope()?.getTransaction(); + const transaction = sentryCore.getCurrentHub().getScope().getTransaction(); expect(transaction).toBeDefined(); expect(transaction).toEqual( @@ -439,7 +439,7 @@ describe('tracingHandler', () => { sentryTracingMiddleware(req, res, next); - const transaction = sentryCore.getCurrentHub().getScope()?.getTransaction(); + const transaction = sentryCore.getCurrentHub().getScope().getTransaction(); expect(transaction?.metadata.request).toEqual(req); }); diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index 6481e9481bf2..3f5a87d15363 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -49,7 +49,7 @@ describe('tracing', () => { ...customContext, }); - hub.getScope()?.setSpan(transaction); + hub.getScope().setSpan(transaction); return transaction; } @@ -266,7 +266,7 @@ describe('tracing', () => { function createTransactionAndPutOnScope(hub: Hub) { addTracingExtensions(); const transaction = hub.startTransaction({ name: 'dogpark' }); - hub.getScope()?.setSpan(transaction); + hub.getScope().setSpan(transaction); return transaction; } diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts index 91d9870f8292..52e20c9d6e4b 100644 --- a/packages/node/test/integrations/requestdata.test.ts +++ b/packages/node/test/integrations/requestdata.test.ts @@ -126,7 +126,7 @@ describe('`RequestData` integration', () => { type GCPHandler = (req: PolymorphicRequest, res: http.ServerResponse) => void; const mockGCPWrapper = (origHandler: GCPHandler, options: Record): GCPHandler => { const wrappedHandler: GCPHandler = (req, res) => { - getCurrentHub().getScope()?.setSDKProcessingMetadata({ + getCurrentHub().getScope().setSDKProcessingMetadata({ request: req, requestDataOptionsFromGCPWrapper: options, }); diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts index 97256bd867a4..cde0c7338d00 100644 --- a/packages/opentelemetry-node/test/spanprocessor.test.ts +++ b/packages/opentelemetry-node/test/spanprocessor.test.ts @@ -114,7 +114,7 @@ describe('SentrySpanProcessor', () => { expect(sentrySpan?.spanId).toEqual(childOtelSpan.spanContext().spanId); expect(sentrySpan?.parentSpanId).toEqual(sentrySpanTransaction?.spanId); - expect(hub.getScope()?.getSpan()).toBeUndefined(); + expect(hub.getScope().getSpan()).toBeUndefined(); child.end(endTime); diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index ef5449067df9..ada328badb78 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -306,7 +306,7 @@ export function startRequestHandlerTransaction( }, }); - hub.getScope()?.setSpan(transaction); + hub.getScope().setSpan(transaction); return transaction; } diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index 65f217f857cd..b6f49c0b9c9a 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -34,7 +34,7 @@ export async function sendReplayRequest({ const transport = client && client.getTransport(); const dsn = client && client.getDsn(); - if (!client || !scope || !transport || !dsn || !session.sampled) { + if (!client || !transport || !dsn || !session.sampled) { return; } diff --git a/packages/replay/test/integration/coreHandlers/handleScope.test.ts b/packages/replay/test/integration/coreHandlers/handleScope.test.ts index d9d30d710a6a..2ccaafdefff7 100644 --- a/packages/replay/test/integration/coreHandlers/handleScope.test.ts +++ b/packages/replay/test/integration/coreHandlers/handleScope.test.ts @@ -23,7 +23,7 @@ describe('Integration | coreHandlers | handleScope', () => { expect(mockHandleScopeListener).toHaveBeenCalledTimes(1); - getCurrentHub().getScope()?.addBreadcrumb({ category: 'console', message: 'testing' }); + getCurrentHub().getScope().addBreadcrumb({ category: 'console', message: 'testing' }); expect(mockHandleScope).toHaveBeenCalledTimes(1); expect(mockHandleScope).toHaveReturnedWith(expect.objectContaining({ category: 'console', message: 'testing' })); @@ -32,7 +32,7 @@ describe('Integration | coreHandlers | handleScope', () => { // This will trigger breadcrumb/scope listener, but handleScope should return // null because breadcrumbs has not changed - getCurrentHub().getScope()?.setUser({ email: 'foo@foo.com' }); + getCurrentHub().getScope().setUser({ email: 'foo@foo.com' }); expect(mockHandleScope).toHaveBeenCalledTimes(1); expect(mockHandleScope).toHaveReturnedWith(null); }); diff --git a/packages/svelte/src/performance.ts b/packages/svelte/src/performance.ts index 5cb9e0254557..8bcafb8d9ed8 100644 --- a/packages/svelte/src/performance.ts +++ b/packages/svelte/src/performance.ts @@ -90,7 +90,5 @@ function recordUpdateSpans(componentName: string, initSpan?: Span): void { } function getActiveTransaction(): Transaction | undefined { - const currentHub = getCurrentHub(); - const scope = currentHub && currentHub.getScope(); - return scope && scope.getTransaction(); + return getCurrentHub().getScope().getTransaction(); } diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 284d8f339435..8045e5feed8a 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -195,8 +195,7 @@ export function fetchCallback( return; } - const currentScope = getCurrentHub().getScope(); - const currentSpan = currentScope && currentScope.getSpan(); + const currentSpan = getCurrentHub().getScope().getSpan(); const activeTransaction = currentSpan && currentSpan.transaction; if (currentSpan && activeTransaction) { @@ -336,8 +335,7 @@ export function xhrCallback( return; } - const currentScope = getCurrentHub().getScope(); - const currentSpan = currentScope && currentScope.getSpan(); + const currentSpan = getCurrentHub().getScope().getSpan(); const activeTransaction = currentSpan && currentSpan.transaction; if (currentSpan && activeTransaction) { diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index d292a231f1b7..044582fcf6e2 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -44,7 +44,7 @@ describe('Hub', () => { scope.setSpan(transaction); }); - expect(hub.getScope()?.getTransaction()).toBe(transaction); + expect(hub.getScope().getTransaction()).toBe(transaction); }); it('should find a transaction which has been set on the scope if sampled = false', () => { @@ -57,7 +57,7 @@ describe('Hub', () => { scope.setSpan(transaction); }); - expect(hub.getScope()?.getTransaction()).toBe(transaction); + expect(hub.getScope().getTransaction()).toBe(transaction); }); it("should not find an open transaction if it's not on the scope", () => { @@ -66,7 +66,7 @@ describe('Hub', () => { makeMain(hub); hub.startTransaction({ name: 'dogpark' }); - expect(hub.getScope()?.getTransaction()).toBeUndefined(); + expect(hub.getScope().getTransaction()).toBeUndefined(); }); }); diff --git a/packages/vue/src/tracing.ts b/packages/vue/src/tracing.ts index 55b7b7304baa..1be68b26b61e 100644 --- a/packages/vue/src/tracing.ts +++ b/packages/vue/src/tracing.ts @@ -30,8 +30,7 @@ const HOOKS: { [key in Operation]: Hook[] } = { /** Grabs active transaction off scope, if any */ export function getActiveTransaction(): Transaction | undefined { - const scope = getCurrentHub().getScope(); - return scope && scope.getTransaction(); + return getCurrentHub().getScope().getTransaction(); } /** Finish top-level span and activity with a debounce configured using `timeout` option */ From 947db1b2fa63e2dac15457b35fc5c7ec6c979e7a Mon Sep 17 00:00:00 2001 From: Jonas Date: Mon, 26 Jun 2023 09:26:41 -0400 Subject: [PATCH 19/20] feat(browser): send profiles in same envelope as transactions (#8375) This change ensures that browser profiles are sent in the same envelope as transactions and enables sourcemap support. --- .../browser/src/profiling/hubextensions.ts | 171 +++------ packages/browser/src/profiling/integration.ts | 92 +++-- .../browser/src/profiling/jsSelfProfiling.ts | 8 - packages/browser/src/profiling/sendProfile.ts | 89 ----- packages/browser/src/profiling/utils.ts | 345 +++++++++++------- .../test/unit/profiling/integration.test.ts | 151 -------- packages/types/src/profiling.ts | 10 +- 7 files changed, 350 insertions(+), 516 deletions(-) delete mode 100644 packages/browser/src/profiling/sendProfile.ts delete mode 100644 packages/browser/test/unit/profiling/integration.test.ts diff --git a/packages/browser/src/profiling/hubextensions.ts b/packages/browser/src/profiling/hubextensions.ts index e2d94a11d33f..1c04eeb68362 100644 --- a/packages/browser/src/profiling/hubextensions.ts +++ b/packages/browser/src/profiling/hubextensions.ts @@ -1,29 +1,17 @@ -import { getCurrentHub, getMainCarrier } from '@sentry/core'; -import type { CustomSamplingContext, Hub, Transaction, TransactionContext } from '@sentry/types'; +/* eslint-disable complexity */ +import { getCurrentHub } from '@sentry/core'; +import type { Transaction } from '@sentry/types'; import { logger, uuid4 } from '@sentry/utils'; import { WINDOW } from '../helpers'; -import type { - JSSelfProfile, - JSSelfProfiler, - JSSelfProfilerConstructor, - ProcessedJSSelfProfile, -} from './jsSelfProfiling'; -import { sendProfile } from './sendProfile'; +import type { JSSelfProfile, JSSelfProfiler, JSSelfProfilerConstructor } from './jsSelfProfiling'; +import { addProfileToMap, isValidSampleRate } from './utils'; -// Max profile duration. -const MAX_PROFILE_DURATION_MS = 30_000; +export const MAX_PROFILE_DURATION_MS = 30_000; // Keep a flag value to avoid re-initializing the profiler constructor. If it fails // once, it will always fail and this allows us to early return. let PROFILING_CONSTRUCTOR_FAILED = false; -// While we experiment, per transaction sampling interval will be more flexible to work with. -type StartTransaction = ( - this: Hub, - transactionContext: TransactionContext, - customSamplingContext?: CustomSamplingContext, -) => Transaction | undefined; - /** * Check if profiler constructor is available. * @param maybeProfiler @@ -55,7 +43,7 @@ export function onProfilingStartRouteTransaction(transaction: Transaction | unde * startProfiling is called after the call to startTransaction in order to avoid our own code from * being profiled. Because of that same reason, stopProfiling is called before the call to stopTransaction. */ -function wrapTransactionWithProfiling(transaction: Transaction): Transaction { +export function wrapTransactionWithProfiling(transaction: Transaction): Transaction { // Feature support check first const JSProfilerConstructor = WINDOW.Profiler; @@ -68,14 +56,6 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { return transaction; } - // profilesSampleRate is multiplied with tracesSampleRate to get the final sampling rate. - if (!transaction.sampled) { - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Transaction is not sampled, skipping profiling'); - } - return transaction; - } - // If constructor failed once, it will always fail, so we can early return. if (PROFILING_CONSTRUCTOR_FAILED) { if (__DEBUG_BUILD__) { @@ -86,21 +66,41 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { const client = getCurrentHub().getClient(); const options = client && client.getOptions(); + if (!options) { + __DEBUG_BUILD__ && logger.log('[Profiling] Profiling disabled, no options found.'); + return transaction; + } - // @ts-ignore not part of the browser options yet - const profilesSampleRate = (options && options.profilesSampleRate) || 0; - if (profilesSampleRate === undefined) { - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Profiling disabled, enable it by setting `profilesSampleRate` option to SDK init call.'); - } + // @ts-ignore profilesSampleRate is not part of the browser options yet + const profilesSampleRate: number | boolean | undefined = options.profilesSampleRate; + + // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The + // only valid values are booleans or numbers between 0 and 1.) + if (!isValidSampleRate(profilesSampleRate)) { + __DEBUG_BUILD__ && logger.warn('[Profiling] Discarding profile because of invalid sample rate.'); return transaction; } + // if the function returned 0 (or false), or if `profileSampleRate` is 0, it's a sign the profile should be dropped + if (!profilesSampleRate) { + __DEBUG_BUILD__ && + logger.log( + '[Profiling] Discarding profile because a negative sampling decision was inherited or profileSampleRate is set to 0', + ); + return transaction; + } + + // Now we roll the dice. Math.random is inclusive of 0, but not of 1, so strict < is safe here. In case sampleRate is + // a boolean, the < comparison will cause it to be automatically cast to 1 if it's true and 0 if it's false. + const sampled = profilesSampleRate === true ? true : Math.random() < profilesSampleRate; // Check if we should sample this profile - if (Math.random() > profilesSampleRate) { - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Skip profiling transaction due to sampling.'); - } + if (!sampled) { + __DEBUG_BUILD__ && + logger.log( + `[Profiling] Discarding profile because it's not included in the random sample (sampling rate = ${Number( + profilesSampleRate, + )})`, + ); return transaction; } @@ -147,19 +147,19 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { // event of an error or user mistake (calling transaction.finish multiple times), it is important that the behavior of onProfileHandler // is idempotent as we do not want any timings or profiles to be overriden by the last call to onProfileHandler. // After the original finish method is called, the event will be reported through the integration and delegated to transport. - let processedProfile: ProcessedJSSelfProfile | null = null; + const processedProfile: JSSelfProfile | null = null; /** * Idempotent handler for profile stop */ - function onProfileHandler(): void { + async function onProfileHandler(): Promise { // Check if the profile exists and return it the behavior has to be idempotent as users may call transaction.finish multiple times. if (!transaction) { - return; + return null; } // Satisfy the type checker, but profiler will always be defined here. if (!profiler) { - return; + return null; } if (processedProfile) { if (__DEBUG_BUILD__) { @@ -169,12 +169,12 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { 'already exists, returning early', ); } - return; + return null; } - profiler + return profiler .stop() - .then((p: JSSelfProfile): void => { + .then((p: JSSelfProfile): null => { if (maxDurationTimeoutID) { WINDOW.clearTimeout(maxDurationTimeoutID); maxDurationTimeoutID = undefined; @@ -192,16 +192,11 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { 'this may indicate an overlapping transaction or a call to stopProfiling with a profile title that was never started', ); } - return; - } - - // If a profile has less than 2 samples, it is not useful and should be discarded. - if (p.samples.length < 2) { - return; + return null; } - processedProfile = { ...p, profile_id: profileId }; - sendProfile(profileId, processedProfile); + addProfileToMap(profileId, p); + return null; }) .catch(error => { if (__DEBUG_BUILD__) { @@ -219,6 +214,7 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { transaction.name || transaction.description, ); } + // If the timeout exceeds, we want to stop profiling, but not finish the transaction void onProfileHandler(); }, MAX_PROFILE_DURATION_MS); @@ -230,73 +226,26 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction { * startProfiling is called after the call to startTransaction in order to avoid our own code from * being profiled. Because of that same reason, stopProfiling is called before the call to stopTransaction. */ - function profilingWrappedTransactionFinish(): Promise { + function profilingWrappedTransactionFinish(): Transaction { if (!transaction) { return originalFinish(); } // onProfileHandler should always return the same profile even if this is called multiple times. // Always call onProfileHandler to ensure stopProfiling is called and the timeout is cleared. - onProfileHandler(); - - // Set profile context - transaction.setContext('profile', { profile_id: profileId }); + void onProfileHandler().then( + () => { + transaction.setContext('profile', { profile_id: profileId }); + originalFinish(); + }, + () => { + // If onProfileHandler fails, we still want to call the original finish method. + originalFinish(); + }, + ); - return originalFinish(); + return transaction; } transaction.finish = profilingWrappedTransactionFinish; return transaction; } - -/** - * Wraps startTransaction with profiling logic. This is done automatically by the profiling integration. - */ -function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: StartTransaction): StartTransaction { - return function wrappedStartTransaction( - this: Hub, - transactionContext: TransactionContext, - customSamplingContext?: CustomSamplingContext, - ): Transaction | undefined { - const transaction: Transaction | undefined = startTransaction.call(this, transactionContext, customSamplingContext); - if (transaction === undefined) { - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Transaction is undefined, skipping profiling'); - } - return transaction; - } - - return wrapTransactionWithProfiling(transaction); - }; -} - -/** - * Patches startTransaction and stopTransaction with profiling logic. - */ -export function addProfilingExtensionMethods(): void { - const carrier = getMainCarrier(); - if (!carrier.__SENTRY__) { - if (__DEBUG_BUILD__) { - logger.log("[Profiling] Can't find main carrier, profiling won't work."); - } - return; - } - carrier.__SENTRY__.extensions = carrier.__SENTRY__.extensions || {}; - - if (!carrier.__SENTRY__.extensions['startTransaction']) { - if (__DEBUG_BUILD__) { - logger.log( - '[Profiling] startTransaction does not exists, profiling will not work. Make sure you import @sentry/tracing package before @sentry/profiling-node as import order matters.', - ); - } - return; - } - - if (__DEBUG_BUILD__) { - logger.log('[Profiling] startTransaction exists, patching it with profiling functionality...'); - } - - carrier.__SENTRY__.extensions['startTransaction'] = __PRIVATE__wrapStartTransactionWithProfiling( - // This is already patched by sentry/tracing, we are going to re-patch it... - carrier.__SENTRY__.extensions['startTransaction'] as StartTransaction, - ); -} diff --git a/packages/browser/src/profiling/integration.ts b/packages/browser/src/profiling/integration.ts index 9a9751c50d61..36fb6432e6df 100644 --- a/packages/browser/src/profiling/integration.ts +++ b/packages/browser/src/profiling/integration.ts @@ -1,8 +1,16 @@ -import type { Event, EventProcessor, Integration } from '@sentry/types'; +import type { EventProcessor, Hub, Integration, Transaction } from '@sentry/types'; +import type { Profile } from '@sentry/types/src/profiling'; import { logger } from '@sentry/utils'; -import { PROFILING_EVENT_CACHE } from './cache'; -import { addProfilingExtensionMethods } from './hubextensions'; +import type { BrowserClient } from './../client'; +import { wrapTransactionWithProfiling } from './hubextensions'; +import type { ProfiledEvent } from './utils'; +import { + addProfilesToEnvelope, + createProfilingEvent, + findProfiledTransactionsFromEnvelope, + PROFILE_MAP, +} from './utils'; /** * Browser profiling integration. Stores any event that has contexts["profile"]["profile_id"] @@ -15,34 +23,66 @@ import { addProfilingExtensionMethods } from './hubextensions'; */ export class BrowserProfilingIntegration implements Integration { public readonly name: string = 'BrowserProfilingIntegration'; + public getCurrentHub?: () => Hub = undefined; /** * @inheritDoc */ - public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { - // Patching the hub to add the extension methods. - // Warning: we have an implicit dependency on import order and we will fail patching if the constructor of - // BrowserProfilingIntegration is called before @sentry/tracing is imported. This is because we need to patch - // the methods of @sentry/tracing which are patched as a side effect of importing @sentry/tracing. - addProfilingExtensionMethods(); - - // Add our event processor - addGlobalEventProcessor(this.handleGlobalEvent.bind(this)); - } + public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { + this.getCurrentHub = getCurrentHub; + const client = this.getCurrentHub().getClient() as BrowserClient; - /** - * @inheritDoc - */ - public handleGlobalEvent(event: Event): Event { - const profileId = event.contexts && event.contexts['profile'] && event.contexts['profile']['profile_id']; - - if (profileId && typeof profileId === 'string') { - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Profiling event found, caching it.'); - } - PROFILING_EVENT_CACHE.add(profileId, event); - } + if (client && typeof client.on === 'function') { + client.on('startTransaction', (transaction: Transaction) => { + wrapTransactionWithProfiling(transaction); + }); + + client.on('beforeEnvelope', (envelope): void => { + // if not profiles are in queue, there is nothing to add to the envelope. + if (!PROFILE_MAP['size']) { + return; + } + + const profiledTransactionEvents = findProfiledTransactionsFromEnvelope(envelope); + if (!profiledTransactionEvents.length) { + return; + } + + const profilesToAddToEnvelope: Profile[] = []; - return event; + for (const profiledTransaction of profiledTransactionEvents) { + const context = profiledTransaction && profiledTransaction.contexts; + const profile_id = context && context['profile'] && (context['profile']['profile_id'] as string); + + if (!profile_id) { + __DEBUG_BUILD__ && + logger.log('[Profiling] cannot find profile for a transaction without a profile context'); + continue; + } + + // Remove the profile from the transaction context before sending, relay will take care of the rest. + if (context && context['profile']) { + delete context.profile; + } + + const profile = PROFILE_MAP.get(profile_id); + if (!profile) { + __DEBUG_BUILD__ && logger.log(`[Profiling] Could not retrieve profile for transaction: ${profile_id}`); + continue; + } + + PROFILE_MAP.delete(profile_id); + const profileEvent = createProfilingEvent(profile_id, profile, profiledTransaction as ProfiledEvent); + + if (profileEvent) { + profilesToAddToEnvelope.push(profileEvent); + } + } + + addProfilesToEnvelope(envelope, profilesToAddToEnvelope); + }); + } else { + logger.warn('[Profiling] Client does not support hooks, profiling will be disabled'); + } } } diff --git a/packages/browser/src/profiling/jsSelfProfiling.ts b/packages/browser/src/profiling/jsSelfProfiling.ts index efa4a0a0a0bc..8dc981d3d5d3 100644 --- a/packages/browser/src/profiling/jsSelfProfiling.ts +++ b/packages/browser/src/profiling/jsSelfProfiling.ts @@ -26,10 +26,6 @@ export type JSSelfProfile = { samples: JSSelfProfileSample[]; }; -export interface ProcessedJSSelfProfile extends JSSelfProfile { - profile_id: string; -} - type BufferFullCallback = (trace: JSSelfProfile) => void; export interface JSSelfProfiler { @@ -49,7 +45,3 @@ declare global { Profiler: typeof JSSelfProfilerConstructor | undefined; } } - -export interface RawThreadCpuProfile extends JSSelfProfile { - profile_id: string; -} diff --git a/packages/browser/src/profiling/sendProfile.ts b/packages/browser/src/profiling/sendProfile.ts deleted file mode 100644 index 83ca990c516e..000000000000 --- a/packages/browser/src/profiling/sendProfile.ts +++ /dev/null @@ -1,89 +0,0 @@ -import { getCurrentHub } from '@sentry/core'; -import { logger } from '@sentry/utils'; - -import { PROFILING_EVENT_CACHE } from './cache'; -import type { ProcessedJSSelfProfile } from './jsSelfProfiling'; -import type { ProfiledEvent } from './utils'; -import { createProfilingEventEnvelope } from './utils'; -/** - * Performs lookup in the event cache and sends the profile to Sentry. - * If the profiled transaction event is found, we use the profiled transaction event and profile - * to construct a profile type envelope and send it to Sentry. - */ -export function sendProfile(profileId: string, profile: ProcessedJSSelfProfile): void { - const event = PROFILING_EVENT_CACHE.get(profileId); - - if (!event) { - // We could not find a corresponding transaction event for this profile. - // Opt to do nothing for now, but in the future we should implement a simple retry mechanism. - if (__DEBUG_BUILD__) { - logger.log("[Profiling] Couldn't find a transaction event for this profile, dropping it."); - } - return; - } - - event.sdkProcessingMetadata = event.sdkProcessingMetadata || {}; - if (event.sdkProcessingMetadata && !event.sdkProcessingMetadata['profile']) { - event.sdkProcessingMetadata['profile'] = profile; - } - - // Client, Dsn and Transport are all required to be able to send the profiling event to Sentry. - // If either of them is not available, we remove the profile from the transaction event. - // and forward it to the next event processor. - const hub = getCurrentHub(); - const client = hub.getClient(); - - if (!client) { - if (__DEBUG_BUILD__) { - logger.log( - '[Profiling] getClient did not return a Client, removing profile from event and forwarding to next event processors.', - ); - } - return; - } - - const dsn = client.getDsn(); - if (!dsn) { - if (__DEBUG_BUILD__) { - logger.log( - '[Profiling] getDsn did not return a Dsn, removing profile from event and forwarding to next event processors.', - ); - } - return; - } - - const transport = client.getTransport(); - if (!transport) { - if (__DEBUG_BUILD__) { - logger.log( - '[Profiling] getTransport did not return a Transport, removing profile from event and forwarding to next event processors.', - ); - } - return; - } - - // If all required components are available, we construct a profiling event envelope and send it to Sentry. - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Preparing envelope and sending a profiling event'); - } - const envelope = createProfilingEventEnvelope(event as ProfiledEvent, dsn); - - // Evict event from the cache - we want to prevent the LRU cache from prioritizing already sent events over new ones. - PROFILING_EVENT_CACHE.delete(profileId); - - if (!envelope) { - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Failed to construct envelope'); - } - return; - } - - if (__DEBUG_BUILD__) { - logger.log('[Profiling] Envelope constructed, sending it'); - } - - // Wrap in try/catch because send will throw in case of a network error. - transport.send(envelope).then(null, reason => { - __DEBUG_BUILD__ && logger.log('[Profiling] Error while sending event:', reason); - }); -} diff --git a/packages/browser/src/profiling/utils.ts b/packages/browser/src/profiling/utils.ts index 7b2e1b60e848..6c9b4d8ed6b9 100644 --- a/packages/browser/src/profiling/utils.ts +++ b/packages/browser/src/profiling/utils.ts @@ -1,20 +1,12 @@ -import { DEFAULT_ENVIRONMENT } from '@sentry/core'; -import type { - DsnComponents, - DynamicSamplingContext, - Event, - EventEnvelope, - EventEnvelopeHeaders, - EventItem, - Profile as SentryProfile, - SdkInfo, - SdkMetadata, - ThreadCpuProfile, -} from '@sentry/types'; -import { createEnvelope, dropUndefinedKeys, dsnToString, logger, uuid4 } from '@sentry/utils'; +/* eslint-disable max-lines */ + +import { DEFAULT_ENVIRONMENT, getCurrentHub } from '@sentry/core'; +import type { DebugImage, Envelope, Event, StackFrame, StackParser } from '@sentry/types'; +import type { Profile, ThreadCpuProfile } from '@sentry/types/src/profiling'; +import { forEachEnvelopeItem, GLOBAL_OBJ, logger, uuid4 } from '@sentry/utils'; import { WINDOW } from '../helpers'; -import type { JSSelfProfile, JSSelfProfileStack, RawThreadCpuProfile } from './jsSelfProfiling'; +import type { JSSelfProfile, JSSelfProfileStack } from './jsSelfProfiling'; const MS_TO_NS = 1e6; // Use 0 as main thread id which is identical to threadId in node:worker_threads @@ -23,9 +15,9 @@ const THREAD_ID_STRING = String(0); const THREAD_NAME = 'main'; // Machine properties (eval only once) -let OS_PLATFORM = ''; // macos -let OS_PLATFORM_VERSION = ''; // 13.2 -let OS_ARCH = ''; // arm64 +let OS_PLATFORM = ''; +let OS_PLATFORM_VERSION = ''; +let OS_ARCH = ''; let OS_BROWSER = (WINDOW.navigator && WINDOW.navigator.userAgent) || ''; let OS_MODEL = ''; const OS_LOCALE = @@ -72,7 +64,7 @@ if (isUserAgentData(userAgentData)) { .catch(e => void e); } -function isRawThreadCpuProfile(profile: ThreadCpuProfile | RawThreadCpuProfile): profile is RawThreadCpuProfile { +function isProcessedJSSelfProfile(profile: ThreadCpuProfile | JSSelfProfile): profile is JSSelfProfile { return !('thread_metadata' in profile); } @@ -81,8 +73,8 @@ function isRawThreadCpuProfile(profile: ThreadCpuProfile | RawThreadCpuProfile): /** * */ -export function enrichWithThreadInformation(profile: ThreadCpuProfile | RawThreadCpuProfile): ThreadCpuProfile { - if (!isRawThreadCpuProfile(profile)) { +export function enrichWithThreadInformation(profile: ThreadCpuProfile | JSSelfProfile): ThreadCpuProfile { + if (!isProcessedJSSelfProfile(profile)) { return profile; } @@ -93,52 +85,7 @@ export function enrichWithThreadInformation(profile: ThreadCpuProfile | RawThrea // by the integration before the event is processed by other integrations. export interface ProfiledEvent extends Event { sdkProcessingMetadata: { - profile?: RawThreadCpuProfile; - }; -} - -/** Extract sdk info from from the API metadata */ -function getSdkMetadataForEnvelopeHeader(metadata?: SdkMetadata): SdkInfo | undefined { - if (!metadata || !metadata.sdk) { - return undefined; - } - - return { name: metadata.sdk.name, version: metadata.sdk.version } as SdkInfo; -} - -/** - * Apply SdkInfo (name, version, packages, integrations) to the corresponding event key. - * Merge with existing data if any. - **/ -function enhanceEventWithSdkInfo(event: Event, sdkInfo?: SdkInfo): Event { - if (!sdkInfo) { - return event; - } - event.sdk = event.sdk || {}; - event.sdk.name = event.sdk.name || sdkInfo.name || 'unknown sdk'; - event.sdk.version = event.sdk.version || sdkInfo.version || 'unknown sdk version'; - event.sdk.integrations = [...(event.sdk.integrations || []), ...(sdkInfo.integrations || [])]; - event.sdk.packages = [...(event.sdk.packages || []), ...(sdkInfo.packages || [])]; - return event; -} - -function createEventEnvelopeHeaders( - event: Event, - sdkInfo: SdkInfo | undefined, - tunnel: string | undefined, - dsn: DsnComponents, -): EventEnvelopeHeaders { - const dynamicSamplingContext = event.sdkProcessingMetadata && event.sdkProcessingMetadata['dynamicSamplingContext']; - - return { - event_id: event.event_id as string, - sent_at: new Date().toISOString(), - ...(sdkInfo && { sdk: sdkInfo }), - ...(!!tunnel && { dsn: dsnToString(dsn) }), - ...(event.type === 'transaction' && - dynamicSamplingContext && { - trace: dropUndefinedKeys({ ...dynamicSamplingContext }) as DynamicSamplingContext, - }), + profile?: JSSelfProfile; }; } @@ -171,50 +118,30 @@ function getTraceId(event: Event): string { /** * Creates a profiling event envelope from a Sentry event. */ -export function createProfilingEventEnvelope( +export function createProfilePayload( event: ProfiledEvent, - dsn: DsnComponents, - metadata?: SdkMetadata, - tunnel?: string, -): EventEnvelope | null { + processedProfile: JSSelfProfile, + profile_id: string, +): Profile { if (event.type !== 'transaction') { // createProfilingEventEnvelope should only be called for transactions, // we type guard this behavior with isProfiledTransactionEvent. throw new TypeError('Profiling events may only be attached to transactions, this should never occur.'); } - const rawProfile = event.sdkProcessingMetadata['profile']; - - if (rawProfile === undefined || rawProfile === null) { + if (processedProfile === undefined || processedProfile === null) { throw new TypeError( - `Cannot construct profiling event envelope without a valid profile. Got ${rawProfile} instead.`, + `Cannot construct profiling event envelope without a valid profile. Got ${processedProfile} instead.`, ); } - if (!rawProfile.profile_id) { - throw new TypeError('Profile is missing profile_id'); - } - - if (rawProfile.samples.length <= 1) { - if (__DEBUG_BUILD__) { - // Log a warning if the profile has less than 2 samples so users can know why - // they are not seeing any profiling data and we cant avoid the back and forth - // of asking them to provide us with a dump of the profile data. - logger.log('[Profiling] Discarding profile because it contains less than 2 samples'); - } - return null; - } - const traceId = getTraceId(event); - const sdkInfo = getSdkMetadataForEnvelopeHeader(metadata); - enhanceEventWithSdkInfo(event, metadata && metadata.sdk); - const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn); - const enrichedThreadProfile = enrichWithThreadInformation(rawProfile); + const enrichedThreadProfile = enrichWithThreadInformation(processedProfile); const transactionStartMs = typeof event.start_timestamp === 'number' ? event.start_timestamp * 1000 : Date.now(); const transactionEndMs = typeof event.timestamp === 'number' ? event.timestamp * 1000 : Date.now(); - const profile: SentryProfile = { - event_id: rawProfile.profile_id, + const profile: Profile = { + event_id: profile_id, timestamp: new Date(transactionStartMs).toISOString(), platform: 'javascript', version: '1', @@ -236,6 +163,9 @@ export function createProfilingEventEnvelope( architecture: OS_ARCH, is_emulator: false, }, + debug_meta: { + images: applyDebugMetadata(processedProfile.resources), + }, profile: enrichedThreadProfile, transactions: [ { @@ -249,15 +179,7 @@ export function createProfilingEventEnvelope( ], }; - const envelopeItem: EventItem = [ - { - type: 'profile', - }, - // @ts-ignore this is missing in typedef - profile, - ]; - - return createEnvelope(envelopeHeaders, [envelopeItem]); + return profile; } /** @@ -267,31 +189,16 @@ export function isProfiledTransactionEvent(event: Event): event is ProfiledEvent return !!(event.sdkProcessingMetadata && event.sdkProcessingMetadata['profile']); } -// Due to how profiles are attached to event metadata, we may sometimes want to remove them to ensure -// they are not processed by other Sentry integrations. This can be the case when we cannot construct a valid -// profile from the data we have or some of the mechanisms to send the event (Hub, Transport etc) are not available to us. -/** - * - */ -export function maybeRemoveProfileFromSdkMetadata(event: Event | ProfiledEvent): Event { - if (!isProfiledTransactionEvent(event)) { - return event; - } - - delete event.sdkProcessingMetadata.profile; - return event; -} - /** * Converts a JSSelfProfile to a our sampled format. * Does not currently perform stack indexing. */ -export function convertJSSelfProfileToSampledFormat(input: JSSelfProfile): ThreadCpuProfile { +export function convertJSSelfProfileToSampledFormat(input: JSSelfProfile): Profile['profile'] { let EMPTY_STACK_ID: undefined | number = undefined; let STACK_ID = 0; // Initialize the profile that we will fill with data - const profile: ThreadCpuProfile = { + const profile: Profile['profile'] = { samples: [], stacks: [], frames: [], @@ -351,7 +258,7 @@ export function convertJSSelfProfileToSampledFormat(input: JSSelfProfile): Threa stackTop = stackTop.parentId === undefined ? undefined : input.stacks[stackTop.parentId]; } - const sample: ThreadCpuProfile['samples'][0] = { + const sample: Profile['profile']['samples'][0] = { // convert ms timestamp to ns elapsed_since_start_ns: ((jsSample.timestamp - start) * MS_TO_NS).toFixed(0), stack_id: STACK_ID, @@ -365,3 +272,195 @@ export function convertJSSelfProfileToSampledFormat(input: JSSelfProfile): Threa return profile; } + +/** + * Adds items to envelope if they are not already present - mutates the envelope. + * @param envelope + */ +export function addProfilesToEnvelope(envelope: Envelope, profiles: Profile[]): Envelope { + if (!profiles.length) { + return envelope; + } + + for (const profile of profiles) { + // @ts-ignore untyped envelope + envelope[1].push([{ type: 'profile' }, profile]); + } + return envelope; +} + +/** + * Finds transactions with profile_id context in the envelope + * @param envelope + * @returns + */ +export function findProfiledTransactionsFromEnvelope(envelope: Envelope): Event[] { + const events: Event[] = []; + + forEachEnvelopeItem(envelope, (item, type) => { + if (type !== 'transaction') { + return; + } + + for (let j = 1; j < item.length; j++) { + const event = item[j] as Event; + + if (event && event.contexts && event.contexts['profile'] && event.contexts['profile']['profile_id']) { + events.push(item[j] as Event); + } + } + }); + + return events; +} + +const debugIdStackParserCache = new WeakMap>(); +/** + * Applies debug meta data to an event from a list of paths to resources (sourcemaps) + */ +export function applyDebugMetadata(resource_paths: ReadonlyArray): DebugImage[] { + const debugIdMap = GLOBAL_OBJ._sentryDebugIds; + + if (!debugIdMap) { + return []; + } + + const hub = getCurrentHub(); + if (!hub) { + return []; + } + const client = hub.getClient(); + if (!client) { + return []; + } + const options = client.getOptions(); + if (!options) { + return []; + } + const stackParser = options.stackParser; + if (!stackParser) { + return []; + } + + let debugIdStackFramesCache: Map; + const cachedDebugIdStackFrameCache = debugIdStackParserCache.get(stackParser); + if (cachedDebugIdStackFrameCache) { + debugIdStackFramesCache = cachedDebugIdStackFrameCache; + } else { + debugIdStackFramesCache = new Map(); + debugIdStackParserCache.set(stackParser, debugIdStackFramesCache); + } + + // Build a map of filename -> debug_id + const filenameDebugIdMap = Object.keys(debugIdMap).reduce>((acc, debugIdStackTrace) => { + let parsedStack: StackFrame[]; + + const cachedParsedStack = debugIdStackFramesCache.get(debugIdStackTrace); + if (cachedParsedStack) { + parsedStack = cachedParsedStack; + } else { + parsedStack = stackParser(debugIdStackTrace); + debugIdStackFramesCache.set(debugIdStackTrace, parsedStack); + } + + for (let i = parsedStack.length - 1; i >= 0; i--) { + const stackFrame = parsedStack[i]; + const file = stackFrame && stackFrame.filename; + + if (stackFrame && file) { + acc[file] = debugIdMap[debugIdStackTrace] as string; + break; + } + } + return acc; + }, {}); + + const images: DebugImage[] = []; + for (const path of resource_paths) { + if (path && filenameDebugIdMap[path]) { + images.push({ + type: 'sourcemap', + code_file: path, + debug_id: filenameDebugIdMap[path] as string, + }); + } + } + + return images; +} + +/** + * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). + */ +export function isValidSampleRate(rate: unknown): boolean { + // we need to check NaN explicitly because it's of type 'number' and therefore wouldn't get caught by this typecheck + if ((typeof rate !== 'number' && typeof rate !== 'boolean') || (typeof rate === 'number' && isNaN(rate))) { + __DEBUG_BUILD__ && + logger.warn( + `[Profiling] Invalid sample rate. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( + rate, + )} of type ${JSON.stringify(typeof rate)}.`, + ); + return false; + } + + // Boolean sample rates are always valid + if (rate === true || rate === false) { + return true; + } + + // in case sampleRate is a boolean, it will get automatically cast to 1 if it's true and 0 if it's false + if (rate < 0 || rate > 1) { + __DEBUG_BUILD__ && + logger.warn(`[Profiling] Invalid sample rate. Sample rate must be between 0 and 1. Got ${rate}.`); + return false; + } + return true; +} + +function isValidProfile(profile: JSSelfProfile): profile is JSSelfProfile & { profile_id: string } { + if (profile.samples.length < 2) { + if (__DEBUG_BUILD__) { + // Log a warning if the profile has less than 2 samples so users can know why + // they are not seeing any profiling data and we cant avoid the back and forth + // of asking them to provide us with a dump of the profile data. + logger.log('[Profiling] Discarding profile because it contains less than 2 samples'); + } + return false; + } + + if (!profile.frames.length) { + if (__DEBUG_BUILD__) { + logger.log('[Profiling] Discarding profile because it contains no frames'); + } + return false; + } + + return true; +} + +/** + * Creates a profiling envelope item, if the profile does not pass validation, returns null. + * @param event + * @returns {Profile | null} + */ +export function createProfilingEvent(profile_id: string, profile: JSSelfProfile, event: ProfiledEvent): Profile | null { + if (!isValidProfile(profile)) { + return null; + } + + return createProfilePayload(event, profile, profile_id); +} + +export const PROFILE_MAP: Map = new Map(); +/** + * + */ +export function addProfileToMap(profile_id: string, profile: JSSelfProfile): void { + PROFILE_MAP.set(profile_id, profile); + + if (PROFILE_MAP.size > 30) { + const last: string = PROFILE_MAP.keys().next().value; + PROFILE_MAP.delete(last); + } +} diff --git a/packages/browser/test/unit/profiling/integration.test.ts b/packages/browser/test/unit/profiling/integration.test.ts deleted file mode 100644 index 1ea59ee7068e..000000000000 --- a/packages/browser/test/unit/profiling/integration.test.ts +++ /dev/null @@ -1,151 +0,0 @@ -import { getCurrentHub } from '@sentry/browser'; -import type { Event } from '@sentry/types'; -import { TextDecoder, TextEncoder } from 'util'; - -// @ts-ignore patch the encoder on the window, else importing JSDOM fails (deleted in afterAll) -const patchedEncoder = (!global.window.TextEncoder && (global.window.TextEncoder = TextEncoder)) || true; -// @ts-ignore patch the encoder on the window, else importing JSDOM fails (deleted in afterAll) -const patchedDecoder = (!global.window.TextDecoder && (global.window.TextDecoder = TextDecoder)) || true; - -import { JSDOM } from 'jsdom'; - -import { PROFILING_EVENT_CACHE } from '../../../src/profiling/cache'; -import { BrowserProfilingIntegration } from '../../../src/profiling/integration'; -import { sendProfile } from '../../../src/profiling/sendProfile'; - -// @ts-ignore store a reference so we can reset it later -const globalDocument = global.document; -// @ts-ignore store a reference so we can reset it later -const globalWindow = global.window; -// @ts-ignore store a reference so we can reset it later -const globalLocation = global.location; - -describe('BrowserProfilingIntegration', () => { - beforeEach(() => { - // Clear profiling event cache - PROFILING_EVENT_CACHE.clear(); - - const dom = new JSDOM(); - // @ts-ignore need to override global document - global.document = dom.window.document; - // @ts-ignore need to override global document - global.window = dom.window; - // @ts-ignore need to override global document - global.location = dom.window.location; - }); - - // Reset back to previous values - afterEach(() => { - // @ts-ignore need to override global document - global.document = globalDocument; - // @ts-ignore need to override global document - global.window = globalWindow; - // @ts-ignore need to override global document - global.location = globalLocation; - }); - - afterAll(() => { - // @ts-ignore patch the encoder on the window, else importing JSDOM fails - patchedEncoder && delete global.window.TextEncoder; - // @ts-ignore patch the encoder on the window, else importing JSDOM fails - patchedDecoder && delete global.window.TextDecoder; - }); - - it('does not store event in profiling event cache if context["profile"]["profile_id"] is not present', () => { - const integration = new BrowserProfilingIntegration(); - const event: Event = { - contexts: {}, - }; - integration.handleGlobalEvent(event); - expect(PROFILING_EVENT_CACHE.size()).toBe(0); - }); - - it('stores event in profiling event cache if context["profile"]["profile_id"] is present', () => { - const integration = new BrowserProfilingIntegration(); - const event: Event = { - contexts: { - profile: { - profile_id: 'profile_id', - }, - }, - }; - integration.handleGlobalEvent(event); - expect(PROFILING_EVENT_CACHE.get(event.contexts!.profile!.profile_id as string)).toBe(event); - }); - - it('sending profile evicts it from the LRU cache', () => { - const hub = getCurrentHub(); - const client: any = { - getDsn() { - return {}; - }, - getTransport() { - return { - send() {}, - }; - }, - }; - - hub.bindClient(client); - - const integration = new BrowserProfilingIntegration(); - const event: Event = { - type: 'transaction', - contexts: { - profile: { - profile_id: 'profile_id', - }, - }, - }; - - integration.handleGlobalEvent(event); - - sendProfile('profile_id', { - resources: [], - samples: [], - stacks: [], - frames: [], - profile_id: 'profile_id', - }); - - expect(PROFILING_EVENT_CACHE.get('profile_id')).toBe(undefined); - }); -}); - -describe('ProfilingEventCache', () => { - beforeEach(() => { - PROFILING_EVENT_CACHE.clear(); - }); - - it('caps the size of the profiling event cache', () => { - for (let i = 0; i <= 21; i++) { - const integration = new BrowserProfilingIntegration(); - const event: Event = { - contexts: { - profile: { - profile_id: `profile_id_${i}`, - }, - }, - }; - integration.handleGlobalEvent(event); - } - expect(PROFILING_EVENT_CACHE.size()).toBe(20); - // Evicts the first item in the cache - expect(PROFILING_EVENT_CACHE.get('profile_id_0')).toBe(undefined); - }); - - it('handles collision by replacing the value', () => { - PROFILING_EVENT_CACHE.add('profile_id_0', {}); - const second = {}; - PROFILING_EVENT_CACHE.add('profile_id_0', second); - - expect(PROFILING_EVENT_CACHE.get('profile_id_0')).toBe(second); - expect(PROFILING_EVENT_CACHE.size()).toBe(1); - }); - - it('clears cache', () => { - PROFILING_EVENT_CACHE.add('profile_id_0', {}); - PROFILING_EVENT_CACHE.clear(); - expect(PROFILING_EVENT_CACHE.size()).toBe(0); - }); -}); diff --git a/packages/types/src/profiling.ts b/packages/types/src/profiling.ts index 1b6dac566901..d99736df735e 100644 --- a/packages/types/src/profiling.ts +++ b/packages/types/src/profiling.ts @@ -1,3 +1,4 @@ +import type { DebugImage } from './debugMeta'; export type ThreadId = string; export type FrameId = number; export type StackId = number; @@ -50,14 +51,7 @@ export interface Profile { platform: string; profile: ThreadCpuProfile; debug_meta?: { - images: { - debug_id: string; - image_addr: string; - code_file: string; - type: string; - image_size: number; - image_vmaddr: string; - }[]; + images: DebugImage[]; }; transaction?: { name: string; From 0c194462a034b9dbf96ec59fb3da44a2f2aeeea3 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Mon, 26 Jun 2023 18:23:51 +0100 Subject: [PATCH 20/20] test(remix): Add Remix v2 future flags integration tests. (#8397) Added an integration test application using [Remix 1.17.0](https://github.com/remix-run/remix/releases/tag/remix%401.17.0) and [v2 future flags](https://remix.run/docs/en/main/pages/api-development-strategy) to see the state of current SDK support for v2. --- .github/workflows/build.yml | 4 +- packages/remix/package.json | 4 +- .../{app => app_v1}/entry.client.tsx | 0 .../{app => app_v1}/entry.server.tsx | 0 .../test/integration/{app => app_v1}/root.tsx | 0 .../routes/action-json-response/$id.tsx | 2 + .../app_v1/routes/capture-exception.tsx | 2 + .../app_v1/routes/capture-message.tsx | 2 + .../routes/error-boundary-capture/$id.tsx | 2 + .../test/integration/app_v1/routes/index.tsx | 2 + .../routes/loader-defer-response/index.tsx | 2 + .../routes/loader-json-response/$id.tsx | 2 + .../app_v1/routes/manual-tracing/$id.tsx | 2 + .../app_v1/routes/scope-bleed/$id.tsx | 2 + .../test/integration/app_v2/entry.client.tsx | 16 +++++ .../test/integration/app_v2/entry.server.tsx | 27 +++++++++ .../remix/test/integration/app_v2/root.tsx | 60 +++++++++++++++++++ .../routes/action-json-response.$id.tsx | 2 + .../app_v2/routes/capture-exception.tsx | 2 + .../app_v2/routes/capture-message.tsx | 2 + .../routes/error-boundary-capture.$id.tsx | 2 + .../test/integration/app_v2/routes/index.tsx | 2 + .../app_v2/routes/loader-defer-response.tsx | 2 + .../routes/loader-json-response.$id.tsx | 2 + .../app_v2/routes/manual-tracing.$id.tsx | 2 + .../app_v2/routes/scope-bleed.$id.tsx | 2 + .../routes/action-json-response.$id.tsx} | 0 .../routes/capture-exception.tsx | 0 .../routes/capture-message.tsx | 2 +- .../routes/error-boundary-capture.$id.tsx} | 0 .../{app => common}/routes/index.tsx | 0 .../routes/loader-defer-response.tsx} | 0 .../routes/loader-json-response.$id.tsx} | 0 .../routes/manual-tracing.$id.tsx} | 2 +- .../routes/scope-bleed.$id.tsx} | 0 .../remix/test/integration/remix.config.js | 11 +++- .../test/client/errorboundary.test.ts | 6 +- .../test/client/manualtracing.test.ts | 4 +- .../integration/test/client/pageload.test.ts | 5 +- .../integration/test/server/action.test.ts | 24 ++++---- .../integration/test/server/loader.test.ts | 57 +++++++++++------- packages/remix/test/integration/tsconfig.json | 2 +- 42 files changed, 220 insertions(+), 40 deletions(-) rename packages/remix/test/integration/{app => app_v1}/entry.client.tsx (100%) rename packages/remix/test/integration/{app => app_v1}/entry.server.tsx (100%) rename packages/remix/test/integration/{app => app_v1}/root.tsx (100%) create mode 100644 packages/remix/test/integration/app_v1/routes/action-json-response/$id.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/capture-exception.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/capture-message.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/error-boundary-capture/$id.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/index.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/loader-defer-response/index.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/loader-json-response/$id.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/manual-tracing/$id.tsx create mode 100644 packages/remix/test/integration/app_v1/routes/scope-bleed/$id.tsx create mode 100644 packages/remix/test/integration/app_v2/entry.client.tsx create mode 100644 packages/remix/test/integration/app_v2/entry.server.tsx create mode 100644 packages/remix/test/integration/app_v2/root.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/action-json-response.$id.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/capture-exception.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/capture-message.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/error-boundary-capture.$id.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/index.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/loader-defer-response.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/loader-json-response.$id.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/manual-tracing.$id.tsx create mode 100644 packages/remix/test/integration/app_v2/routes/scope-bleed.$id.tsx rename packages/remix/test/integration/{app/routes/action-json-response/$id.tsx => common/routes/action-json-response.$id.tsx} (100%) rename packages/remix/test/integration/{app => common}/routes/capture-exception.tsx (100%) rename packages/remix/test/integration/{app => common}/routes/capture-message.tsx (89%) rename packages/remix/test/integration/{app/routes/error-boundary-capture/$id.tsx => common/routes/error-boundary-capture.$id.tsx} (100%) rename packages/remix/test/integration/{app => common}/routes/index.tsx (100%) rename packages/remix/test/integration/{app/routes/loader-defer-response/index.tsx => common/routes/loader-defer-response.tsx} (100%) rename packages/remix/test/integration/{app/routes/loader-json-response/$id.tsx => common/routes/loader-json-response.$id.tsx} (100%) rename packages/remix/test/integration/{app/routes/manual-tracing/$id.tsx => common/routes/manual-tracing.$id.tsx} (91%) rename packages/remix/test/integration/{app/routes/scope-bleed/$id.tsx => common/routes/scope-bleed.$id.tsx} (100%) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b8e3fcafcb34..e2ec7fdcf411 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -685,7 +685,7 @@ jobs: yarn test job_remix_integration_tests: - name: Remix (Node ${{ matrix.node }}) Tests + name: Remix v${{ matrix.remix }} (Node ${{ matrix.node }}) Tests needs: [job_get_metadata, job_build] if: needs.job_get_metadata.outputs.changed_remix == 'true' || github.event_name != 'pull_request' runs-on: ubuntu-20.04 @@ -694,6 +694,7 @@ jobs: fail-fast: false matrix: node: [14, 16, 18] + remix: [1, 2] steps: - name: Check out current commit (${{ needs.job_get_metadata.outputs.commit_label }}) uses: actions/checkout@v3 @@ -710,6 +711,7 @@ jobs: - name: Run integration tests env: NODE_VERSION: ${{ matrix.node }} + REMIX_VERSION: ${{ matrix.remix }} run: | cd packages/remix yarn test:integration:ci diff --git a/packages/remix/package.json b/packages/remix/package.json index e964b237149c..a9bf20778ea8 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -59,7 +59,9 @@ "lint:eslint": "eslint . --format stylish", "lint:prettier": "prettier --check \"{src,test,scripts}/**/**.ts\"", "test": "yarn test:unit", - "test:integration": "run-s test:integration:clean test:integration:prepare test:integration:client test:integration:server", + "test:integration": "run-s test:integration:v1 test:integration:v2", + "test:integration:v1": "run-s test:integration:clean test:integration:prepare test:integration:client test:integration:server", + "test:integration:v2": "export REMIX_VERSION=2 && run-s test:integration:v1", "test:integration:ci": "run-s test:integration:clean test:integration:prepare test:integration:client:ci test:integration:server", "test:integration:prepare": "(cd test/integration && yarn)", "test:integration:clean": "(cd test/integration && rimraf .cache node_modules build)", diff --git a/packages/remix/test/integration/app/entry.client.tsx b/packages/remix/test/integration/app_v1/entry.client.tsx similarity index 100% rename from packages/remix/test/integration/app/entry.client.tsx rename to packages/remix/test/integration/app_v1/entry.client.tsx diff --git a/packages/remix/test/integration/app/entry.server.tsx b/packages/remix/test/integration/app_v1/entry.server.tsx similarity index 100% rename from packages/remix/test/integration/app/entry.server.tsx rename to packages/remix/test/integration/app_v1/entry.server.tsx diff --git a/packages/remix/test/integration/app/root.tsx b/packages/remix/test/integration/app_v1/root.tsx similarity index 100% rename from packages/remix/test/integration/app/root.tsx rename to packages/remix/test/integration/app_v1/root.tsx diff --git a/packages/remix/test/integration/app_v1/routes/action-json-response/$id.tsx b/packages/remix/test/integration/app_v1/routes/action-json-response/$id.tsx new file mode 100644 index 000000000000..ed034a14c52a --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/action-json-response/$id.tsx @@ -0,0 +1,2 @@ +export * from '../../../common/routes/action-json-response.$id'; +export { default } from '../../../common/routes/action-json-response.$id'; diff --git a/packages/remix/test/integration/app_v1/routes/capture-exception.tsx b/packages/remix/test/integration/app_v1/routes/capture-exception.tsx new file mode 100644 index 000000000000..1ba745d2e63d --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/capture-exception.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/capture-exception'; +export { default } from '../../common/routes/capture-exception'; diff --git a/packages/remix/test/integration/app_v1/routes/capture-message.tsx b/packages/remix/test/integration/app_v1/routes/capture-message.tsx new file mode 100644 index 000000000000..9dae2318cc14 --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/capture-message.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/capture-message'; +export { default } from '../../common/routes/capture-message'; diff --git a/packages/remix/test/integration/app_v1/routes/error-boundary-capture/$id.tsx b/packages/remix/test/integration/app_v1/routes/error-boundary-capture/$id.tsx new file mode 100644 index 000000000000..2c287dfe9696 --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/error-boundary-capture/$id.tsx @@ -0,0 +1,2 @@ +export * from '../../../common/routes/error-boundary-capture.$id'; +export { default } from '../../../common/routes/error-boundary-capture.$id'; diff --git a/packages/remix/test/integration/app_v1/routes/index.tsx b/packages/remix/test/integration/app_v1/routes/index.tsx new file mode 100644 index 000000000000..22c086a4c2cf --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/index.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/index'; +export { default } from '../../common/routes/index'; diff --git a/packages/remix/test/integration/app_v1/routes/loader-defer-response/index.tsx b/packages/remix/test/integration/app_v1/routes/loader-defer-response/index.tsx new file mode 100644 index 000000000000..fd3a7b3f898d --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/loader-defer-response/index.tsx @@ -0,0 +1,2 @@ +export * from '../../../common/routes/loader-defer-response'; +export { default } from '../../../common/routes/loader-defer-response'; diff --git a/packages/remix/test/integration/app_v1/routes/loader-json-response/$id.tsx b/packages/remix/test/integration/app_v1/routes/loader-json-response/$id.tsx new file mode 100644 index 000000000000..ddf33953d77d --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/loader-json-response/$id.tsx @@ -0,0 +1,2 @@ +export * from '../../../common/routes/loader-json-response.$id'; +export { default } from '../../../common/routes/loader-json-response.$id'; diff --git a/packages/remix/test/integration/app_v1/routes/manual-tracing/$id.tsx b/packages/remix/test/integration/app_v1/routes/manual-tracing/$id.tsx new file mode 100644 index 000000000000..9979714818ff --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/manual-tracing/$id.tsx @@ -0,0 +1,2 @@ +export * from '../../../common/routes/manual-tracing.$id'; +export { default } from '../../../common/routes/manual-tracing.$id'; diff --git a/packages/remix/test/integration/app_v1/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app_v1/routes/scope-bleed/$id.tsx new file mode 100644 index 000000000000..d86864dccb9b --- /dev/null +++ b/packages/remix/test/integration/app_v1/routes/scope-bleed/$id.tsx @@ -0,0 +1,2 @@ +export * from '../../../common/routes/scope-bleed.$id'; +export { default } from '../../../common/routes/scope-bleed.$id'; diff --git a/packages/remix/test/integration/app_v2/entry.client.tsx b/packages/remix/test/integration/app_v2/entry.client.tsx new file mode 100644 index 000000000000..f9cfc14f2507 --- /dev/null +++ b/packages/remix/test/integration/app_v2/entry.client.tsx @@ -0,0 +1,16 @@ +import { RemixBrowser, useLocation, useMatches } from '@remix-run/react'; +import { hydrate } from 'react-dom'; +import * as Sentry from '@sentry/remix'; +import { useEffect } from 'react'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1, + integrations: [ + new Sentry.BrowserTracing({ + routingInstrumentation: Sentry.remixRouterInstrumentation(useEffect, useLocation, useMatches), + }), + ], +}); + +hydrate(, document); diff --git a/packages/remix/test/integration/app_v2/entry.server.tsx b/packages/remix/test/integration/app_v2/entry.server.tsx new file mode 100644 index 000000000000..ae879492e236 --- /dev/null +++ b/packages/remix/test/integration/app_v2/entry.server.tsx @@ -0,0 +1,27 @@ +import type { EntryContext } from '@remix-run/node'; +import { RemixServer } from '@remix-run/react'; +import { renderToString } from 'react-dom/server'; +import * as Sentry from '@sentry/remix'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1, + // Disabling to test series of envelopes deterministically. + autoSessionTracking: false, +}); + +export default function handleRequest( + request: Request, + responseStatusCode: number, + responseHeaders: Headers, + remixContext: EntryContext, +) { + let markup = renderToString(); + + responseHeaders.set('Content-Type', 'text/html'); + + return new Response('' + markup, { + status: responseStatusCode, + headers: responseHeaders, + }); +} diff --git a/packages/remix/test/integration/app_v2/root.tsx b/packages/remix/test/integration/app_v2/root.tsx new file mode 100644 index 000000000000..faf075951d69 --- /dev/null +++ b/packages/remix/test/integration/app_v2/root.tsx @@ -0,0 +1,60 @@ +import { V2_MetaFunction, LoaderFunction, json, defer, redirect } from '@remix-run/node'; +import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; +import { withSentry } from '@sentry/remix'; + +export const meta: V2_MetaFunction = ({ data }) => [ + { charset: 'utf-8' }, + { title: 'New Remix App' }, + { name: 'viewport', content: 'width=device-width,initial-scale=1' }, + { name: 'sentry-trace', content: data.sentryTrace }, + { name: 'baggage', content: data.sentryBaggage }, +]; + +export const loader: LoaderFunction = async ({ request }) => { + const url = new URL(request.url); + const type = url.searchParams.get('type'); + + switch (type) { + case 'empty': + return {}; + case 'plain': + return { + data_one: [], + data_two: 'a string', + }; + case 'json': + return json({ data_one: [], data_two: 'a string' }, { headers: { 'Cache-Control': 'max-age=300' } }); + case 'defer': + return defer({ data_one: [], data_two: 'a string' }); + case 'null': + return null; + case 'undefined': + return undefined; + case 'throwRedirect': + throw redirect('/?type=plain'); + case 'returnRedirect': + return redirect('/?type=plain'); + default: { + return {}; + } + } +}; + +function App() { + return ( + + + + + + + + + + + + + ); +} + +export default withSentry(App); diff --git a/packages/remix/test/integration/app_v2/routes/action-json-response.$id.tsx b/packages/remix/test/integration/app_v2/routes/action-json-response.$id.tsx new file mode 100644 index 000000000000..7a00bfb2bfe7 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/action-json-response.$id.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/action-json-response.$id'; +export { default } from '../../common/routes/action-json-response.$id'; diff --git a/packages/remix/test/integration/app_v2/routes/capture-exception.tsx b/packages/remix/test/integration/app_v2/routes/capture-exception.tsx new file mode 100644 index 000000000000..1ba745d2e63d --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/capture-exception.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/capture-exception'; +export { default } from '../../common/routes/capture-exception'; diff --git a/packages/remix/test/integration/app_v2/routes/capture-message.tsx b/packages/remix/test/integration/app_v2/routes/capture-message.tsx new file mode 100644 index 000000000000..9dae2318cc14 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/capture-message.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/capture-message'; +export { default } from '../../common/routes/capture-message'; diff --git a/packages/remix/test/integration/app_v2/routes/error-boundary-capture.$id.tsx b/packages/remix/test/integration/app_v2/routes/error-boundary-capture.$id.tsx new file mode 100644 index 000000000000..011f92462069 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/error-boundary-capture.$id.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/error-boundary-capture.$id'; +export { default } from '../../common/routes/error-boundary-capture.$id'; diff --git a/packages/remix/test/integration/app_v2/routes/index.tsx b/packages/remix/test/integration/app_v2/routes/index.tsx new file mode 100644 index 000000000000..22c086a4c2cf --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/index.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/index'; +export { default } from '../../common/routes/index'; diff --git a/packages/remix/test/integration/app_v2/routes/loader-defer-response.tsx b/packages/remix/test/integration/app_v2/routes/loader-defer-response.tsx new file mode 100644 index 000000000000..38415a9a3781 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/loader-defer-response.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/loader-defer-response'; +export { default } from '../../common/routes/loader-defer-response'; diff --git a/packages/remix/test/integration/app_v2/routes/loader-json-response.$id.tsx b/packages/remix/test/integration/app_v2/routes/loader-json-response.$id.tsx new file mode 100644 index 000000000000..7761875bdb76 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/loader-json-response.$id.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/loader-json-response.$id'; +export { default } from '../../common/routes/loader-json-response.$id'; diff --git a/packages/remix/test/integration/app_v2/routes/manual-tracing.$id.tsx b/packages/remix/test/integration/app_v2/routes/manual-tracing.$id.tsx new file mode 100644 index 000000000000..a7cfebe4ed46 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/manual-tracing.$id.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/manual-tracing.$id'; +export { default } from '../../common/routes/manual-tracing.$id'; diff --git a/packages/remix/test/integration/app_v2/routes/scope-bleed.$id.tsx b/packages/remix/test/integration/app_v2/routes/scope-bleed.$id.tsx new file mode 100644 index 000000000000..5ba2376f0339 --- /dev/null +++ b/packages/remix/test/integration/app_v2/routes/scope-bleed.$id.tsx @@ -0,0 +1,2 @@ +export * from '../../common/routes/scope-bleed.$id'; +export { default } from '../../common/routes/scope-bleed.$id'; diff --git a/packages/remix/test/integration/app/routes/action-json-response/$id.tsx b/packages/remix/test/integration/common/routes/action-json-response.$id.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/action-json-response/$id.tsx rename to packages/remix/test/integration/common/routes/action-json-response.$id.tsx diff --git a/packages/remix/test/integration/app/routes/capture-exception.tsx b/packages/remix/test/integration/common/routes/capture-exception.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/capture-exception.tsx rename to packages/remix/test/integration/common/routes/capture-exception.tsx diff --git a/packages/remix/test/integration/app/routes/capture-message.tsx b/packages/remix/test/integration/common/routes/capture-message.tsx similarity index 89% rename from packages/remix/test/integration/app/routes/capture-message.tsx rename to packages/remix/test/integration/common/routes/capture-message.tsx index 459e25e1b4ee..06e92f79e931 100644 --- a/packages/remix/test/integration/app/routes/capture-message.tsx +++ b/packages/remix/test/integration/common/routes/capture-message.tsx @@ -3,5 +3,5 @@ import * as Sentry from '@sentry/remix'; export default function ErrorBoundaryCapture() { Sentry.captureMessage('Sentry Manually Captured Message'); - return
; + return
; } diff --git a/packages/remix/test/integration/app/routes/error-boundary-capture/$id.tsx b/packages/remix/test/integration/common/routes/error-boundary-capture.$id.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/error-boundary-capture/$id.tsx rename to packages/remix/test/integration/common/routes/error-boundary-capture.$id.tsx diff --git a/packages/remix/test/integration/app/routes/index.tsx b/packages/remix/test/integration/common/routes/index.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/index.tsx rename to packages/remix/test/integration/common/routes/index.tsx diff --git a/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx b/packages/remix/test/integration/common/routes/loader-defer-response.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/loader-defer-response/index.tsx rename to packages/remix/test/integration/common/routes/loader-defer-response.tsx diff --git a/packages/remix/test/integration/app/routes/loader-json-response/$id.tsx b/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/loader-json-response/$id.tsx rename to packages/remix/test/integration/common/routes/loader-json-response.$id.tsx diff --git a/packages/remix/test/integration/app/routes/manual-tracing/$id.tsx b/packages/remix/test/integration/common/routes/manual-tracing.$id.tsx similarity index 91% rename from packages/remix/test/integration/app/routes/manual-tracing/$id.tsx rename to packages/remix/test/integration/common/routes/manual-tracing.$id.tsx index 75cf8574819a..2f925881b9cf 100644 --- a/packages/remix/test/integration/app/routes/manual-tracing/$id.tsx +++ b/packages/remix/test/integration/common/routes/manual-tracing.$id.tsx @@ -3,5 +3,5 @@ import * as Sentry from '@sentry/remix'; export default function ManualTracing() { const transaction = Sentry.startTransaction({ name: 'test_transaction_1' }); transaction.finish(); - return
; + return
; } diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/common/routes/scope-bleed.$id.tsx similarity index 100% rename from packages/remix/test/integration/app/routes/scope-bleed/$id.tsx rename to packages/remix/test/integration/common/routes/scope-bleed.$id.tsx diff --git a/packages/remix/test/integration/remix.config.js b/packages/remix/test/integration/remix.config.js index 02f847cbf1ca..b4c7ac0837b8 100644 --- a/packages/remix/test/integration/remix.config.js +++ b/packages/remix/test/integration/remix.config.js @@ -1,7 +1,16 @@ /** @type {import('@remix-run/dev').AppConfig} */ +const useV2 = process.env.REMIX_VERSION === '2'; + module.exports = { - appDirectory: 'app', + appDirectory: useV2 ? 'app_v2' : 'app_v1', assetsBuildDirectory: 'public/build', serverBuildPath: 'build/index.js', publicPath: '/build/', + future: { + v2_errorBoundary: useV2, + v2_headers: useV2, + v2_meta: useV2, + v2_normalizeFormMethod: useV2, + v2_routeConvention: useV2, + }, }; diff --git a/packages/remix/test/integration/test/client/errorboundary.test.ts b/packages/remix/test/integration/test/client/errorboundary.test.ts index 6bf6314095fd..b90b3e8d3eaa 100644 --- a/packages/remix/test/integration/test/client/errorboundary.test.ts +++ b/packages/remix/test/integration/test/client/errorboundary.test.ts @@ -2,6 +2,8 @@ import { getMultipleSentryEnvelopeRequests } from './utils/helpers'; import { test, expect } from '@playwright/test'; import { Event } from '@sentry/types'; +const useV2 = process.env.REMIX_VERSION === '2'; + test('should capture React component errors.', async ({ page }) => { const envelopes = await getMultipleSentryEnvelopeRequests(page, 2, { url: '/error-boundary-capture/0', @@ -12,7 +14,9 @@ test('should capture React component errors.', async ({ page }) => { expect(pageloadEnvelope.contexts?.trace.op).toBe('pageload'); expect(pageloadEnvelope.tags?.['routing.instrumentation']).toBe('remix-router'); expect(pageloadEnvelope.type).toBe('transaction'); - expect(pageloadEnvelope.transaction).toBe('routes/error-boundary-capture/$id'); + expect(pageloadEnvelope.transaction).toBe( + useV2 ? 'routes/error-boundary-capture.$id' : 'routes/error-boundary-capture/$id', + ); expect(errorEnvelope.level).toBe('error'); expect(errorEnvelope.sdk?.name).toBe('sentry.javascript.remix'); diff --git a/packages/remix/test/integration/test/client/manualtracing.test.ts b/packages/remix/test/integration/test/client/manualtracing.test.ts index edc919d2d4a9..424408e7be9d 100644 --- a/packages/remix/test/integration/test/client/manualtracing.test.ts +++ b/packages/remix/test/integration/test/client/manualtracing.test.ts @@ -2,6 +2,8 @@ import { getMultipleSentryEnvelopeRequests } from './utils/helpers'; import { test, expect } from '@playwright/test'; import { Event } from '@sentry/types'; +const useV2 = process.env.REMIX_VERSION === '2'; + test('should report a manually created / finished transaction.', async ({ page }) => { const envelopes = await getMultipleSentryEnvelopeRequests(page, 2, { url: '/manual-tracing/0', @@ -17,5 +19,5 @@ test('should report a manually created / finished transaction.', async ({ page } expect(pageloadEnvelope.contexts?.trace?.op).toBe('pageload'); expect(pageloadEnvelope.tags?.['routing.instrumentation']).toBe('remix-router'); expect(pageloadEnvelope.type).toBe('transaction'); - expect(pageloadEnvelope.transaction).toBe('routes/manual-tracing/$id'); + expect(pageloadEnvelope.transaction).toBe(useV2 ? 'routes/manual-tracing.$id' : 'routes/manual-tracing/$id'); }); diff --git a/packages/remix/test/integration/test/client/pageload.test.ts b/packages/remix/test/integration/test/client/pageload.test.ts index 1543bd2a342c..7c49e4ac9c8c 100644 --- a/packages/remix/test/integration/test/client/pageload.test.ts +++ b/packages/remix/test/integration/test/client/pageload.test.ts @@ -1,3 +1,5 @@ +const useV2 = process.env.REMIX_VERSION === '2'; + import { getFirstSentryEnvelopeRequest } from './utils/helpers'; import { test, expect } from '@playwright/test'; import { Event } from '@sentry/types'; @@ -8,5 +10,6 @@ test('should add `pageload` transaction on load.', async ({ page }) => { expect(envelope.contexts?.trace.op).toBe('pageload'); expect(envelope.tags?.['routing.instrumentation']).toBe('remix-router'); expect(envelope.type).toBe('transaction'); - expect(envelope.transaction).toBe('routes/index'); + + expect(envelope.transaction).toBe(useV2 ? 'root' : 'routes/index'); }); diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index 6bb4b74d3540..664162f2076d 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -1,5 +1,7 @@ import { assertSentryTransaction, assertSentryEvent, RemixTestEnv } from './utils/helpers'; +const useV2 = process.env.REMIX_VERSION === '2'; + jest.spyOn(console, 'error').mockImplementation(); // Repeat tests for each adapter @@ -11,10 +13,10 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada const transaction = envelope[2]; assertSentryTransaction(transaction, { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, spans: [ { - description: 'routes/action-json-response/$id', + description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, op: 'function.remix.action', }, { @@ -22,11 +24,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada op: 'function.remix.loader', }, { - description: 'routes/action-json-response/$id', + description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, op: 'function.remix.loader', }, { - description: 'routes/action-json-response/$id', + description: `routes/action-json-response${useV2 ? '.' : '/'}$id`, op: 'function.remix.document_request', }, ], @@ -102,7 +104,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); assertSentryTransaction(transaction[2], { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, request: { method: 'POST', url, @@ -161,7 +163,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, tags: { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -177,7 +179,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, tags: { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -227,7 +229,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, tags: { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -277,7 +279,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, tags: { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -327,7 +329,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, tags: { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -377,7 +379,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }, tags: { - transaction: 'routes/action-json-response/$id', + transaction: `routes/action-json-response${useV2 ? '.' : '/'}$id`, }, }); diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 2545f63d6e92..9613d19fce42 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -1,6 +1,8 @@ import { assertSentryTransaction, RemixTestEnv, assertSentryEvent } from './utils/helpers'; import { Event } from '@sentry/types'; +const useV2 = process.env.REMIX_VERSION === '2'; + jest.spyOn(console, 'error').mockImplementation(); // Repeat tests for each adapter @@ -52,7 +54,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const transaction = envelope[2]; assertSentryTransaction(transaction, { - transaction: 'routes/loader-json-response/$id', + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, transaction_info: { source: 'route', }, @@ -62,11 +64,11 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada op: 'function.remix.loader', }, { - description: 'routes/loader-json-response/$id', + description: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, op: 'function.remix.loader', }, { - description: 'routes/loader-json-response/$id', + description: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, op: 'function.remix.document_request', }, ], @@ -98,7 +100,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }, tags: { - transaction: 'routes/loader-json-response/$id', + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -114,7 +116,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }, tags: { - transaction: 'routes/loader-json-response/$id', + transaction: `routes/loader-json-response${useV2 ? '.' : '/'}$id`, }, }); @@ -195,24 +197,39 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const transaction = envelope[2]; assertSentryTransaction(transaction, { - transaction: 'root', + transaction: useV2 ? 'routes/loader-defer-response' : 'root', transaction_info: { source: 'route', }, - spans: [ - { - description: 'root', - op: 'function.remix.loader', - }, - { - description: 'routes/loader-defer-response/index', - op: 'function.remix.loader', - }, - { - description: 'root', - op: 'function.remix.document_request', - }, - ], + spans: useV2 + ? [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response', + op: 'function.remix.document_request', + }, + ] + : [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response/index', + op: 'function.remix.loader', + }, + { + description: 'root', + op: 'function.remix.document_request', + }, + ], }); }); }); diff --git a/packages/remix/test/integration/tsconfig.json b/packages/remix/test/integration/tsconfig.json index 2129c1a599f6..f190b5da307f 100644 --- a/packages/remix/test/integration/tsconfig.json +++ b/packages/remix/test/integration/tsconfig.json @@ -13,7 +13,7 @@ "forceConsistentCasingInFileNames": true, "baseUrl": ".", "paths": { - "~/*": ["./app/*"] + "~/*": ["app_v1/*", "app_v2/*"] }, "noEmit": true }