From 0eb81bc861f107574fdfc1e3a4a8433ac9224cd1 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 May 2023 11:22:27 +0200 Subject: [PATCH 01/20] fix(node): Add LRU map for tracePropagationTargets calculation (#8130) --- packages/node/src/integrations/http.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 41fe2a68c058..5e2ce9e253a2 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -138,7 +138,7 @@ function _createWrappedRequestMethodFactory( ): WrappedRequestMethodFactory { // We're caching results so we don't have to recompute regexp every time we create a request. const createSpanUrlMap = new LRUMap(100); - const headersUrlMap: Record = {}; + const headersUrlMap = new LRUMap(100); const shouldCreateSpan = (url: string): boolean => { if (tracingOptions?.shouldCreateSpanForRequest === undefined) { @@ -160,13 +160,14 @@ function _createWrappedRequestMethodFactory( return true; } - if (headersUrlMap[url]) { - return headersUrlMap[url]; + const cachedDecision = headersUrlMap.get(url); + if (cachedDecision !== undefined) { + return cachedDecision; } - headersUrlMap[url] = stringMatchesSomePattern(url, tracingOptions.tracePropagationTargets); - - return headersUrlMap[url]; + const decision = stringMatchesSomePattern(url, tracingOptions.tracePropagationTargets); + headersUrlMap.set(url, decision); + return decision; }; return function wrappedRequestMethodFactory(originalRequestMethod: OriginalRequestMethod): WrappedRequestMethod { From 374f6a93030be411addd5d8203cb54d8088c04fd Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 May 2023 13:20:13 +0200 Subject: [PATCH 02/20] fix(node): Add debug logging for node checkin (#8131) --- packages/node/src/client.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 7424e4b10a26..ccdbeda279e1 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -186,6 +186,8 @@ export class NodeClient extends BaseClient { } const envelope = createCheckInEnvelope(serializedCheckIn, this.getSdkMetadata(), tunnel, this.getDsn()); + + __DEBUG_BUILD__ && logger.info('Sending checkin:', checkIn.monitorSlug, checkIn.status); void this._sendEnvelope(envelope); return id; } From 85a6e60078faa7c6b44c0ce961ae5c3b398f7abe Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 3 May 2023 16:05:18 +0200 Subject: [PATCH 03/20] Remove relay extension from AWS Layer we're reverting back to the older setup since the whole 'relay as AWS extension' experiment didn't really work out. * revert port override in DSN * remove gh action that bundles relay * zip in place as part of `buildLambdaLayer` part of getsentry/team-webplatform-meta#58 --- .github/workflows/build.yml | 38 +----------- packages/serverless/package.json | 3 +- .../serverless/scripts/buildLambdaLayer.ts | 6 ++ packages/serverless/src/awslambda.ts | 36 +---------- packages/serverless/tsconfig.json | 3 +- scripts/aws-delete-local-layers.sh | 18 ++++++ scripts/aws-deploy-local-layer.sh | 59 ++----------------- 7 files changed, 33 insertions(+), 130 deletions(-) create mode 100755 scripts/aws-delete-local-layers.sh diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index cac9a0da588a..3670646ec8a7 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -253,43 +253,6 @@ jobs: # `job_build` can't see `job_install_deps` and what it returned) dependency_cache_key: ${{ needs.job_install_deps.outputs.dependency_cache_key }} - job_pack_aws_lambda_layer: - name: Pack and Upload AWS Lambda Layer - needs: [job_get_metadata, job_build] - # only upload the zipped layer file if we're about to release - if: startsWith(github.ref, 'refs/heads/release/') - runs-on: ubuntu-20.04 - steps: - - name: Check out current commit (${{ needs.job_get_metadata.outputs.commit_label }}) - uses: actions/checkout@v3 - with: - ref: ${{ env.HEAD_COMMIT }} - - name: Set up Node - uses: actions/setup-node@v3 - with: - node-version-file: 'package.json' - - name: Restore caches - uses: ./.github/actions/restore-cache - env: - DEPENDENCY_CACHE_KEY: ${{ needs.job_build.outputs.dependency_cache_key }} - - - name: Get SDK version - # `jq` reads JSON files, and `tee` pipes its input to the given location and to stdout. (Adding `-a` is the - # equivalent of using >> rather than >.) - run: | - export SDK_VERSION=$(cat packages/core/package.json | jq --raw-output '.version') - echo "SDK_VERSION=$SDK_VERSION" | tee -a $GITHUB_ENV - - name: Move dist-serverless to root directory (requirement for zipping action) - run: | - mv ./packages/serverless/build/aws/dist-serverless . - - name: Create and upload final zip file - uses: getsentry/action-build-aws-lambda-extension@v1 - with: - artifact_name: ${{ env.HEAD_COMMIT }} - zip_file_name: sentry-node-serverless-${{ env.SDK_VERSION }}.zip - build_cache_paths: ${{ env.CACHED_BUILD_PATHS }} - build_cache_key: ${{ env.BUILD_CACHE_KEY }} - job_size_check: name: Size Check needs: [job_get_metadata, job_build] @@ -399,6 +362,7 @@ jobs: ${{ github.workspace }}/packages/integrations/build/bundles/** ${{ github.workspace }}/packages/replay/build/bundles/** ${{ github.workspace }}/packages/**/*.tgz + ${{ github.workspace }}/packages/serverless/build/aws/dist-serverless/*.zip job_browser_unit_tests: name: Browser Unit Tests diff --git a/packages/serverless/package.json b/packages/serverless/package.json index ec0623fa8016..92b9abc8ef28 100644 --- a/packages/serverless/package.json +++ b/packages/serverless/package.json @@ -34,8 +34,7 @@ "find-up": "^5.0.0", "google-gax": "^2.9.0", "nock": "^13.0.4", - "npm-packlist": "^2.1.4", - "read-pkg": "^5.2.0" + "npm-packlist": "^2.1.4" }, "scripts": { "build": "run-p build:transpile build:types build:bundle", diff --git a/packages/serverless/scripts/buildLambdaLayer.ts b/packages/serverless/scripts/buildLambdaLayer.ts index 459560a660fe..99140ccb0513 100644 --- a/packages/serverless/scripts/buildLambdaLayer.ts +++ b/packages/serverless/scripts/buildLambdaLayer.ts @@ -4,6 +4,7 @@ import * as fs from 'fs'; import * as rimraf from 'rimraf'; import { ensureBundleBuildPrereqs } from '../../../scripts/ensure-bundle-deps'; +import { version } from '../package.json'; /** * Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current @@ -46,6 +47,11 @@ async function buildLambdaLayer(): Promise { '../build/npm/cjs/awslambda-auto.js', 'build/aws/dist-serverless/nodejs/node_modules/@sentry/serverless/dist/awslambda-auto.js', ); + + const zipFilename = `sentry-node-serverless-${version}.zip`; + console.log(`Creating final layer zip file ${zipFilename}.`); + // need to preserve the symlink above with -y + run(`zip -r -y ${zipFilename} .`, { cwd: 'build/aws/dist-serverless' }); } void buildLambdaLayer(); diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index 3f2883385422..888b57852327 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -3,14 +3,7 @@ import type { Scope } from '@sentry/node'; import * as Sentry from '@sentry/node'; import { captureException, captureMessage, flush, getCurrentHub, withScope } from '@sentry/node'; import type { Integration } from '@sentry/types'; -import { - baggageHeaderToDynamicSamplingContext, - dsnFromString, - dsnToString, - extractTraceparentData, - isString, - logger, -} from '@sentry/utils'; +import { baggageHeaderToDynamicSamplingContext, extractTraceparentData, isString, logger } from '@sentry/utils'; // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil // eslint-disable-next-line import/no-unresolved import type { Context, Handler } from 'aws-lambda'; @@ -56,27 +49,6 @@ export interface WrapperOptions { export const defaultIntegrations: Integration[] = [...Sentry.defaultIntegrations, new AWSServices({ optional: true })]; -/** - * Changes a Dsn to point to the `relay` server running in the Lambda Extension. - * - * This is only used by the serverless integration for AWS Lambda. - * - * @param originalDsn The original Dsn of the customer. - * @returns Dsn pointing to Lambda extension. - */ -function extensionRelayDSN(originalDsn: string | undefined): string | undefined { - if (originalDsn === undefined) { - return undefined; - } - - const dsn = dsnFromString(originalDsn); - dsn.host = 'localhost'; - dsn.port = '5333'; - dsn.protocol = 'http'; - - return dsnToString(dsn); -} - interface AWSLambdaOptions extends Sentry.NodeOptions { /** * Internal field that is set to `true` when init() is called by the Sentry AWS Lambda layer. @@ -106,12 +78,6 @@ export function init(options: AWSLambdaOptions = {}): void { version: Sentry.SDK_VERSION, }; - // If invoked by the Sentry Lambda Layer point the SDK to the Lambda Extension (inside the layer) instead of the host - // specified in the DSN - if (options._invokedByLambdaLayer) { - options.dsn = extensionRelayDSN(options.dsn); - } - Sentry.init(options); Sentry.addGlobalEventProcessor(serverlessEventProcessor); } diff --git a/packages/serverless/tsconfig.json b/packages/serverless/tsconfig.json index 05c709778aa0..a2731860dfa0 100644 --- a/packages/serverless/tsconfig.json +++ b/packages/serverless/tsconfig.json @@ -5,6 +5,7 @@ "compilerOptions": { // package-specific options - "target": "ES2018" + "target": "ES2018", + "resolveJsonModule": true } } diff --git a/scripts/aws-delete-local-layers.sh b/scripts/aws-delete-local-layers.sh new file mode 100755 index 000000000000..5fa14fd57cba --- /dev/null +++ b/scripts/aws-delete-local-layers.sh @@ -0,0 +1,18 @@ +#!/usr/bin/env bash +# +# Deletes all versions of the layer specified in LAYER_NAME in one region. +# + +set -euo pipefail + +# override default AWS region +export AWS_REGION=eu-central-1 + +LAYER_NAME=SentryNodeServerlessSDK-local-dev +VERSION="0" + +while [[ $VERSION != "1" ]] +do + VERSION=$(aws lambda list-layer-versions --layer-name $LAYER_NAME | jq '.LayerVersions[0].Version') + aws lambda delete-layer-version --layer-name $LAYER_NAME --version-number $VERSION +done diff --git a/scripts/aws-deploy-local-layer.sh b/scripts/aws-deploy-local-layer.sh index a979ddbbdcbe..db5398adbc63 100755 --- a/scripts/aws-deploy-local-layer.sh +++ b/scripts/aws-deploy-local-layer.sh @@ -16,74 +16,23 @@ rm -rf dist-serverless/ rm -rf ./packages/serverless/build rm -rf ./packages/serverless/dist rm -rf ./packages/serverless/node_modules -rm -f sentry-node-serverless-*.zip # Creating Lambda layer echo "Creating Lambda layer in ./packages/serverless/build/aws/dist-serverless..." cd packages/serverless yarn build -cd ../../ echo "Done creating Lambda layer in ./packages/serverless/build/aws/dist-serverless." -# Move dist-serverless/ to the root folder for the action to pick it up. -# This is only needed in this script, because in GitHub workflow -# this is done with the upload-artifact/download-artifact actions -echo "Copying Lambda layer in ./packages/serverless/build/aws/dist-serverless to working directory..." -mv ./packages/serverless/build/aws/dist-serverless . -echo "Done copying Lambda layer in ./packages/serverless/build/aws/dist-serverless to working directory." - -# IMPORTANT: -# Please make sure that this does the same as the GitHub action that -# is building the Lambda layer in production! -# see: https://github.com/getsentry/action-build-aws-lambda-extension/blob/main/action.yml#L23-L40 - -echo "Downloading relay..." -# Make directory (if not existing) -mkdir -p dist-serverless/relay -# Download releay from release registry to dist-serverless/relay/relay -curl -0 --silent \ - --output dist-serverless/relay/relay \ - "$(curl -s https://release-registry.services.sentry.io/apps/relay/latest | jq -r .files.\"relay-Linux-x86_64\".url)" -# Make file executable -chmod +x dist-serverless/relay/relay -echo "Done downloading relay." - -echo "Creating start script..." -# Make directory (if not existing) -mkdir -p dist-serverless/extensions -# Create 'sentry-lambda-extension' script that starts relay. -# The file has to have exactly this name, because the executable files of -# Lambda extensions need to have same file name as the name that they use -# to register with AWS Lambda environment -cat > dist-serverless/extensions/sentry-lambda-extension << EOT -#!/bin/bash -set -euo pipefail -exec /opt/relay/relay run \ - --mode=proxy \ - --shutdown-timeout=2 \ - --upstream-dsn="\$SENTRY_DSN" \ - --aws-runtime-api="\$AWS_LAMBDA_RUNTIME_API" -EOT -# Make script executable -chmod +x dist-serverless/extensions/sentry-lambda-extension -echo "Done creating start script." - -# Zip Lambda layer and included Lambda extension -echo "Zipping Lambda layer and included Lambda extension..." -cd dist-serverless/ -zip -r -y ../sentry-node-serverless-x.x.x-dev.zip . -cd .. -echo "Done Zipping Lambda layer and included Lambda extension to ./sentry-node-serverless-x.x.x-dev.zip." - # Deploying zipped Lambda layer to AWS -echo "Deploying zipped Lambda layer to AWS..." +ZIP=$(ls build/aws/dist-serverless | grep sentry-node-serverless | head -n 1) +echo "Deploying zipped Lambda layer $ZIP to AWS..." aws lambda publish-layer-version \ --layer-name "SentryNodeServerlessSDK-local-dev" \ --region "eu-central-1" \ - --zip-file "fileb://sentry-node-serverless-x.x.x-dev.zip" \ + --zip-file "fileb://build/aws/dist-serverless/$ZIP" \ --description "Local test build of SentryNodeServerlessSDK (can be deleted)" \ - --no-cli-pager + --compatible-runtimes nodejs10.x nodejs12.x nodejs14.x nodejs16.x nodejs18.x echo "Done deploying zipped Lambda layer to AWS as 'SentryNodeServerlessSDK-local-dev'." From 359a517e38dbc0b51b4548c659c48832fffed7c1 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 May 2023 17:56:04 +0200 Subject: [PATCH 04/20] ref(node): Cache undici trace propagation decisions (#8136) --- .../node/src/integrations/undici/index.ts | 45 +++++++++++++++---- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/packages/node/src/integrations/undici/index.ts b/packages/node/src/integrations/undici/index.ts index b609cba70b50..fabc045515e2 100644 --- a/packages/node/src/integrations/undici/index.ts +++ b/packages/node/src/integrations/undici/index.ts @@ -6,6 +6,7 @@ import { stringMatchesSomePattern, stripUrlQueryAndFragment, } from '@sentry/utils'; +import { LRUMap } from 'lru_map'; import type { NodeClient } from '../../client'; import { NODE_VERSION } from '../../nodeVersion'; @@ -29,7 +30,7 @@ export interface UndiciOptions { * Function determining whether or not to create spans to track outgoing requests to the given URL. * By default, spans will be created for all outgoing requests. */ - shouldCreateSpanForRequest: (url: string) => boolean; + shouldCreateSpanForRequest?: (url: string) => boolean; } // Please note that you cannot use `console.log` to debug the callbacks registered to the `diagnostics_channel` API. @@ -57,10 +58,13 @@ export class Undici implements Integration { private readonly _options: UndiciOptions; + private readonly _createSpanUrlMap: LRUMap = new LRUMap(100); + private readonly _headersUrlMap: LRUMap = new LRUMap(100); + public constructor(_options: Partial = {}) { this._options = { breadcrumbs: _options.breadcrumbs === undefined ? true : _options.breadcrumbs, - shouldCreateSpanForRequest: _options.shouldCreateSpanForRequest || (() => true), + shouldCreateSpanForRequest: _options.shouldCreateSpanForRequest, }; } @@ -85,6 +89,21 @@ export class Undici implements Integration { return; } + const shouldCreateSpan = (url: string): boolean => { + if (this._options.shouldCreateSpanForRequest === undefined) { + return true; + } + + const cachedDecision = this._createSpanUrlMap.get(url); + if (cachedDecision !== undefined) { + return cachedDecision; + } + + const decision = this._options.shouldCreateSpanForRequest(url); + this._createSpanUrlMap.set(url, decision); + return decision; + }; + // https://github.com/nodejs/undici/blob/e6fc80f809d1217814c044f52ed40ef13f21e43c/docs/api/DiagnosticsChannel.md ds.subscribe(ChannelName.RequestCreate, message => { const hub = getCurrentHub(); @@ -108,9 +127,8 @@ export class Undici implements Integration { if (activeSpan && client) { const clientOptions = client.getOptions(); - const shouldCreateSpan = this._options.shouldCreateSpanForRequest(stringUrl); - if (shouldCreateSpan) { + if (shouldCreateSpan(stringUrl)) { const method = request.method || 'GET'; const data: Record = { 'http.method': method, @@ -129,11 +147,22 @@ export class Undici implements Integration { }); request.__sentry__ = span; - const shouldPropagate = clientOptions.tracePropagationTargets - ? stringMatchesSomePattern(stringUrl, clientOptions.tracePropagationTargets) - : true; + const shouldAttachTraceData = (url: string): boolean => { + if (clientOptions.tracePropagationTargets === undefined) { + return true; + } + + const cachedDecision = this._headersUrlMap.get(url); + if (cachedDecision !== undefined) { + return cachedDecision; + } + + const decision = stringMatchesSomePattern(url, clientOptions.tracePropagationTargets); + this._headersUrlMap.set(url, decision); + return decision; + }; - if (shouldPropagate) { + if (shouldAttachTraceData(stringUrl)) { request.addHeader('sentry-trace', span.toTraceparent()); if (span.transaction) { const dynamicSamplingContext = span.transaction.getDynamicSamplingContext(); From 70df2c69ea1a9888f407d2a061fd056b17d086bb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 May 2023 19:21:32 +0200 Subject: [PATCH 05/20] chore(vue): Remove vue bundle generation (#8138) --- packages/vue/package.json | 2 -- packages/vue/rollup.bundle.config.js | 11 ----------- 2 files changed, 13 deletions(-) delete mode 100644 packages/vue/rollup.bundle.config.js diff --git a/packages/vue/package.json b/packages/vue/package.json index 9f630d742200..56b2e07b9d33 100644 --- a/packages/vue/package.json +++ b/packages/vue/package.json @@ -30,12 +30,10 @@ }, "scripts": { "build": "run-p build:transpile build:types", - "build:bundle": "rollup --config rollup.bundle.config.js", "build:dev": "run-p build:transpile build:types", "build:transpile": "rollup -c rollup.npm.config.js", "build:types": "tsc -p tsconfig.types.json", "build:watch": "run-p build:transpile:watch build:types:watch", - "build:bundle:watch": "rollup --config --watch", "build:dev:watch": "run-p build:transpile:watch build:types:watch", "build:transpile:watch": "rollup -c rollup.npm.config.js --watch", "build:types:watch": "tsc -p tsconfig.types.json --watch", diff --git a/packages/vue/rollup.bundle.config.js b/packages/vue/rollup.bundle.config.js deleted file mode 100644 index 4df9b0d5b614..000000000000 --- a/packages/vue/rollup.bundle.config.js +++ /dev/null @@ -1,11 +0,0 @@ -import { makeBaseBundleConfig, makeBundleConfigVariants } from '../../rollup/index.js'; - -const baseBundleConfig = makeBaseBundleConfig({ - bundleType: 'standalone', - entrypoints: ['src/index.bundle.ts'], - jsVersion: 'es6', - licenseTitle: '@sentry/vue', - outputFileBase: () => 'bundle.vue', -}); - -export default makeBundleConfigVariants(baseBundleConfig); From 60a60ad987012785d763919d984bfe05e1093708 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 16 May 2023 16:18:38 -0230 Subject: [PATCH 06/20] feat(replay): Do not capture replays < 5 seconds (#7949) Do not immediately flush on snapshot checkouts, instead delay by minimum flush delay (5 seconds). This means that we will not collect replays < 5 seconds. e.g. User opens site and immediately closes the tab. Ref https://github.com/getsentry/team-replay/issues/63 --- packages/replay/src/replay.ts | 32 +- packages/replay/src/types.ts | 3 + .../replay/src/util/handleRecordingEmit.ts | 24 + .../errorSampleRate-delayFlush.test.ts | 759 ++++++++++++++++++ 4 files changed, 805 insertions(+), 13 deletions(-) create mode 100644 packages/replay/test/integration/errorSampleRate-delayFlush.test.ts diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 8d89aa0b2653..e45830e9fdde 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -463,7 +463,17 @@ export class ReplayContainer implements ReplayContainerInterface { } /** - * + * Only flush if `this.recordingMode === 'session'` + */ + public conditionalFlush(): Promise { + if (this.recordingMode === 'buffer') { + return Promise.resolve(); + } + + return this.flushImmediate(); + } + + /** * Always flush via `_debouncedFlush` so that we do not have flushes triggered * from calling both `flush` and `_debouncedFlush`. Otherwise, there could be * cases of mulitple flushes happening closely together. @@ -474,6 +484,13 @@ export class ReplayContainer implements ReplayContainerInterface { return this._debouncedFlush.flush() as Promise; } + /** + * Cancels queued up flushes. + */ + public cancelFlush(): void { + this._debouncedFlush.cancel(); + } + /** Get the current sesion (=replay) ID */ public getSessionId(): string | undefined { return this.session && this.session.id; @@ -723,7 +740,7 @@ export class ReplayContainer implements ReplayContainerInterface { // Send replay when the page/tab becomes hidden. There is no reason to send // replay if it becomes visible, since no actions we care about were done // while it was hidden - this._conditionalFlush(); + void this.conditionalFlush(); } /** @@ -807,17 +824,6 @@ export class ReplayContainer implements ReplayContainerInterface { return Promise.all(createPerformanceSpans(this, createPerformanceEntries(entries))); } - /** - * Only flush if `this.recordingMode === 'session'` - */ - private _conditionalFlush(): void { - if (this.recordingMode === 'buffer') { - return; - } - - void this.flushImmediate(); - } - /** * Clear _context */ diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index 12900dc22e74..dfeeb0982194 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -285,6 +285,7 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions { scrollTimeout: number; ignoreSelectors: string[]; }; + delayFlushOnCheckout: number; }>; } @@ -515,7 +516,9 @@ export interface ReplayContainer { startRecording(): void; stopRecording(): boolean; sendBufferedReplayOrFlush(options?: SendBufferedReplayOptions): Promise; + conditionalFlush(): Promise; flushImmediate(): Promise; + cancelFlush(): void; triggerUserActivity(): void; addUpdate(cb: AddUpdateCallback): void; getOptions(): ReplayPluginOptions; diff --git a/packages/replay/src/util/handleRecordingEmit.ts b/packages/replay/src/util/handleRecordingEmit.ts index f72850f5536c..3a9dcc211edd 100644 --- a/packages/replay/src/util/handleRecordingEmit.ts +++ b/packages/replay/src/util/handleRecordingEmit.ts @@ -80,6 +80,30 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa } } + const options = replay.getOptions(); + + // TODO: We want this as an experiment so that we can test + // internally and create metrics before making this the default + if (options._experiments.delayFlushOnCheckout) { + // If the full snapshot is due to an initial load, we will not have + // a previous session ID. In this case, we want to buffer events + // for a set amount of time before flushing. This can help avoid + // capturing replays of users that immediately close the window. + setTimeout(() => replay.conditionalFlush(), options._experiments.delayFlushOnCheckout); + + // Cancel any previously debounced flushes to ensure there are no [near] + // simultaneous flushes happening. The latter request should be + // insignificant in this case, so wait for additional user interaction to + // trigger a new flush. + // + // This can happen because there's no guarantee that a recording event + // happens first. e.g. a mouse click can happen and trigger a debounced + // flush before the checkout. + replay.cancelFlush(); + + return true; + } + // Flush immediately so that we do not miss the first segment, otherwise // it can prevent loading on the UI. This will cause an increase in short // replays (e.g. opening and closing a tab quickly), but these can be diff --git a/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts b/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts new file mode 100644 index 000000000000..c0b711c028f8 --- /dev/null +++ b/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts @@ -0,0 +1,759 @@ +import { captureException, getCurrentHub } from '@sentry/core'; + +import { + BUFFER_CHECKOUT_TIME, + DEFAULT_FLUSH_MIN_DELAY, + MAX_SESSION_LIFE, + REPLAY_SESSION_KEY, + SESSION_IDLE_EXPIRE_DURATION, + WINDOW, +} from '../../src/constants'; +import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; +import { addEvent } from '../../src/util/addEvent'; +import { createOptionsEvent } from '../../src/util/handleRecordingEmit'; +import { PerformanceEntryResource } from '../fixtures/performanceEntry/resource'; +import type { RecordMock } from '../index'; +import { BASE_TIMESTAMP } from '../index'; +import { resetSdkMock } from '../mocks/resetSdkMock'; +import type { DomHandler } from '../types'; +import { useFakeTimers } from '../utils/use-fake-timers'; + +useFakeTimers(); + +async function advanceTimers(time: number) { + jest.advanceTimersByTime(time); + await new Promise(process.nextTick); +} + +async function waitForBufferFlush() { + await new Promise(process.nextTick); + await new Promise(process.nextTick); +} + +async function waitForFlush() { + await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); +} + +describe('Integration | errorSampleRate with delayed flush', () => { + let replay: ReplayContainer; + let mockRecord: RecordMock; + let domHandler: DomHandler; + + beforeEach(async () => { + ({ mockRecord, domHandler, replay } = await resetSdkMock({ + replayOptions: { + stickySession: true, + _experiments: { + delayFlushOnCheckout: DEFAULT_FLUSH_MIN_DELAY, + }, + }, + sentryOptions: { + replaysSessionSampleRate: 0.0, + replaysOnErrorSampleRate: 1.0, + }, + })); + }); + + afterEach(async () => { + clearSession(replay); + replay.stop(); + }); + + it('uploads a replay when `Sentry.captureException` is called and continues recording', async () => { + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + const optionsEvent = createOptionsEvent(replay); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + + // Does not capture on mouse click + domHandler({ + name: 'click', + }); + jest.runAllTimers(); + await new Promise(process.nextTick); + expect(replay).not.toHaveLastSentReplay(); + + captureException(new Error('testing')); + + await waitForBufferFlush(); + + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + recordingData: JSON.stringify([ + { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, + optionsEvent, + TEST_EVENT, + { + type: 5, + timestamp: BASE_TIMESTAMP, + data: { + tag: 'breadcrumb', + payload: { + timestamp: BASE_TIMESTAMP / 1000, + type: 'default', + category: 'ui.click', + message: '', + data: {}, + }, + }, + }, + ]), + }); + + await waitForFlush(); + + // This is from when we stop recording and start a session recording + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 1 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 40, type: 2 }]), + }); + + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + + // Check that click will get captured + domHandler({ + name: 'click', + }); + + await waitForFlush(); + + expect(replay).toHaveLastSentReplay({ + recordingData: JSON.stringify([ + { + type: 5, + timestamp: BASE_TIMESTAMP + 10000 + 80, + data: { + tag: 'breadcrumb', + payload: { + timestamp: (BASE_TIMESTAMP + 10000 + 80) / 1000, + type: 'default', + category: 'ui.click', + message: '', + data: {}, + }, + }, + }, + ]), + }); + }); + + it('manually flushes replay and does not continue to record', async () => { + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + const optionsEvent = createOptionsEvent(replay); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + + // Does not capture on mouse click + domHandler({ + name: 'click', + }); + jest.runAllTimers(); + await new Promise(process.nextTick); + expect(replay).not.toHaveLastSentReplay(); + + replay.sendBufferedReplayOrFlush({ continueRecording: false }); + + await waitForBufferFlush(); + + expect(replay).toHaveSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + recordingData: JSON.stringify([ + { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, + optionsEvent, + TEST_EVENT, + { + type: 5, + timestamp: BASE_TIMESTAMP, + data: { + tag: 'breadcrumb', + payload: { + timestamp: BASE_TIMESTAMP / 1000, + type: 'default', + category: 'ui.click', + message: '', + data: {}, + }, + }, + }, + ]), + }); + + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + // Check that click will not get captured + domHandler({ + name: 'click', + }); + + await waitForFlush(); + + // This is still the last replay sent since we passed `continueRecording: + // false`. + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + recordingData: JSON.stringify([ + { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, + optionsEvent, + TEST_EVENT, + { + type: 5, + timestamp: BASE_TIMESTAMP, + data: { + tag: 'breadcrumb', + payload: { + timestamp: BASE_TIMESTAMP / 1000, + type: 'default', + category: 'ui.click', + message: '', + data: {}, + }, + }, + }, + ]), + }); + }); + + it('does not send a replay when triggering a full dom snapshot when document becomes visible after [SESSION_IDLE_EXPIRE_DURATION]ms', async () => { + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get: function () { + return 'visible'; + }, + }); + + jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1); + + document.dispatchEvent(new Event('visibilitychange')); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(replay).not.toHaveLastSentReplay(); + }); + + it('does not send a replay if user hides the tab and comes back within 60 seconds', async () => { + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get: function () { + return 'hidden'; + }, + }); + document.dispatchEvent(new Event('visibilitychange')); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(replay).not.toHaveLastSentReplay(); + + // User comes back before `SESSION_IDLE_EXPIRE_DURATION` elapses + jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION - 100); + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get: function () { + return 'visible'; + }, + }); + document.dispatchEvent(new Event('visibilitychange')); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + }); + + it('does not upload a replay event when document becomes hidden', async () => { + Object.defineProperty(document, 'visibilityState', { + configurable: true, + get: function () { + return 'hidden'; + }, + }); + + // Pretend 5 seconds have passed + const ELAPSED = 5000; + jest.advanceTimersByTime(ELAPSED); + + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 2 }; + addEvent(replay, TEST_EVENT); + + document.dispatchEvent(new Event('visibilitychange')); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + }); + + it('does not upload a replay event if 5 seconds have elapsed since the last replay event occurred', async () => { + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + // Pretend 5 seconds have passed + const ELAPSED = 5000; + await advanceTimers(ELAPSED); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(replay).not.toHaveLastSentReplay(); + }); + + it('does not upload a replay event if 15 seconds have elapsed since the last replay upload', async () => { + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + // Fire a new event every 4 seconds, 4 times + [...Array(4)].forEach(() => { + mockRecord._emitter(TEST_EVENT); + jest.advanceTimersByTime(4000); + }); + + // We are at time = +16seconds now (relative to BASE_TIMESTAMP) + // The next event should cause an upload immediately + mockRecord._emitter(TEST_EVENT); + await new Promise(process.nextTick); + + expect(replay).not.toHaveLastSentReplay(); + + // There should also not be another attempt at an upload 5 seconds after the last replay event + await waitForFlush(); + expect(replay).not.toHaveLastSentReplay(); + + // Let's make sure it continues to work + mockRecord._emitter(TEST_EVENT); + await waitForFlush(); + jest.runAllTimers(); + await new Promise(process.nextTick); + expect(replay).not.toHaveLastSentReplay(); + }); + + // When the error session records as a normal session, we want to stop + // recording after the session ends. Otherwise, we get into a state where the + // new session is a session type replay (this could conflict with the session + // sample rate of 0.0), or an error session that has no errors. Instead we + // simply stop the session replay completely and wait for a new page load to + // resample. + it.each([ + ['MAX_SESSION_LIFE', MAX_SESSION_LIFE], + ['SESSION_IDLE_DURATION', SESSION_IDLE_EXPIRE_DURATION], + ])( + 'stops replay if session had an error and exceeds %s and does not start a new session thereafter', + async (_label, waitTime) => { + expect(replay.session?.shouldRefresh).toBe(true); + + captureException(new Error('testing')); + + await waitForBufferFlush(); + + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + }); + + await waitForFlush(); + + // segment_id is 1 because it sends twice on error + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 1 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + }); + expect(replay.session?.shouldRefresh).toBe(false); + + // Idle for given time + jest.advanceTimersByTime(waitTime + 1); + await new Promise(process.nextTick); + + const TEST_EVENT = { + data: { name: 'lost event' }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + mockRecord._emitter(TEST_EVENT); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + // We stop recording after 15 minutes of inactivity in error mode + + // still no new replay sent + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 1 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + }); + + expect(replay.isEnabled()).toBe(false); + + domHandler({ + name: 'click', + }); + + // Remains disabled! + expect(replay.isEnabled()).toBe(false); + }, + ); + + it.each([ + ['MAX_SESSION_LIFE', MAX_SESSION_LIFE], + ['SESSION_IDLE_EXPIRE_DURATION', SESSION_IDLE_EXPIRE_DURATION], + ])('continues buffering replay if session had no error and exceeds %s', async (_label, waitTime) => { + expect(replay).not.toHaveLastSentReplay(); + + // Idle for given time + jest.advanceTimersByTime(waitTime + 1); + await new Promise(process.nextTick); + + const TEST_EVENT = { + data: { name: 'lost event' }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + mockRecord._emitter(TEST_EVENT); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + // still no new replay sent + expect(replay).not.toHaveLastSentReplay(); + + expect(replay.isEnabled()).toBe(true); + expect(replay.isPaused()).toBe(false); + expect(replay.recordingMode).toBe('buffer'); + + domHandler({ + name: 'click', + }); + + await waitForFlush(); + + expect(replay).not.toHaveLastSentReplay(); + expect(replay.isEnabled()).toBe(true); + expect(replay.isPaused()).toBe(false); + expect(replay.recordingMode).toBe('buffer'); + + // should still react to errors later on + captureException(new Error('testing')); + + await waitForBufferFlush(); + + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + }); + + expect(replay.isEnabled()).toBe(true); + expect(replay.isPaused()).toBe(false); + expect(replay.recordingMode).toBe('session'); + expect(replay.session?.sampled).toBe('buffer'); + expect(replay.session?.shouldRefresh).toBe(false); + }); + + // Should behave the same as above test + it('stops replay if user has been idle for more than SESSION_IDLE_EXPIRE_DURATION and does not start a new session thereafter', async () => { + // Idle for 15 minutes + jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1); + + const TEST_EVENT = { + data: { name: 'lost event' }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + mockRecord._emitter(TEST_EVENT); + expect(replay).not.toHaveLastSentReplay(); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + // We stop recording after SESSION_IDLE_EXPIRE_DURATION of inactivity in error mode + expect(replay).not.toHaveLastSentReplay(); + expect(replay.isEnabled()).toBe(true); + expect(replay.isPaused()).toBe(false); + expect(replay.recordingMode).toBe('buffer'); + + // should still react to errors later on + captureException(new Error('testing')); + + await new Promise(process.nextTick); + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + await new Promise(process.nextTick); + + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + replay_type: 'buffer', + }), + }); + + expect(replay.isEnabled()).toBe(true); + expect(replay.isPaused()).toBe(false); + expect(replay.recordingMode).toBe('session'); + expect(replay.session?.sampled).toBe('buffer'); + expect(replay.session?.shouldRefresh).toBe(false); + }); + + it('has the correct timestamps with deferred root event and last replay update', async () => { + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + const optionsEvent = createOptionsEvent(replay); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + + captureException(new Error('testing')); + + await new Promise(process.nextTick); + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + await new Promise(process.nextTick); + + expect(replay).toHaveSentReplay({ + recordingData: JSON.stringify([ + { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, + optionsEvent, + TEST_EVENT, + ]), + replayEventPayload: expect.objectContaining({ + replay_start_timestamp: BASE_TIMESTAMP / 1000, + // the exception happens roughly 10 seconds after BASE_TIMESTAMP + // (advance timers + waiting for flush after the checkout) and + // extra time is likely due to async of `addMemoryEntry()` + + timestamp: (BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY + DEFAULT_FLUSH_MIN_DELAY + 40) / 1000, + error_ids: [expect.any(String)], + trace_ids: [], + urls: ['http://localhost/'], + replay_id: expect.any(String), + }), + recordingPayloadHeader: { segment_id: 0 }, + }); + }); + + it('has correct timestamps when error occurs much later than initial pageload/checkout', async () => { + const ELAPSED = BUFFER_CHECKOUT_TIME; + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + + // add a mock performance event + replay.performanceEvents.push(PerformanceEntryResource()); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + + jest.advanceTimersByTime(ELAPSED); + + // in production, this happens at a time interval + // session started time should be updated to this current timestamp + mockRecord.takeFullSnapshot(true); + const optionsEvent = createOptionsEvent(replay); + + jest.runAllTimers(); + jest.advanceTimersByTime(20); + await new Promise(process.nextTick); + + captureException(new Error('testing')); + + await waitForBufferFlush(); + + expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + 20); + + // Does not capture mouse click + expect(replay).toHaveSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + replayEventPayload: expect.objectContaining({ + // Make sure the old performance event is thrown out + replay_start_timestamp: (BASE_TIMESTAMP + ELAPSED + 20) / 1000, + }), + recordingData: JSON.stringify([ + { + data: { isCheckout: true }, + timestamp: BASE_TIMESTAMP + ELAPSED + 20, + type: 2, + }, + optionsEvent, + ]), + }); + }); + + it('stops replay when user goes idle', async () => { + jest.setSystemTime(BASE_TIMESTAMP); + + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + captureException(new Error('testing')); + + await waitForBufferFlush(); + + expect(replay).toHaveLastSentReplay(); + + // Flush from calling `stopRecording` + await waitForFlush(); + + // Now wait after session expires - should stop recording + mockRecord.takeFullSnapshot.mockClear(); + (getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance).mockClear(); + + expect(replay).not.toHaveLastSentReplay(); + + // Go idle + jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1); + await new Promise(process.nextTick); + + mockRecord._emitter(TEST_EVENT); + + expect(replay).not.toHaveLastSentReplay(); + + await waitForFlush(); + + expect(replay).not.toHaveLastSentReplay(); + expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0); + expect(replay.isEnabled()).toBe(false); + }); + + it('stops replay when session exceeds max length', async () => { + jest.setSystemTime(BASE_TIMESTAMP); + + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + captureException(new Error('testing')); + + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + await new Promise(process.nextTick); + + expect(replay).toHaveLastSentReplay(); + + // Wait a bit, shortly before session expires + jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000); + await new Promise(process.nextTick); + + mockRecord._emitter(TEST_EVENT); + replay.triggerUserActivity(); + + expect(replay).toHaveLastSentReplay(); + + // Now wait after session expires - should stop recording + mockRecord.takeFullSnapshot.mockClear(); + (getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance).mockClear(); + + jest.advanceTimersByTime(10_000); + await new Promise(process.nextTick); + + mockRecord._emitter(TEST_EVENT); + replay.triggerUserActivity(); + + jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); + await new Promise(process.nextTick); + + expect(replay).not.toHaveLastSentReplay(); + expect(mockRecord.takeFullSnapshot).toHaveBeenCalledTimes(0); + expect(replay.isEnabled()).toBe(false); + }); +}); + +/** + * This is testing a case that should only happen with error-only sessions. + * Previously we had assumed that loading a session from session storage meant + * that the session was not new. However, this is not the case with error-only + * sampling since we can load a saved session that did not have an error (and + * thus no replay was created). + */ +it('sends a replay after loading the session from storage', async () => { + // Pretend that a session is already saved before loading replay + WINDOW.sessionStorage.setItem( + REPLAY_SESSION_KEY, + `{"segmentId":0,"id":"fd09adfc4117477abc8de643e5a5798a","sampled":"buffer","started":${BASE_TIMESTAMP},"lastActivity":${BASE_TIMESTAMP}}`, + ); + const { mockRecord, replay, integration } = await resetSdkMock({ + replayOptions: { + stickySession: true, + _experiments: { + delayFlushOnCheckout: DEFAULT_FLUSH_MIN_DELAY, + }, + }, + sentryOptions: { + replaysOnErrorSampleRate: 1.0, + }, + autoStart: false, + }); + integration['_initialize'](); + const optionsEvent = createOptionsEvent(replay); + + jest.runAllTimers(); + + await new Promise(process.nextTick); + const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; + mockRecord._emitter(TEST_EVENT); + + expect(replay).not.toHaveLastSentReplay(); + + captureException(new Error('testing')); + + // 2 ticks to send replay from an error + await waitForBufferFlush(); + + // Buffered events before error + expect(replay).toHaveSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + recordingData: JSON.stringify([ + { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, + optionsEvent, + TEST_EVENT, + ]), + }); + + // `startRecording()` after switching to session mode to continue recording + await waitForFlush(); + + // Latest checkout when we call `startRecording` again after uploading segment + // after an error occurs (e.g. when we switch to session replay recording) + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 1 }, + recordingData: JSON.stringify([ + { data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY + 40, type: 2 }, + ]), + }); +}); From 389cfd363b4a5b5fa16e8904072f5f67c008f6c5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 16 May 2023 23:15:52 +0200 Subject: [PATCH 07/20] fix(nextjs): Import path issue on Windows --- packages/nextjs/src/config/loaders/wrappingLoader.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 28c8f58b2eb9..53891bb298a9 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -203,7 +203,15 @@ export default function wrappingLoader( } if (sentryConfigFilePath) { - templateCode = `import "${sentryConfigFilePath}";`.concat(templateCode); + let importPath = sentryConfigFilePath; + + // absolute paths do not work with Windows + // https://github.com/getsentry/sentry-javascript/issues/8133 + if (path.isAbsolute(importPath)) { + importPath = path.relative(path.dirname(this.resourcePath), importPath); + } + + templateCode = `import "${importPath.replace(/\\/g, '/')}";\n`.concat(templateCode); } } else if (wrappingTargetKind === 'middleware') { templateCode = middlewareWrapperTemplateCode; From 23a169786fa469609783cad3f548c63ba45d169f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 17 May 2023 13:14:09 +0200 Subject: [PATCH 08/20] fix(node): remove new URL usage in Undici integration (#8147) --- .../node/src/integrations/undici/index.ts | 29 ++++++++++++------- .../node/test/integrations/undici.test.ts | 19 ++++++++++++ 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/packages/node/src/integrations/undici/index.ts b/packages/node/src/integrations/undici/index.ts index fabc045515e2..38f920283b7e 100644 --- a/packages/node/src/integrations/undici/index.ts +++ b/packages/node/src/integrations/undici/index.ts @@ -3,8 +3,9 @@ import type { EventProcessor, Integration } from '@sentry/types'; import { dynamicRequire, dynamicSamplingContextToSentryBaggageHeader, + getSanitizedUrlString, + parseUrl, stringMatchesSomePattern, - stripUrlQueryAndFragment, } from '@sentry/utils'; import { LRUMap } from 'lru_map'; @@ -36,6 +37,15 @@ export interface UndiciOptions { // Please note that you cannot use `console.log` to debug the callbacks registered to the `diagnostics_channel` API. // To debug, you can use `writeFileSync` to write to a file: // https://nodejs.org/api/async_hooks.html#printing-in-asynchook-callbacks +// +// import { writeFileSync } from 'fs'; +// import { format } from 'util'; +// +// function debug(...args: any): void { +// // Use a function like this one when debugging inside an AsyncHook callback +// // @ts-ignore any +// writeFileSync('log.out', `${format(...args)}\n`, { flag: 'a' }); +// } /** * Instruments outgoing HTTP requests made with the `undici` package via @@ -113,8 +123,8 @@ export class Undici implements Integration { const { request } = message as RequestCreateMessage; - const url = new URL(request.path, request.origin); - const stringUrl = url.toString(); + const stringUrl = request.origin ? request.origin.toString() + request.path : request.path; + const url = parseUrl(stringUrl); if (isSentryRequest(stringUrl) || request.__sentry__ !== undefined) { return; @@ -133,16 +143,15 @@ export class Undici implements Integration { const data: Record = { 'http.method': method, }; - const params = url.searchParams.toString(); - if (params) { - data['http.query'] = `?${params}`; + if (url.search) { + data['http.query'] = url.search; } if (url.hash) { data['http.fragment'] = url.hash; } const span = activeSpan.startChild({ op: 'http.client', - description: `${method} ${stripUrlQueryAndFragment(stringUrl)}`, + description: `${method} ${getSanitizedUrlString(url)}`, data, }); request.__sentry__ = span; @@ -184,8 +193,7 @@ export class Undici implements Integration { const { request, response } = message as RequestEndMessage; - const url = new URL(request.path, request.origin); - const stringUrl = url.toString(); + const stringUrl = request.origin ? request.origin.toString() + request.path : request.path; if (isSentryRequest(stringUrl)) { return; @@ -225,8 +233,7 @@ export class Undici implements Integration { const { request } = message as RequestErrorMessage; - const url = new URL(request.path, request.origin); - const stringUrl = url.toString(); + const stringUrl = request.origin ? request.origin.toString() + request.path : request.path; if (isSentryRequest(stringUrl)) { return; diff --git a/packages/node/test/integrations/undici.test.ts b/packages/node/test/integrations/undici.test.ts index ab3c6bfe346e..f4925f7046a2 100644 --- a/packages/node/test/integrations/undici.test.ts +++ b/packages/node/test/integrations/undici.test.ts @@ -132,6 +132,25 @@ conditionalTest({ min: 16 })('Undici integration', () => { expect(span).toEqual(expect.objectContaining({ status: 'internal_error' })); }); + it('creates a span for invalid looking urls', async () => { + const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction; + hub.getScope().setSpan(transaction); + + try { + // Intentionally add // to the url + // fetch accepts this URL, but throws an error later on + await fetch('http://a-url-that-no-exists.com//'); + } catch (e) { + // ignore + } + + expect(transaction.spanRecorder?.spans.length).toBe(2); + + const span = transaction.spanRecorder?.spans[1]; + expect(span).toEqual(expect.objectContaining({ description: 'GET http://a-url-that-no-exists.com//' })); + expect(span).toEqual(expect.objectContaining({ status: 'internal_error' })); + }); + it('does not create a span for sentry requests', async () => { const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction; hub.getScope().setSpan(transaction); From 6bf5d3aed0a06ce2bfece8efd8e43fb248cd431e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 May 2023 13:36:08 +0200 Subject: [PATCH 09/20] fix(utils): Fail silently if the provided Dsn is invalid (#8121) We don't want to cause app crashes if the SDK gets an invalid DSN. This patch fixes that by allowing `makeDsn` and `dsnFromString` to return `undefined`, which we do if the Dsn is invalid. --- packages/core/src/api.ts | 4 + packages/core/src/baseclient.ts | 8 +- packages/core/src/transports/multiplexed.ts | 12 +- packages/core/test/lib/api.test.ts | 2 +- packages/core/test/lib/base.test.ts | 12 +- .../test/lib/transports/multiplexed.test.ts | 18 +- .../core/test/lib/transports/offline.test.ts | 2 +- packages/nextjs/src/client/tunnelRoute.ts | 3 + .../nextjs/test/utils/tunnelRoute.test.ts | 19 +- packages/node/test/transports/http.test.ts | 4 +- .../unit/util/createReplayEnvelope.test.ts | 2 +- packages/utils/src/dsn.ts | 45 +++-- packages/utils/test/dsn.test.ts | 175 ++++++++++-------- 13 files changed, 191 insertions(+), 115 deletions(-) diff --git a/packages/core/src/api.ts b/packages/core/src/api.ts index 7191871d9ec4..d6c04ea7c350 100644 --- a/packages/core/src/api.ts +++ b/packages/core/src/api.ts @@ -58,6 +58,10 @@ export function getReportDialogEndpoint( }, ): string { const dsn = makeDsn(dsnLike); + if (!dsn) { + return ''; + } + const endpoint = `${getBaseApiEndpoint(dsn)}embed/error-page/`; let encodedOptions = `dsn=${dsnToString(dsn)}`; diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 80be9dc037bd..9a1312a131ac 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -112,16 +112,20 @@ export abstract class BaseClient implements Client { */ protected constructor(options: O) { this._options = options; + if (options.dsn) { this._dsn = makeDsn(options.dsn); + } else { + __DEBUG_BUILD__ && logger.warn('No DSN provided, client will not do anything.'); + } + + if (this._dsn) { const url = getEnvelopeEndpointWithUrlEncodedAuth(this._dsn, options); this._transport = options.transport({ recordDroppedEvent: this.recordDroppedEvent.bind(this), ...options.transportOptions, url, }); - } else { - __DEBUG_BUILD__ && logger.warn('No DSN provided, client will not do anything.'); } } diff --git a/packages/core/src/transports/multiplexed.ts b/packages/core/src/transports/multiplexed.ts index 9f47b5b76542..c7045f4b2686 100644 --- a/packages/core/src/transports/multiplexed.ts +++ b/packages/core/src/transports/multiplexed.ts @@ -51,9 +51,13 @@ export function makeMultiplexedTransport( const fallbackTransport = createTransport(options); const otherTransports: Record = {}; - function getTransport(dsn: string): Transport { + function getTransport(dsn: string): Transport | undefined { if (!otherTransports[dsn]) { - const url = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(dsn)); + const validatedDsn = dsnFromString(dsn); + if (!validatedDsn) { + return undefined; + } + const url = getEnvelopeEndpointWithUrlEncodedAuth(validatedDsn); otherTransports[dsn] = createTransport({ ...options, url }); } @@ -66,7 +70,9 @@ export function makeMultiplexedTransport( return eventFromEnvelope(envelope, eventTypes); } - const transports = matcher({ envelope, getEvent }).map(dsn => getTransport(dsn)); + const transports = matcher({ envelope, getEvent }) + .map(dsn => getTransport(dsn)) + .filter((t): t is Transport => !!t); // If we have no transports to send to, use the fallback transport if (transports.length === 0) { diff --git a/packages/core/test/lib/api.test.ts b/packages/core/test/lib/api.test.ts index 141301878a5d..ecb92d22a064 100644 --- a/packages/core/test/lib/api.test.ts +++ b/packages/core/test/lib/api.test.ts @@ -9,7 +9,7 @@ const dsnPublic = 'https://abc@sentry.io:1234/subpath/123'; const tunnel = 'https://hello.com/world'; const _metadata = { sdk: { name: 'sentry.javascript.browser', version: '12.31.12' } } as ClientOptions['_metadata']; -const dsnPublicComponents = makeDsn(dsnPublic); +const dsnPublicComponents = makeDsn(dsnPublic)!; describe('API', () => { describe('getEnvelopeEndpointWithUrlEncodedAuth', () => { diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 9705f7a6622f..aca8784ad511 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -70,19 +70,19 @@ describe('BaseClient', () => { }); test('allows missing Dsn', () => { - expect.assertions(1); - const options = getDefaultTestClientOptions(); const client = new TestClient(options); expect(client.getDsn()).toBeUndefined(); + expect(client.getTransport()).toBeUndefined(); }); - test('throws with invalid Dsn', () => { - expect.assertions(1); - + test('handles being passed an invalid Dsn', () => { const options = getDefaultTestClientOptions({ dsn: 'abc' }); - expect(() => new TestClient(options)).toThrow(SentryError); + const client = new TestClient(options); + + expect(client.getDsn()).toBeUndefined(); + expect(client.getTransport()).toBeUndefined(); }); }); diff --git a/packages/core/test/lib/transports/multiplexed.test.ts b/packages/core/test/lib/transports/multiplexed.test.ts index f4f0144e045e..2d2dcb5ce46d 100644 --- a/packages/core/test/lib/transports/multiplexed.test.ts +++ b/packages/core/test/lib/transports/multiplexed.test.ts @@ -12,10 +12,10 @@ import { TextEncoder } from 'util'; import { createTransport, getEnvelopeEndpointWithUrlEncodedAuth, makeMultiplexedTransport } from '../../../src'; const DSN1 = 'https://1234@5678.ingest.sentry.io/4321'; -const DSN1_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN1)); +const DSN1_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN1)!); const DSN2 = 'https://5678@1234.ingest.sentry.io/8765'; -const DSN2_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN2)); +const DSN2_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN2)!); const ERROR_EVENT = { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }; const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [ @@ -83,6 +83,20 @@ describe('makeMultiplexedTransport', () => { await transport.send(ERROR_ENVELOPE); }); + it('Falls back to options DSN when a matched DSN is invalid', async () => { + expect.assertions(1); + + const makeTransport = makeMultiplexedTransport( + createTestTransport(url => { + expect(url).toBe(DSN1_URL); + }), + () => ['invalidDsn'], + ); + + const transport = makeTransport({ url: DSN1_URL, ...transportOptions }); + await transport.send(ERROR_ENVELOPE); + }); + it('DSN can be overridden via match callback', async () => { expect.assertions(1); diff --git a/packages/core/test/lib/transports/offline.test.ts b/packages/core/test/lib/transports/offline.test.ts index d7d2ee7a90ae..056d11aac90a 100644 --- a/packages/core/test/lib/transports/offline.test.ts +++ b/packages/core/test/lib/transports/offline.test.ts @@ -38,7 +38,7 @@ const REPLAY_EVENT: ReplayEvent = { replay_type: 'buffer', }; -const DSN = dsnFromString('https://public@dsn.ingest.sentry.io/1337'); +const DSN = dsnFromString('https://public@dsn.ingest.sentry.io/1337')!; const DATA = 'nothing'; diff --git a/packages/nextjs/src/client/tunnelRoute.ts b/packages/nextjs/src/client/tunnelRoute.ts index 81f9f936cc82..6f10b190727a 100644 --- a/packages/nextjs/src/client/tunnelRoute.ts +++ b/packages/nextjs/src/client/tunnelRoute.ts @@ -12,6 +12,9 @@ export function applyTunnelRouteOption(options: BrowserOptions): void { const tunnelRouteOption = globalWithInjectedValues.__sentryRewritesTunnelPath__; if (tunnelRouteOption && options.dsn) { const dsnComponents = dsnFromString(options.dsn); + if (!dsnComponents) { + return; + } const sentrySaasDsnMatch = dsnComponents.host.match(/^o(\d+)\.ingest\.sentry\.io$/); if (sentrySaasDsnMatch) { const orgId = sentrySaasDsnMatch[1]; diff --git a/packages/nextjs/test/utils/tunnelRoute.test.ts b/packages/nextjs/test/utils/tunnelRoute.test.ts index 0e0a6d48f47c..62b2fb28cf9c 100644 --- a/packages/nextjs/test/utils/tunnelRoute.test.ts +++ b/packages/nextjs/test/utils/tunnelRoute.test.ts @@ -11,7 +11,7 @@ beforeEach(() => { }); describe('applyTunnelRouteOption()', () => { - it('should correctly apply `tunnelRoute` option when conditions are met', () => { + it('Correctly applies `tunnelRoute` option when conditions are met', () => { globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route'; const options: any = { dsn: 'https://11111111111111111111111111111111@o2222222.ingest.sentry.io/3333333', @@ -22,7 +22,7 @@ describe('applyTunnelRouteOption()', () => { expect(options.tunnel).toBe('/my-error-monitoring-route?o=2222222&p=3333333'); }); - it('should not apply `tunnelRoute` when DSN is missing', () => { + it("Doesn't apply `tunnelRoute` when DSN is missing", () => { globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route'; const options: any = { // no dsn @@ -33,7 +33,18 @@ describe('applyTunnelRouteOption()', () => { expect(options.tunnel).toBeUndefined(); }); - it("should not apply `tunnelRoute` option when `tunnelRoute` option wasn't injected", () => { + it("Doesn't apply `tunnelRoute` when DSN is invalid", () => { + globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route'; + const options: any = { + dsn: 'invalidDsn', + } as BrowserOptions; + + applyTunnelRouteOption(options); + + expect(options.tunnel).toBeUndefined(); + }); + + it("Doesn't apply `tunnelRoute` option when `tunnelRoute` option wasn't injected", () => { const options: any = { dsn: 'https://11111111111111111111111111111111@o2222222.ingest.sentry.io/3333333', } as BrowserOptions; @@ -43,7 +54,7 @@ describe('applyTunnelRouteOption()', () => { expect(options.tunnel).toBeUndefined(); }); - it('should not apply `tunnelRoute` option when DSN is not a SaaS DSN', () => { + it("Doesn't `tunnelRoute` option when DSN is not a SaaS DSN", () => { globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route'; const options: any = { dsn: 'https://11111111111111111111111111111111@example.com/3333333', diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index 58b2710f1ac5..4b914f234981 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -83,6 +83,9 @@ const defaultOptions = { textEncoder, }; +// empty function to keep test output clean +const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + describe('makeNewHttpTransport()', () => { afterEach(() => { jest.clearAllMocks(); @@ -403,7 +406,6 @@ describe('makeNewHttpTransport()', () => { }); it('should warn if an invalid url is passed', async () => { - const consoleWarnSpy = jest.spyOn(console, 'warn'); const transport = makeNodeTransport({ ...defaultOptions, url: 'invalid url' }); await transport.send(EVENT_ENVELOPE); expect(consoleWarnSpy).toHaveBeenCalledWith( diff --git a/packages/replay/test/unit/util/createReplayEnvelope.test.ts b/packages/replay/test/unit/util/createReplayEnvelope.test.ts index 76f22709b4cb..150da47edf00 100644 --- a/packages/replay/test/unit/util/createReplayEnvelope.test.ts +++ b/packages/replay/test/unit/util/createReplayEnvelope.test.ts @@ -42,7 +42,7 @@ describe('Unit | util | createReplayEnvelope', () => { projectId: '123', protocol: 'https', publicKey: 'abc', - }); + })!; it('creates an envelope for a given Replay event', () => { const envelope = createReplayEnvelope(replayEvent, payloadWithSequence, dsn); diff --git a/packages/utils/src/dsn.ts b/packages/utils/src/dsn.ts index f506a8fcb9be..f149b67a9386 100644 --- a/packages/utils/src/dsn.ts +++ b/packages/utils/src/dsn.ts @@ -1,6 +1,6 @@ import type { DsnComponents, DsnLike, DsnProtocol } from '@sentry/types'; -import { SentryError } from './error'; +import { logger } from './logger'; /** Regular expression used to parse a Dsn. */ const DSN_REGEX = /^(?:(\w+):)\/\/(?:(\w+)(?::(\w+)?)?@)([\w.-]+)(?::(\d+))?\/(.+)/; @@ -30,13 +30,16 @@ export function dsnToString(dsn: DsnComponents, withPassword: boolean = false): * Parses a Dsn from a given string. * * @param str A Dsn as string - * @returns Dsn as DsnComponents + * @returns Dsn as DsnComponents or undefined if @param str is not a valid DSN string */ -export function dsnFromString(str: string): DsnComponents { +export function dsnFromString(str: string): DsnComponents | undefined { const match = DSN_REGEX.exec(str); if (!match) { - throw new SentryError(`Invalid Sentry Dsn: ${str}`); + // This should be logged to the console + // eslint-disable-next-line no-console + console.error(`Invalid Sentry Dsn: ${str}`); + return undefined; } const [protocol, publicKey, pass = '', host, port = '', lastPath] = match.slice(1); @@ -71,38 +74,52 @@ function dsnFromComponents(components: DsnComponents): DsnComponents { }; } -function validateDsn(dsn: DsnComponents): boolean | void { +function validateDsn(dsn: DsnComponents): boolean { if (!__DEBUG_BUILD__) { - return; + return true; } const { port, projectId, protocol } = dsn; const requiredComponents: ReadonlyArray = ['protocol', 'publicKey', 'host', 'projectId']; - requiredComponents.forEach(component => { + const hasMissingRequiredComponent = requiredComponents.find(component => { if (!dsn[component]) { - throw new SentryError(`Invalid Sentry Dsn: ${component} missing`); + logger.error(`Invalid Sentry Dsn: ${component} missing`); + return true; } + return false; }); + if (hasMissingRequiredComponent) { + return false; + } + if (!projectId.match(/^\d+$/)) { - throw new SentryError(`Invalid Sentry Dsn: Invalid projectId ${projectId}`); + logger.error(`Invalid Sentry Dsn: Invalid projectId ${projectId}`); + return false; } if (!isValidProtocol(protocol)) { - throw new SentryError(`Invalid Sentry Dsn: Invalid protocol ${protocol}`); + logger.error(`Invalid Sentry Dsn: Invalid protocol ${protocol}`); + return false; } if (port && isNaN(parseInt(port, 10))) { - throw new SentryError(`Invalid Sentry Dsn: Invalid port ${port}`); + logger.error(`Invalid Sentry Dsn: Invalid port ${port}`); + return false; } return true; } -/** The Sentry Dsn, identifying a Sentry instance and project. */ -export function makeDsn(from: DsnLike): DsnComponents { +/** + * Creates a valid Sentry Dsn object, identifying a Sentry instance and project. + * @returns a valid DsnComponents object or `undefined` if @param from is an invalid DSN source + */ +export function makeDsn(from: DsnLike): DsnComponents | undefined { const components = typeof from === 'string' ? dsnFromString(from) : dsnFromComponents(from); - validateDsn(components); + if (!components || !validateDsn(components)) { + return undefined; + } return components; } diff --git a/packages/utils/test/dsn.test.ts b/packages/utils/test/dsn.test.ts index 3a5bb3e7da6c..3de435bf5fcf 100644 --- a/packages/utils/test/dsn.test.ts +++ b/packages/utils/test/dsn.test.ts @@ -1,11 +1,18 @@ import { dsnToString, makeDsn } from '../src/dsn'; -import { SentryError } from '../src/error'; +import { logger } from '../src/logger'; function testIf(condition: boolean): jest.It { return condition ? test : test.skip; } +const loggerErrorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {}); +const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + describe('Dsn', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + describe('fromComponents', () => { test('applies all components', () => { const dsn = makeDsn({ @@ -16,13 +23,13 @@ describe('Dsn', () => { protocol: 'https', publicKey: 'abc', }); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe('xyz'); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe('1234'); - expect(dsn.path).toBe(''); - expect(dsn.projectId).toBe('123'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe('xyz'); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe('1234'); + expect(dsn?.path).toBe(''); + expect(dsn?.projectId).toBe('123'); }); test('applies partial components', () => { @@ -32,60 +39,62 @@ describe('Dsn', () => { protocol: 'https', publicKey: 'abc', }); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe(''); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe(''); - expect(dsn.path).toBe(''); - expect(dsn.projectId).toBe('123'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe(''); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe(''); + expect(dsn?.path).toBe(''); + expect(dsn?.projectId).toBe('123'); }); - testIf(__DEBUG_BUILD__)('throws for missing components', () => { - expect(() => + testIf(__DEBUG_BUILD__)('returns `undefined` for missing components', () => { + expect( makeDsn({ host: '', projectId: '123', protocol: 'https', publicKey: 'abc', }), - ).toThrow(SentryError); - expect(() => + ).toBeUndefined(); + expect( makeDsn({ host: 'sentry.io', projectId: '', protocol: 'https', publicKey: 'abc', }), - ).toThrow(SentryError); - expect(() => + ).toBeUndefined(); + expect( makeDsn({ host: 'sentry.io', projectId: '123', protocol: '' as 'http', // Trick the type checker here publicKey: 'abc', }), - ).toThrow(SentryError); - expect(() => + ).toBeUndefined(); + expect( makeDsn({ host: 'sentry.io', projectId: '123', protocol: 'https', publicKey: '', }), - ).toThrow(SentryError); + ).toBeUndefined(); + + expect(loggerErrorSpy).toHaveBeenCalledTimes(4); }); - testIf(__DEBUG_BUILD__)('throws for invalid components', () => { - expect(() => + testIf(__DEBUG_BUILD__)('returns `undefined` if components are invalid', () => { + expect( makeDsn({ host: 'sentry.io', projectId: '123', protocol: 'httpx' as 'http', // Trick the type checker here publicKey: 'abc', }), - ).toThrow(SentryError); - expect(() => + ).toBeUndefined(); + expect( makeDsn({ host: 'sentry.io', port: 'xxx', @@ -93,108 +102,114 @@ describe('Dsn', () => { protocol: 'https', publicKey: 'abc', }), - ).toThrow(SentryError); + ).toBeUndefined(); + + expect(loggerErrorSpy).toHaveBeenCalledTimes(2); }); }); describe('fromString', () => { test('parses a valid full Dsn', () => { const dsn = makeDsn('https://abc:xyz@sentry.io:1234/123'); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe('xyz'); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe('1234'); - expect(dsn.path).toBe(''); - expect(dsn.projectId).toBe('123'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe('xyz'); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe('1234'); + expect(dsn?.path).toBe(''); + expect(dsn?.projectId).toBe('123'); }); test('parses a valid partial Dsn', () => { const dsn = makeDsn('https://abc@sentry.io/123/321'); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe(''); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe(''); - expect(dsn.path).toBe('123'); - expect(dsn.projectId).toBe('321'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe(''); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe(''); + expect(dsn?.path).toBe('123'); + expect(dsn?.projectId).toBe('321'); }); test('parses a Dsn with empty password', () => { const dsn = makeDsn('https://abc:@sentry.io/123/321'); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe(''); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe(''); - expect(dsn.path).toBe('123'); - expect(dsn.projectId).toBe('321'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe(''); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe(''); + expect(dsn?.path).toBe('123'); + expect(dsn?.projectId).toBe('321'); }); test('with a long path', () => { const dsn = makeDsn('https://abc@sentry.io/sentry/custom/installation/321'); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe(''); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe(''); - expect(dsn.path).toBe('sentry/custom/installation'); - expect(dsn.projectId).toBe('321'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe(''); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe(''); + expect(dsn?.path).toBe('sentry/custom/installation'); + expect(dsn?.projectId).toBe('321'); }); test('with a query string', () => { const dsn = makeDsn('https://abc@sentry.io/321?sample.rate=0.1&other=value'); - expect(dsn.protocol).toBe('https'); - expect(dsn.publicKey).toBe('abc'); - expect(dsn.pass).toBe(''); - expect(dsn.host).toBe('sentry.io'); - expect(dsn.port).toBe(''); - expect(dsn.path).toBe(''); - expect(dsn.projectId).toBe('321'); + expect(dsn?.protocol).toBe('https'); + expect(dsn?.publicKey).toBe('abc'); + expect(dsn?.pass).toBe(''); + expect(dsn?.host).toBe('sentry.io'); + expect(dsn?.port).toBe(''); + expect(dsn?.path).toBe(''); + expect(dsn?.projectId).toBe('321'); }); - testIf(__DEBUG_BUILD__)('throws when provided invalid Dsn', () => { - expect(() => makeDsn('some@random.dsn')).toThrow(SentryError); + testIf(__DEBUG_BUILD__)('returns undefined when provided invalid Dsn', () => { + expect(makeDsn('some@random.dsn')).toBeUndefined(); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); - testIf(__DEBUG_BUILD__)('throws without mandatory fields', () => { - expect(() => makeDsn('://abc@sentry.io/123')).toThrow(SentryError); - expect(() => makeDsn('https://@sentry.io/123')).toThrow(SentryError); - expect(() => makeDsn('https://abc@123')).toThrow(SentryError); - expect(() => makeDsn('https://abc@sentry.io/')).toThrow(SentryError); + testIf(__DEBUG_BUILD__)('returns undefined if mandatory fields are missing', () => { + expect(makeDsn('://abc@sentry.io/123')).toBeUndefined(); + expect(makeDsn('https://@sentry.io/123')).toBeUndefined(); + expect(makeDsn('https://abc@123')).toBeUndefined(); + expect(makeDsn('https://abc@sentry.io/')).toBeUndefined(); + expect(consoleErrorSpy).toHaveBeenCalledTimes(4); }); - testIf(__DEBUG_BUILD__)('throws for invalid fields', () => { - expect(() => makeDsn('httpx://abc@sentry.io/123')).toThrow(SentryError); - expect(() => makeDsn('httpx://abc@sentry.io:xxx/123')).toThrow(SentryError); - expect(() => makeDsn('http://abc@sentry.io/abc')).toThrow(SentryError); + testIf(__DEBUG_BUILD__)('returns undefined if fields are invalid', () => { + expect(makeDsn('httpx://abc@sentry.io/123')).toBeUndefined(); + expect(makeDsn('httpx://abc@sentry.io:xxx/123')).toBeUndefined(); + expect(makeDsn('http://abc@sentry.io/abc')).toBeUndefined(); + expect(loggerErrorSpy).toHaveBeenCalledTimes(2); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); }); describe('toString', () => { test('excludes the password by default', () => { const dsn = makeDsn('https://abc:xyz@sentry.io:1234/123'); - expect(dsnToString(dsn)).toBe('https://abc@sentry.io:1234/123'); + expect(dsnToString(dsn!)).toBe('https://abc@sentry.io:1234/123'); }); test('optionally includes the password', () => { const dsn = makeDsn('https://abc:xyz@sentry.io:1234/123'); - expect(dsnToString(dsn, true)).toBe('https://abc:xyz@sentry.io:1234/123'); + expect(dsnToString(dsn!, true)).toBe('https://abc:xyz@sentry.io:1234/123'); }); test('renders no password if missing', () => { const dsn = makeDsn('https://abc@sentry.io:1234/123'); - expect(dsnToString(dsn, true)).toBe('https://abc@sentry.io:1234/123'); + expect(dsnToString(dsn!, true)).toBe('https://abc@sentry.io:1234/123'); }); test('renders no port if missing', () => { const dsn = makeDsn('https://abc@sentry.io/123'); - expect(dsnToString(dsn)).toBe('https://abc@sentry.io/123'); + expect(dsnToString(dsn!)).toBe('https://abc@sentry.io/123'); }); test('renders the full path correctly', () => { const dsn = makeDsn('https://abc@sentry.io/sentry/custom/installation/321'); - expect(dsnToString(dsn)).toBe('https://abc@sentry.io/sentry/custom/installation/321'); + expect(dsnToString(dsn!)).toBe('https://abc@sentry.io/sentry/custom/installation/321'); }); }); }); From e52847e87309286ebd3a7ccc449b8615fada4ec2 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 May 2023 13:37:26 +0200 Subject: [PATCH 10/20] fix(sveltekit): Avoid double-wrapping `load` functions (#8094) Applying the `wrap(server)?LoadWithSentry` wrappers multiple times shouldn't lead to double wrapping but instead we should detect if we already wrapped a load function and no-op in this case. This patch adds a flag to the respective load events to detect double wrapping. --- packages/sveltekit/src/client/load.ts | 15 +- packages/sveltekit/src/common/utils.ts | 8 + packages/sveltekit/src/server/load.ts | 26 ++- packages/sveltekit/test/client/load.test.ts | 13 ++ packages/sveltekit/test/server/load.test.ts | 167 +++++++++++--------- 5 files changed, 153 insertions(+), 76 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index aa288b9e2ddd..1996523346e2 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -6,6 +6,7 @@ import { captureException } from '@sentry/svelte'; import type { ClientOptions, SanitizedRequestData } from '@sentry/types'; import { addExceptionMechanism, + addNonEnumerableProperty, getSanitizedUrlString, objectify, parseFetchArgs, @@ -14,8 +15,11 @@ import { } from '@sentry/utils'; import type { LoadEvent } from '@sveltejs/kit'; +import type { SentryWrappedFlag } from '../common/utils'; import { isRedirect } from '../common/utils'; +type PatchedLoadEvent = LoadEvent & Partial; + function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. @@ -66,13 +70,20 @@ export function wrapLoadWithSentry any>(origLoad: T) return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `Load` (see comment above function signature) - const event = args[0] as LoadEvent; + const event = args[0] as PatchedLoadEvent; - const patchedEvent = { + // Check if already wrapped + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + const patchedEvent: PatchedLoadEvent = { ...event, fetch: instrumentSvelteKitFetch(event.fetch), }; + addNonEnumerableProperty(patchedEvent as unknown as Record, '__sentry_wrapped__', true); + const routeId = event.route.id; return trace( { diff --git a/packages/sveltekit/src/common/utils.ts b/packages/sveltekit/src/common/utils.ts index 86035117b6f6..ecf762cee8c4 100644 --- a/packages/sveltekit/src/common/utils.ts +++ b/packages/sveltekit/src/common/utils.ts @@ -1,5 +1,13 @@ import type { Redirect } from '@sveltejs/kit'; +export type SentryWrappedFlag = { + /** + * If this flag is set, we know that the load event was already wrapped once + * and we shouldn't wrap it again. + */ + __sentry_wrapped__?: true; +}; + /** * Determines if a thrown "error" is a Redirect object which SvelteKit users can throw to redirect to another route * see: https://kit.svelte.dev/docs/modules#sveltejs-kit-redirect diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index a5350c6075c2..ab9f9ebdafb3 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -2,12 +2,16 @@ import { trace } from '@sentry/core'; import { captureException } from '@sentry/node'; import type { TransactionContext } from '@sentry/types'; -import { addExceptionMechanism, objectify } from '@sentry/utils'; +import { addExceptionMechanism, addNonEnumerableProperty, objectify } from '@sentry/utils'; import type { HttpError, LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; +import type { SentryWrappedFlag } from '../common/utils'; import { isRedirect } from '../common/utils'; import { getTracePropagationData } from './utils'; +type PatchedLoadEvent = LoadEvent & SentryWrappedFlag; +type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag; + function isHttpError(err: unknown): err is HttpError { return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; } @@ -59,7 +63,15 @@ export function wrapLoadWithSentry any>(origLoad: T) return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `Load` (see comment above function signature) - const event = args[0] as LoadEvent; + // Also, this event possibly already has a sentry wrapped flag attached + const event = args[0] as PatchedLoadEvent; + + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + addNonEnumerableProperty(event as unknown as Record, '__sentry_wrapped__', true); + const routeId = event.route && event.route.id; const traceLoadContext: TransactionContext = { @@ -102,7 +114,15 @@ export function wrapServerLoadWithSentry any>(origSe return new Proxy(origServerLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `ServerLoad` (see comment above function signature) - const event = args[0] as ServerLoadEvent; + // Also, this event possibly already has a sentry wrapped flag attached + const event = args[0] as PatchedServerLoadEvent; + + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + addNonEnumerableProperty(event as unknown as Record, '__sentry_wrapped__', true); + const routeId = event.route && event.route.id; const { dynamicSamplingContext, traceparentData } = getTracePropagationData(event); diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index a6b5ebcbd278..b07e0c12108f 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -450,4 +450,17 @@ describe('wrapLoadWithSentry', () => { { handled: false, type: 'sveltekit', data: { function: 'load' } }, ); }); + + it("doesn't wrap load more than once if the wrapper was applied multiple times", async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } + + const wrappedLoad = wrapLoadWithSentry(wrapLoadWithSentry(load)); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index 4c57d6245451..fa57dc6b7c46 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -48,70 +48,80 @@ function getById(_id?: string) { throw new Error('error'); } -const MOCK_LOAD_ARGS: any = { - params: { id: '123' }, - route: { - id: '/users/[id]', - }, - url: new URL('http://localhost:3000/users/123'), -}; - -const MOCK_LOAD_NO_ROUTE_ARGS: any = { - params: { id: '123' }, - url: new URL('http://localhost:3000/users/123'), -}; - -const MOCK_SERVER_ONLY_LOAD_ARGS: any = { - ...MOCK_LOAD_ARGS, - request: { - method: 'GET', - headers: { - get: (key: string) => { - if (key === 'sentry-trace') { - return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; - } - - if (key === 'baggage') { - return ( - 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' - ); - } - - return null; +function getLoadArgs() { + return { + params: { id: '123' }, + route: { + id: '/users/[id]', + }, + url: new URL('http://localhost:3000/users/123'), + }; +} + +function getLoadArgsWithoutRoute() { + return { + params: { id: '123' }, + url: new URL('http://localhost:3000/users/123'), + }; +} + +function getServerOnlyArgs() { + return { + ...getLoadArgs(), + request: { + method: 'GET', + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + if (key === 'baggage') { + return ( + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' + ); + } + + return null; + }, }, }, - }, -}; - -const MOCK_SERVER_ONLY_NO_TRACE_LOAD_ARGS: any = { - ...MOCK_LOAD_ARGS, - request: { - method: 'GET', - headers: { - get: (_: string) => { - return null; + }; +} + +function getServerArgsWithoutTracingHeaders() { + return { + ...getLoadArgs(), + request: { + method: 'GET', + headers: { + get: (_: string) => { + return null; + }, }, }, - }, -}; - -const MOCK_SERVER_ONLY_NO_BAGGAGE_LOAD_ARGS: any = { - ...MOCK_LOAD_ARGS, - request: { - method: 'GET', - headers: { - get: (key: string) => { - if (key === 'sentry-trace') { - return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; - } - - return null; + }; +} + +function getServerArgsWithoutBaggageHeader() { + return { + ...getLoadArgs(), + request: { + method: 'GET', + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + return null; + }, }, }, - }, -}; + }; +} beforeAll(() => { addTracingExtensions(); @@ -136,7 +146,7 @@ describe.each([ } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad(MOCK_LOAD_ARGS); + const res = wrappedLoad(getLoadArgs()); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(1); @@ -162,7 +172,7 @@ describe.each([ } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ ...MOCK_LOAD_ARGS }); + const res = wrappedLoad(getLoadArgs()); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(times); @@ -170,12 +180,12 @@ describe.each([ }); it("doesn't call captureException for thrown `Redirect`s", async () => { - async function load(_: Parameters[0]): Promise> { + async function load(_params: any): Promise> { throw redirect(300, 'other/route'); } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad(MOCK_LOAD_ARGS); + const res = wrappedLoad(getLoadArgs()); await expect(res).rejects.toThrow(); expect(mockCaptureException).not.toHaveBeenCalled(); @@ -194,7 +204,7 @@ describe.each([ } const wrappedLoad = sentryLoadWrapperFn.call(this, load); - const res = wrappedLoad(MOCK_SERVER_ONLY_LOAD_ARGS); + const res = wrappedLoad(getServerOnlyArgs()); await expect(res).rejects.toThrow(); expect(addEventProcessorSpy).toBeCalledTimes(1); @@ -206,7 +216,7 @@ describe.each([ }); }); describe('wrapLoadWithSentry calls trace', () => { - async function load({ params }: Parameters[0]): Promise> { + async function load({ params }): Promise> { return { post: params.id, }; @@ -214,7 +224,7 @@ describe('wrapLoadWithSentry calls trace', () => { it('with the context of the universal load function', async () => { const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); + await wrappedLoad(getLoadArgs()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -233,7 +243,7 @@ describe('wrapLoadWithSentry calls trace', () => { it('falls back to the raw url if `event.route.id` is not available', async () => { const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_NO_ROUTE_ARGS); + await wrappedLoad(getLoadArgsWithoutRoute()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -249,10 +259,17 @@ describe('wrapLoadWithSentry calls trace', () => { expect.any(Function), ); }); + + it("doesn't wrap load more than once if the wrapper was applied multiple times", async () => { + const wrappedLoad = wrapLoadWithSentry(wrapLoadWithSentry(wrapLoadWithSentry(load))); + await wrappedLoad(getLoadArgs()); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); }); describe('wrapServerLoadWithSentry calls trace', () => { - async function serverLoad({ params }: Parameters[0]): Promise> { + async function serverLoad({ params }): Promise> { return { post: params.id, }; @@ -260,7 +277,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { it('attaches trace data if available', async () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); - await wrappedLoad(MOCK_SERVER_ONLY_LOAD_ARGS); + await wrappedLoad(getServerOnlyArgs()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -294,7 +311,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { it("doesn't attach trace data if it's not available", async () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); - await wrappedLoad(MOCK_SERVER_ONLY_NO_TRACE_LOAD_ARGS); + await wrappedLoad(getServerArgsWithoutTracingHeaders()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -316,7 +333,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { it("doesn't attach the DSC data if the baggage header not available", async () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); - await wrappedLoad(MOCK_SERVER_ONLY_NO_BAGGAGE_LOAD_ARGS); + await wrappedLoad(getServerArgsWithoutBaggageHeader()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -341,7 +358,8 @@ describe('wrapServerLoadWithSentry calls trace', () => { }); it('falls back to the raw url if `event.route.id` is not available', async () => { - const event = { ...MOCK_SERVER_ONLY_LOAD_ARGS }; + const event = getServerOnlyArgs(); + // @ts-ignore - this is fine (just tests here) delete event.route; const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(event); @@ -375,4 +393,11 @@ describe('wrapServerLoadWithSentry calls trace', () => { expect.any(Function), ); }); + + it("doesn't wrap server load more than once if the wrapper was applied multiple times", async () => { + const wrappedLoad = wrapServerLoadWithSentry(wrapServerLoadWithSentry(serverLoad)); + await wrappedLoad(getServerOnlyArgs()); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); }); From aa5286e68c5b38b358127b7fb8c1a80509b24dc7 Mon Sep 17 00:00:00 2001 From: Kev <6111995+k-fish@users.noreply.github.com> Date: Wed, 17 May 2023 07:42:53 -0400 Subject: [PATCH 11/20] fix(tracing): Change where content-length gets added (#8139) Co-authored-by: Abhijeet Prasad --- packages/tracing-internal/src/browser/request.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 94587c0380b1..d4d2edd21b85 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -175,6 +175,13 @@ export function fetchCallback( // TODO (kmclb) remove this once types PR goes through // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access span.setHttpStatus(handlerData.response.status); + + const contentLength = + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + handlerData.response && handlerData.response.headers && handlerData.response.headers.get('content-length'); + if (contentLength > 0) { + span.setData('http.response_content_length', contentLength); + } } else if (handlerData.error) { span.setStatus('internal_error'); } @@ -186,9 +193,6 @@ export function fetchCallback( return; } - const contentLength = - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - handlerData.response && handlerData.response.headers && handlerData.response.headers.get('content-length'); const currentScope = getCurrentHub().getScope(); const currentSpan = currentScope && currentScope.getSpan(); const activeTransaction = currentSpan && currentSpan.transaction; @@ -199,7 +203,6 @@ export function fetchCallback( data: { url, type: 'fetch', - ...(contentLength ? { 'http.response_content_length': contentLength } : {}), 'http.method': method, }, description: `${method} ${url}`, From 8f74eb339c7dd299cbe92d9abd2f0958b83cf6a6 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 17 May 2023 16:56:21 +0200 Subject: [PATCH 12/20] fix(tracing): Use integer for content length (#8152) --- .../test/client/tracingFetch.test.ts | 7 +++++- .../tracing-internal/src/browser/request.ts | 8 ++++--- .../test/browser/request.test.ts | 22 ++++++++++++++----- packages/utils/test/envelope.test.ts | 2 +- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/test/integration/test/client/tracingFetch.test.ts b/packages/nextjs/test/integration/test/client/tracingFetch.test.ts index 7c3256c5e6a7..88634c92012e 100644 --- a/packages/nextjs/test/integration/test/client/tracingFetch.test.ts +++ b/packages/nextjs/test/integration/test/client/tracingFetch.test.ts @@ -31,7 +31,12 @@ test('should correctly instrument `fetch` for performance tracing', async ({ pag expect(transaction[0].spans).toEqual( expect.arrayContaining([ expect.objectContaining({ - data: { 'http.method': 'GET', url: 'http://example.com', type: 'fetch' }, + data: { + 'http.method': 'GET', + url: 'http://example.com', + type: 'fetch', + 'http.response_content_length': expect.any(Number), + }, description: 'GET http://example.com', op: 'http.client', parent_span_id: expect.any(String), diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index d4d2edd21b85..284d8f339435 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -176,11 +176,13 @@ export function fetchCallback( // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access span.setHttpStatus(handlerData.response.status); - const contentLength = + const contentLength: string = // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access handlerData.response && handlerData.response.headers && handlerData.response.headers.get('content-length'); - if (contentLength > 0) { - span.setData('http.response_content_length', contentLength); + + const contentLengthNum = parseInt(contentLength); + if (contentLengthNum > 0) { + span.setData('http.response_content_length', contentLengthNum); } } else if (handlerData.error) { span.setStatus('internal_error'); diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index c677818752cc..ee714e111a8b 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -214,9 +214,8 @@ describe('callbacks', () => { expect(newSpan).toBeUndefined(); }); - it('adds content-length to span data', () => { - const spans = {}; - fetchHandlerData['response'] = { headers: { 'content-length': 123 } }; + it('adds content-length to span data on finish', () => { + const spans: Record = {}; // triggered by request being sent fetchCallback(fetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); @@ -224,8 +223,21 @@ describe('callbacks', () => { const newSpan = transaction.spanRecorder?.spans[1] as Span; expect(newSpan).toBeDefined(); - expect(newSpan).toBeInstanceOf(Span); - expect(newSpan.data).toEqual({ + + const postRequestFetchHandlerData = { + ...fetchHandlerData, + endTimestamp, + response: { status: 404, headers: { get: () => 123 } }, + }; + + // triggered by response coming back + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans); + + const finishedSpan = transaction.spanRecorder?.spans[1] as Span; + + expect(finishedSpan).toBeDefined(); + expect(finishedSpan).toBeInstanceOf(Span); + expect(finishedSpan.data).toEqual({ 'http.response_content_length': 123, 'http.method': 'GET', type: 'fetch', diff --git a/packages/utils/test/envelope.test.ts b/packages/utils/test/envelope.test.ts index 2996b6dcde06..9649da0e2108 100644 --- a/packages/utils/test/envelope.test.ts +++ b/packages/utils/test/envelope.test.ts @@ -36,7 +36,7 @@ describe('envelope', () => { expect(headers).toEqual({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }); }); - it.only('serializes an envelope with attachments', () => { + it('serializes an envelope with attachments', () => { const items: EventEnvelope[1] = [ [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }], [{ type: 'attachment', filename: 'bar.txt', length: 6 }, Uint8Array.from([1, 2, 3, 4, 5, 6])], From 5543808adf70280b0154a4e7c1d446f2b171c07d Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 17 May 2023 15:03:00 -0230 Subject: [PATCH 13/20] feat(replay): Add `beforeAddRecordingEvent` Replay option (#8124) Allows you to modify/filter custom recording events for replays. Note this is only a recording event, not the replay event. Custom means that the events do not relate or affect the DOM recording, but rather they are additional events that the Replay integration adds for additional features. This adds an option for the Replay integration `beforeAddRecordingEvent` to process a recording (rrweb) event before it is added to the event buffer. Example: ```javascript new Sentry.Replay({ beforeAddRecordingEvent: (event) => { // Filter out specific events if (event.data.tag === 'foo') { return null; } // Remember to return an event if you want to keep it! return event; } }); ``` Closes https://github.com/getsentry/sentry-javascript/issues/8127 --- CHANGELOG.md | 2 + packages/replay/src/integration.ts | 3 + packages/replay/src/types.ts | 17 ++ packages/replay/src/util/addEvent.ts | 14 +- .../beforeAddRecordingEvent.test.ts | 145 ++++++++++++++++++ 5 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 packages/replay/test/integration/beforeAddRecordingEvent.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index db0fc8c538b4..3baecec3d8f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- feat(replay): Add `beforeAddRecordingEvent` Replay option + - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott ## 7.52.1 diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index d5d4115fffc7..114d015f2702 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -72,6 +72,8 @@ export class Replay implements Integration { ignore = [], maskFn, + beforeAddRecordingEvent, + // eslint-disable-next-line deprecation/deprecation blockClass, // eslint-disable-next-line deprecation/deprecation @@ -129,6 +131,7 @@ export class Replay implements Integration { networkCaptureBodies, networkRequestHeaders: _getMergedNetworkHeaders(networkRequestHeaders), networkResponseHeaders: _getMergedNetworkHeaders(networkResponseHeaders), + beforeAddRecordingEvent, _experiments, }; diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index dfeeb0982194..ca2c0efbedcc 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -183,6 +183,10 @@ export interface WorkerResponse { export type AddEventResult = void; +export interface BeforeAddRecordingEvent { + (event: RecordingEvent): RecordingEvent | null | undefined; +} + export interface ReplayNetworkOptions { /** * Capture request/response details for XHR/Fetch requests that match the given URLs. @@ -267,6 +271,19 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions { */ maskAllText: boolean; + /** + * Callback before adding a custom recording event + * + * Events added by the underlying DOM recording library can *not* be modified, + * only custom recording events from the Replay integration will trigger the + * callback listeners. This can be used to scrub certain fields in an event (e.g. URLs from navigation events). + * + * Returning a `null` will drop the event completely. Note, dropping a recording + * event is not the same as dropping the replay, the replay will still exist and + * continue to function. + */ + beforeAddRecordingEvent?: BeforeAddRecordingEvent; + /** * _experiments allows users to enable experimental or internal features. * We don't consider such features as part of the public API and hence we don't guarantee semver for them. diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index 8064515c3f87..e40a9c2f6486 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -2,6 +2,7 @@ import { getCurrentHub } from '@sentry/core'; import { logger } from '@sentry/utils'; import type { AddEventResult, RecordingEvent, ReplayContainer } from '../types'; +import { EventType } from '../types/rrweb'; import { timestampToMs } from './timestampToMs'; /** @@ -38,7 +39,18 @@ export async function addEvent( replay.eventBuffer.clear(); } - return await replay.eventBuffer.addEvent(event); + const replayOptions = replay.getOptions(); + + const eventAfterPossibleCallback = + typeof replayOptions.beforeAddRecordingEvent === 'function' && event.type === EventType.Custom + ? replayOptions.beforeAddRecordingEvent(event) + : event; + + if (!eventAfterPossibleCallback) { + return; + } + + return await replay.eventBuffer.addEvent(eventAfterPossibleCallback); } catch (error) { __DEBUG_BUILD__ && logger.error(error); await replay.stop('addEvent'); diff --git a/packages/replay/test/integration/beforeAddRecordingEvent.test.ts b/packages/replay/test/integration/beforeAddRecordingEvent.test.ts new file mode 100644 index 000000000000..c01140045389 --- /dev/null +++ b/packages/replay/test/integration/beforeAddRecordingEvent.test.ts @@ -0,0 +1,145 @@ +import * as SentryCore from '@sentry/core'; +import type { Transport } from '@sentry/types'; +import * as SentryUtils from '@sentry/utils'; + +import type { Replay } from '../../src'; +import type { ReplayContainer } from '../../src/replay'; +import { clearSession } from '../../src/session/clearSession'; +import * as SendReplayRequest from '../../src/util/sendReplayRequest'; +import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; +import { useFakeTimers } from '../utils/use-fake-timers'; + +useFakeTimers(); + +async function advanceTimers(time: number) { + jest.advanceTimersByTime(time); + await new Promise(process.nextTick); +} + +type MockTransportSend = jest.MockedFunction; + +describe('Integration | beforeAddRecordingEvent', () => { + let replay: ReplayContainer; + let integration: Replay; + let mockTransportSend: MockTransportSend; + let mockSendReplayRequest: jest.SpyInstance; + let domHandler: (args: any) => any; + const { record: mockRecord } = mockRrweb(); + + beforeAll(async () => { + jest.setSystemTime(new Date(BASE_TIMESTAMP)); + jest.spyOn(SentryUtils, 'addInstrumentationHandler').mockImplementation((type, handler: (args: any) => any) => { + if (type === 'dom') { + domHandler = handler; + } + }); + + ({ replay, integration } = await mockSdk({ + replayOptions: { + beforeAddRecordingEvent: event => { + const eventData = event.data as Record; + + if (eventData.tag === 'breadcrumb' && eventData.payload.category === 'ui.click') { + return { + ...event, + data: { + ...eventData, + payload: { + ...eventData.payload, + message: 'beforeAddRecordingEvent', + }, + }, + }; + } + + // This should not do anything because callback should not be called + // for `event.type != 5` + if (event.type === 2) { + return null; + } + + if (eventData.tag === 'options') { + return null; + } + + return event; + }, + _experiments: { + captureExceptions: true, + }, + }, + })); + + mockSendReplayRequest = jest.spyOn(SendReplayRequest, 'sendReplayRequest'); + + jest.runAllTimers(); + mockTransportSend = SentryCore.getCurrentHub()?.getClient()?.getTransport()?.send as MockTransportSend; + }); + + beforeEach(() => { + jest.setSystemTime(new Date(BASE_TIMESTAMP)); + mockRecord.takeFullSnapshot.mockClear(); + mockTransportSend.mockClear(); + + // Create a new session and clear mocks because a segment (from initial + // checkout) will have already been uploaded by the time the tests run + clearSession(replay); + replay['_loadAndCheckSession'](); + + mockSendReplayRequest.mockClear(); + }); + + afterEach(async () => { + jest.runAllTimers(); + await new Promise(process.nextTick); + jest.setSystemTime(new Date(BASE_TIMESTAMP)); + clearSession(replay); + replay['_loadAndCheckSession'](); + }); + + afterAll(() => { + integration && integration.stop(); + }); + + it('changes click breadcrumbs message', async () => { + domHandler({ + name: 'click', + }); + + await advanceTimers(5000); + + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + recordingData: JSON.stringify([ + { + type: 5, + timestamp: BASE_TIMESTAMP, + data: { + tag: 'breadcrumb', + payload: { + timestamp: BASE_TIMESTAMP / 1000, + type: 'default', + category: 'ui.click', + message: 'beforeAddRecordingEvent', + data: {}, + }, + }, + }, + ]), + }); + }); + + it('filters out the options event, but *NOT* full snapshot', async () => { + mockTransportSend.mockClear(); + await integration.stop(); + + integration.start(); + + jest.runAllTimers(); + await new Promise(process.nextTick); + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, + recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }]), + }); + }); +}); From a44589a519e2cf42885314491dff41c57da0e0cd Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Fri, 19 May 2023 14:17:40 -0700 Subject: [PATCH 14/20] fix(replays): Show the correct Replay config option name `maskFn` --- packages/replay/README.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/replay/README.md b/packages/replay/README.md index a790370c2d49..091f51d785bf 100644 --- a/packages/replay/README.md +++ b/packages/replay/README.md @@ -195,17 +195,17 @@ The following options can be configured as options to the integration, in `new R The following options can be configured as options to the integration, in `new Replay({})`: -| key | type | default | description | -| ---------------- | ------------------------ | ----------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| maskAllText | boolean | `true` | Mask _all_ text content. Will pass text content through `maskTextFn` before sending to server. | -| maskAllInputs | boolean | `true` | Mask values of `` elements. Passes input values through `maskInputFn` before sending to server. | -| blockAllMedia | boolean | `true` | Block _all_ media elements (`img, svg, video, object, picture, embed, map, audio`) -| maskTextFn | (text: string) => string | `(text) => '*'.repeat(text.length)` | Function to customize how text content is masked before sending to server. By default, masks text with `*`. | -| block | Array | `.sentry-block, [data-sentry-block]` | Redact any elements that match the DOM selectors. See [privacy](#blocking) section for an example. | -| unblock | Array | `.sentry-unblock, [data-sentry-unblock]`| Do not redact any elements that match the DOM selectors. Useful when using `blockAllMedia`. See [privacy](#blocking) section for an example. | -| mask | Array | `.sentry-mask, [data-sentry-mask]` | Mask all elements that match the given DOM selectors. See [privacy](#masking) section for an example. | -| unmask | Array | `.sentry-unmask, [data-sentry-unmask]` | Unmask all elements that match the given DOM selectors. Useful when using `maskAllText`. See [privacy](#masking) section for an example. | -| ignore | Array | `.sentry-ignore, [data-sentry-ignore]` | Ignores all events on the matching input fields. See [privacy](#ignoring) section for an example. | +| key | type | default | description | +| ---------------- | ------------------------ | --------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| maskAllText | boolean | `true` | Mask _all_ text content. Will pass text content through `maskFn` before sending to server. | +| maskAllInputs | boolean | `true` | Mask values of `` elements. Passes input values through `maskInputFn` before sending to server. | +| blockAllMedia | boolean | `true` | Block _all_ media elements (`img, svg, video, object, picture, embed, map, audio`) | +| maskFn | (text: string) => string | `(text) => '*'.repeat(text.length)` | Function to customize how text content is masked before sending to server. By default, masks text with `*`. | +| block | Array | `.sentry-block, [data-sentry-block]` | Redact any elements that match the DOM selectors. See [privacy](#blocking) section for an example. | +| unblock | Array | `.sentry-unblock, [data-sentry-unblock]`| Do not redact any elements that match the DOM selectors. Useful when using `blockAllMedia`. See [privacy](#blocking) section for an example. | +| mask | Array | `.sentry-mask, [data-sentry-mask]` | Mask all elements that match the given DOM selectors. See [privacy](#masking) section for an example. | +| unmask | Array | `.sentry-unmask, [data-sentry-unmask]` | Unmask all elements that match the given DOM selectors. Useful when using `maskAllText`. See [privacy](#masking) section for an example. | +| ignore | Array | `.sentry-ignore, [data-sentry-ignore]` | Ignores all events on the matching input fields. See [privacy](#ignoring) section for an example. | #### Deprecated options In order to streamline our privacy options, the following have been deprecated in favor for the respective options above. From 0354135157bf31e383ae9a5a9650db2b11c65011 Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Fri, 19 May 2023 14:21:33 -0700 Subject: [PATCH 15/20] Fix the type, because `recordOptions` is falling back to Record<>! --- packages/replay/src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/replay/src/types.ts b/packages/replay/src/types.ts index 12900dc22e74..9f8402bb71e0 100644 --- a/packages/replay/src/types.ts +++ b/packages/replay/src/types.ts @@ -330,7 +330,7 @@ export interface ReplayIntegrationPrivacyOptions { /** * A callback function to customize how your text is masked. */ - maskFn?: Pick; + maskFn?: (s: string) => string; } // These are optional for ReplayPluginOptions because the plugin sets default values From ea551c9a40076390fcda7546469f4ac967aa77d7 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 22 May 2023 10:07:58 +0200 Subject: [PATCH 16/20] meta(tracing): Add deprecation notice to `@sentry/tracing` readme (#8165) --- packages/tracing/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/tracing/README.md b/packages/tracing/README.md index 5ba5309906d3..462f2da52ff8 100644 --- a/packages/tracing/README.md +++ b/packages/tracing/README.md @@ -4,6 +4,8 @@

+> ⚠️ **Deprecation Notice:** From SDK versions >= `7.47.0` onwards, the `@sentry/tracing` package is officially deprecated. This package will be removed in a future major release. More details can be found in the [migration docs](https://github.com/getsentry/sentry-javascript/blob/7.47.0/MIGRATION.md/#remove-requirement-for-sentrytracing-package-since-7460). + # Sentry Tracing Extensions [![npm version](https://img.shields.io/npm/v/@sentry/tracing.svg)](https://www.npmjs.com/package/@sentry/tracing) From 3dd4a587d36d60efeec86d30d3f7f607622bf393 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 22 May 2023 10:36:49 +0200 Subject: [PATCH 17/20] fix(nextjs): Guard for non-absolute paths when injecting sentry config (#8151) --- .../nextjs/src/config/loaders/wrappingLoader.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 53891bb298a9..3157d41df71f 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -202,16 +202,13 @@ export default function wrappingLoader( templateCode = templateCode.replace(/__COMPONENT_TYPE__/g, 'Unknown'); } - if (sentryConfigFilePath) { - let importPath = sentryConfigFilePath; - - // absolute paths do not work with Windows - // https://github.com/getsentry/sentry-javascript/issues/8133 - if (path.isAbsolute(importPath)) { - importPath = path.relative(path.dirname(this.resourcePath), importPath); - } - - templateCode = `import "${importPath.replace(/\\/g, '/')}";\n`.concat(templateCode); + // 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; From 6be4e133be7a627d5991b8fa1fef4ef23a20f45e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 22 May 2023 16:54:52 +0200 Subject: [PATCH 18/20] test(nextjs): Unbreak Next.js integration tests (#8179) --- packages/nextjs/test/integration/test/server/utils/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/integration/test/server/utils/helpers.ts b/packages/nextjs/test/integration/test/server/utils/helpers.ts index e483f08a144c..c7213785493f 100644 --- a/packages/nextjs/test/integration/test/server/utils/helpers.ts +++ b/packages/nextjs/test/integration/test/server/utils/helpers.ts @@ -9,7 +9,7 @@ import next from 'next'; // Type not exported from NextJS // @ts-ignore export const createNextServer = async config => { - const app = next(config); + const app = next({ ...config, customServer: false }); // customServer: false because: https://github.com/vercel/next.js/pull/49805#issuecomment-1557321794 const handle = app.getRequestHandler(); await app.prepare(); From ad3547dff0174d51c26a5c5df81d0e6783ee5b82 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 22 May 2023 17:11:56 +0200 Subject: [PATCH 19/20] fix(nextjs): Make `withSentryConfig` isomorphic (#8166) --- packages/nextjs/src/client/index.ts | 7 +++++++ packages/nextjs/src/edge/index.ts | 7 +++++++ packages/nextjs/src/index.types.ts | 2 ++ 3 files changed, 16 insertions(+) diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index d0be07a3c8d0..665b557e4e7b 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -123,6 +123,13 @@ function addClientIntegrations(options: BrowserOptions): void { options.integrations = integrations; } +/** + * Just a passthrough in case this is imported from the client. + */ +export function withSentryConfig(exportedUserNextConfig: T): T { + return exportedUserNextConfig; +} + export { // eslint-disable-next-line deprecation/deprecation withSentryServerSideGetInitialProps, diff --git a/packages/nextjs/src/edge/index.ts b/packages/nextjs/src/edge/index.ts index ca56ce3facbe..8e8edfdc3b07 100644 --- a/packages/nextjs/src/edge/index.ts +++ b/packages/nextjs/src/edge/index.ts @@ -126,6 +126,13 @@ export function lastEventId(): string | undefined { return getCurrentHub().lastEventId(); } +/** + * Just a passthrough in case this is imported from the client. + */ +export function withSentryConfig(exportedUserNextConfig: T): T { + return exportedUserNextConfig; +} + export { flush } from './utils/flush'; export * from '@sentry/core'; diff --git a/packages/nextjs/src/index.types.ts b/packages/nextjs/src/index.types.ts index a75b61e02ea3..6fcf7436399b 100644 --- a/packages/nextjs/src/index.types.ts +++ b/packages/nextjs/src/index.types.ts @@ -39,6 +39,8 @@ export declare const withErrorBoundary: typeof clientSdk.withErrorBoundary; export declare const Span: typeof edgeSdk.Span; export declare const Transaction: typeof edgeSdk.Transaction; +export { withSentryConfig } from './config'; + /** * @deprecated Use `wrapApiHandlerWithSentry` instead */ From 573514aeaa38e7ae9e4a5d8c4084f8c1898d952d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 23 May 2023 10:31:27 +0200 Subject: [PATCH 20/20] meta: Update Changelog for 7.53.0 --- CHANGELOG.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3baecec3d8f9..5a373bbae480 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,27 @@ ## Unreleased -- feat(replay): Add `beforeAddRecordingEvent` Replay option - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 7.53.0 + +- feat(replay): Add `beforeAddRecordingEvent` Replay option (#8124) +- feat(replay): Do not capture replays < 5 seconds (#7949) +- fix(nextjs): Guard for non-absolute paths when injecting sentry config (#8151) +- fix(nextjs): Import path issue on Windows (#8142) +- fix(nextjs): Make `withSentryConfig` isomorphic (#8166) +- fix(node): Add debug logging for node checkin (#8131) +- fix(node): Add LRU map for tracePropagationTargets calculation (#8130) +- fix(node): Remove new URL usage in Undici integration (#8147) +- fix(replay): Show the correct Replay config option name `maskFn` +- fix(sveltekit): Avoid double-wrapping load functions (#8094) +- fix(tracing): Change where content-length gets added (#8139) +- fix(tracing): Use integer for content length (#8152) +- fix(utils): Fail silently if the provided Dsn is invalid (#8121) +- ref(node): Cache undici trace propagation decisions (#8136) +- ref(serverless): Remove relay extension from AWS Layer (#8080) + ## 7.52.1 - feat(replay): Capture slow clicks (experimental) (#8052)