Skip to content

feat(browser): Export Replay integration from Browser SDK #6508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
"karma-firefox-launcher": "^1.1.0",
"lerna": "3.13.4",
"madge": "4.0.2",
"magic-string": "^0.27.0",
"mocha": "^6.1.4",
"nodemon": "^2.0.16",
"npm-run-all": "^4.1.5",
Expand Down
1 change: 1 addition & 0 deletions packages/browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
},
"dependencies": {
"@sentry/core": "7.26.0",
"@sentry/replay": "7.26.0",
"@sentry/types": "7.26.0",
"@sentry/utils": "7.26.0",
"tslib": "^1.9.3"
Expand Down
26 changes: 9 additions & 17 deletions packages/browser/src/client.ts
Original file line number Diff line number Diff line change
@@ -1,28 +1,20 @@
import { BaseClient, getEnvelopeEndpointWithUrlEncodedAuth, Scope, SDK_VERSION } from '@sentry/core';
import { ClientOptions, Event, EventHint, Options, Severity, SeverityLevel } from '@sentry/types';
import {
BrowserClientReplayOptions,
ClientOptions,
Event,
EventHint,
Options,
Severity,
SeverityLevel,
} from '@sentry/types';
import { createClientReportEnvelope, dsnToString, logger, serializeEnvelope } from '@sentry/utils';

import { eventFromException, eventFromMessage } from './eventbuilder';
import { WINDOW } from './helpers';
import { Breadcrumbs } from './integrations';
import { BREADCRUMB_INTEGRATION_ID } from './integrations/breadcrumbs';
import { BrowserTransportOptions } from './transports/types';

type BrowserClientReplayOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering: Why are we moving this type into the types package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that @sentry/replay does not need to import anything from @sentry/browser, to avoid circular dependencies.

/**
* The sample rate for session-long replays.
* 1.0 will record all sessions and 0 will record none.
*/
replaysSessionSampleRate?: number;

/**
* The sample rate for sessions that has had an error occur.
* This is independent of `sessionSampleRate`.
* 1.0 will record all sessions and 0 will record none.
*/
replaysOnErrorSampleRate?: number;
};

/**
* Configuration options for the Sentry Browser SDK.
* @see @sentry/types Options for more information.
Expand Down
9 changes: 9 additions & 0 deletions packages/browser/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,12 @@ const INTEGRATIONS = {
};

export { INTEGRATIONS as Integrations };

// DO NOT DELETE THESE COMMENTS!
// We want to exclude Replay from CDN bundles, so we remove the block below with our
// excludeReplay Rollup plugin when generating bundles. Everything between
// ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN and _END__ is removed for bundles.

// __ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__
export { Replay } from '@sentry/replay';
// __ROLLUP_EXCLUDE_FROM_BUNDLES_END__
1 change: 0 additions & 1 deletion packages/replay/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
"homepage": "https://github.com/getsentry/sentry-replay#readme",
"devDependencies": {
"@babel/core": "^7.17.5",
"@sentry/browser": "7.26.0",
"@types/lodash.debounce": "4.0.7",
"@types/pako": "^2.0.0",
"jsdom-worker": "^0.2.1",
Expand Down
6 changes: 3 additions & 3 deletions packages/replay/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { BrowserClient, BrowserOptions } from '@sentry/browser';
import { getCurrentHub } from '@sentry/core';
import type { BrowserClientReplayOptions } from '@sentry/types';
import { Integration } from '@sentry/types';

import { DEFAULT_ERROR_SAMPLE_RATE, DEFAULT_SESSION_SAMPLE_RATE } from './constants';
Expand Down Expand Up @@ -184,8 +184,8 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`,

/** Parse Replay-related options from SDK options */
private _loadReplayOptionsFromClient(): void {
const client = getCurrentHub().getClient() as BrowserClient | undefined;
const opt = client && (client.getOptions() as BrowserOptions | undefined);
const client = getCurrentHub().getClient();
const opt = client && (client.getOptions() as BrowserClientReplayOptions | undefined);

if (opt && typeof opt.replaysSessionSampleRate === 'number') {
this.options.sessionSampleRate = opt.replaysSessionSampleRate;
Expand Down
18 changes: 18 additions & 0 deletions packages/types/src/browseroptions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Options added to the Browser SDK's init options that are specific for Replay.
* Note: This type was moved to @sentry/types to avoid a circular dependency between Browser and Replay.
*/
export type BrowserClientReplayOptions = {
/**
* The sample rate for session-long replays.
* 1.0 will record all sessions and 0 will record none.
*/
replaysSessionSampleRate?: number;

/**
* The sample rate for sessions that has had an error occur.
* This is independent of `sessionSampleRate`.
* 1.0 will record all sessions and 0 will record none.
*/
replaysOnErrorSampleRate?: number;
};
2 changes: 2 additions & 0 deletions packages/types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,5 @@ export type {
export type { User, UserFeedback } from './user';
export type { WrappedFunction } from './wrappedfunction';
export type { Instrumenter } from './instrumenter';

export type { BrowserClientReplayOptions } from './browseroptions';
4 changes: 3 additions & 1 deletion rollup/bundleHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
makeSucrasePlugin,
makeTerserPlugin,
makeTSPlugin,
makeExcludeReplayPlugin,
} from './plugins/index.js';
import { mergePlugins } from './utils';

Expand All @@ -30,6 +31,7 @@ export function makeBaseBundleConfig(options) {
const markAsBrowserBuildPlugin = makeBrowserBuildPlugin(true);
const licensePlugin = makeLicensePlugin(licenseTitle);
const tsPlugin = makeTSPlugin(jsVersion.toLowerCase());
const excludeReplayPlugin = makeExcludeReplayPlugin();

// The `commonjs` plugin is the `esModuleInterop` of the bundling world. When used with `transformMixedEsModules`, it
// will include all dependencies, imported or required, in the final bundle. (Without it, CJS modules aren't included
Expand All @@ -43,7 +45,7 @@ export function makeBaseBundleConfig(options) {
name: 'Sentry',
},
context: 'window',
plugins: [markAsBrowserBuildPlugin],
plugins: [markAsBrowserBuildPlugin, excludeReplayPlugin],
};

// used by `@sentry/integrations` and `@sentry/wasm` (bundles which need to be combined with a stand-alone SDK bundle)
Expand Down
34 changes: 34 additions & 0 deletions rollup/plugins/bundlePlugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
* Typescript plugin docs: https://github.com/ezolenko/rollup-plugin-typescript2
*/

import path from 'path';

import commonjs from '@rollup/plugin-commonjs';
import deepMerge from 'deepmerge';
import license from 'rollup-plugin-license';
import resolve from '@rollup/plugin-node-resolve';
import replace from '@rollup/plugin-replace';
import { terser } from 'rollup-plugin-terser';
import typescript from 'rollup-plugin-typescript2';
import MagicString from 'magic-string';

/**
* Create a plugin to add an identification banner to the top of stand-alone bundles.
Expand Down Expand Up @@ -164,6 +167,37 @@ export function makeTSPlugin(jsVersion) {
return plugin;
}

/**
* Creates a Rollup plugin that removes all code between the `__ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__`
* and `__ROLLUP_EXCLUDE_FROM_BUNDLES_END__` comment guards. This is used to exclude the Replay integration
* from the browser and browser+tracing bundles.
* If we need to add more such guards in the future, we might want to refactor this into a more generic plugin.
*/
export function makeExcludeReplayPlugin() {
const replacementRegex = /\/\/ __ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__(.|\n)*__ROLLUP_EXCLUDE_FROM_BUNDLES_END__/m;
const browserIndexFilePath = path.resolve(__dirname, '../../packages/browser/src/index.ts');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Just a tiny bit worried that if we move this file (or less likely but worse, the place of the guard) that this is gonna break. Thinking can we somehow test for these cases? Or make this more robust.

Honestly, I can't come up with anything obvious that wouldn't be super complicated so I am totally fine with just leaving it as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO the best way to check this is via size checks. What about actually decreasing the allowed size limits to reasonable amounts? E.g. currently we have stuff like 100kb in there for browser. What if we just set this to like "current size + 10kb"? Then tests would actually fail when something becomes too large?

Copy link
Member Author

@Lms24 Lms24 Dec 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea with size-check! I can lower the limits as this should be a good indicator when something goes wrong with the replay export removal.

In terms of this being brittle, yes I agree but then again, we're talking about the main index.ts file which IMO is unlikely to change location or name. Obviously, moving the guard would still be a problem but size-check would let us know about this (fwiw, even without limit adjustments as it failed while I still had a bug in this PR).

If we decide to make this plugin more general at some point, we could pass a "transform files allowlist" or something similar to make changing file entries more easy but for now I'd vote to keep the plugin as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to pull this out into its own PR so we have a better history as to why we made this change: #6543


const plugin = {
transform(code, id) {
const isBrowserIndexFile = path.resolve(id) === browserIndexFilePath;
if (!isBrowserIndexFile || !replacementRegex.test(code)) {
return null;
}

const ms = new MagicString(code);
const transformedCode = ms.replace(replacementRegex, '');
return {
code: transformedCode.toString(),
map: transformedCode.generateMap({ hires: true }),
};
},
};

plugin.name = 'excludeReplay';

return plugin;
}

// We don't pass these plugins any options which need to be calculated or changed by us, so no need to wrap them in
// another factory function, as they are themselves already factory functions.
export { resolve as makeNodeResolvePlugin };
Expand Down
8 changes: 5 additions & 3 deletions rollup/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ export const insertAt = (arr, index, ...insertees) => {
export function mergePlugins(pluginsA, pluginsB) {
const plugins = [...pluginsA, ...pluginsB];
plugins.sort((a, b) => {
// Hacky way to make sure the ones we care about end up where they belong in the order. (Really the TS and sucrase
// Hacky way to make sure the ones we care about end up where they belong in the order. Really the TS and sucrase
// plugins are tied - both should come first - but they're mutually exclusive, so they can come in arbitrary order
// here.)
const order = ['typescript', 'sucrase', '...', 'terser', 'license'];
// here.
// Additionally, the excludeReplay plugin must run before TS/Sucrase so that we can eliminate the replay code
// before anything is type-checked (TS-only) and transpiled.
const order = ['excludeReplay', 'typescript', 'sucrase', '...', 'terser', 'license'];
const sortKeyA = order.includes(a.name) ? a.name : '...';
const sortKeyB = order.includes(b.name) ? b.name : '...';

Expand Down
9 changes: 8 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2232,7 +2232,7 @@
resolved "https://registry.yarnpkg.com/@jridgewell/set-array/-/set-array-1.1.2.tgz#7c6cf998d6d20b914c0a55a91ae928ff25965e72"
integrity sha512-xnkseuNADM0gt2bs+BvhO0p78Mk762YnZdsuzFV018NoG1Sj1SCQvpSqa7XUaTam5vAGasABV9qXASMKnFMwMw==

"@jridgewell/[email protected]", "@jridgewell/sourcemap-codec@^1.4.10":
"@jridgewell/[email protected]", "@jridgewell/sourcemap-codec@^1.4.10", "@jridgewell/sourcemap-codec@^1.4.13":
version "1.4.14"
resolved "https://registry.yarnpkg.com/@jridgewell/sourcemap-codec/-/sourcemap-codec-1.4.14.tgz#add4c98d341472a289190b424efbdb096991bb24"
integrity sha512-XPSJHWmi394fuUuzDnGz1wiKqWfo1yXecHQMRf2l6hztTO+nPru658AyDngaBe7isIxEkRsPR3FZh+s7iVa4Uw==
Expand Down Expand Up @@ -16502,6 +16502,13 @@ magic-string@^0.26.2:
dependencies:
sourcemap-codec "^1.4.8"

magic-string@^0.27.0:
version "0.27.0"
resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.27.0.tgz#e4a3413b4bab6d98d2becffd48b4a257effdbbf3"
integrity sha512-8UnnX2PeRAPZuN12svgR9j7M1uWMovg/CEnIwIG0LFkXSJJe4PdfUGiTGl8V9bsBHFUtfVINcSyYxd7q+kx9fA==
dependencies:
"@jridgewell/sourcemap-codec" "^1.4.13"

make-dir@^1.0.0:
version "1.3.0"
resolved "https://registry.yarnpkg.com/make-dir/-/make-dir-1.3.0.tgz#79c1033b80515bd6d24ec9933e860ca75ee27f0c"
Expand Down