Skip to content

Commit

Permalink
fix(profiling): guard from throwing if profiler constructor throws
Browse files Browse the repository at this point in the history
  • Loading branch information
JonasBa committed Mar 3, 2023
1 parent 95a5f48 commit 4d4b39c
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 8 deletions.
53 changes: 48 additions & 5 deletions packages/browser/src/profiling/hubextensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,19 @@ import type { CustomSamplingContext, Hub, Transaction, TransactionContext } from
import { logger, uuid4 } from '@sentry/utils';

import { WINDOW } from '../helpers';
import type { JSSelfProfile, JSSelfProfiler, ProcessedJSSelfProfile } from './jsSelfProfiling';
import type {
JSSelfProfile,
JSSelfProfiler,
JSSelfProfilerConstructor,
ProcessedJSSelfProfile,
} from './jsSelfProfiling';
import { sendProfile } from './sendProfile';

// Max profile duration.
const MAX_PROFILE_DURATION_MS = 30_000;
// Keep a flag value to avoid re-initializing the profiler constructor. If it fails
// once, it will always fail and this allows us to early return.
let PROFILING_CONSTRUCTOR_FAILED = false;

// While we experiment, per transaction sampling interval will be more flexible to work with.
type StartTransaction = (
Expand All @@ -20,7 +28,7 @@ type StartTransaction = (
* Check if profiler constructor is available.
* @param maybeProfiler
*/
function isJSProfilerSupported(maybeProfiler: unknown): maybeProfiler is typeof JSSelfProfiler {
function isJSProfilerSupported(maybeProfiler: unknown): maybeProfiler is typeof JSSelfProfilerConstructor {
return typeof maybeProfiler === 'function';
}

Expand Down Expand Up @@ -49,8 +57,9 @@ export function onProfilingStartRouteTransaction(transaction: Transaction | unde
*/
function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
// Feature support check first
const JSProfiler = WINDOW.Profiler;
if (!isJSProfilerSupported(JSProfiler)) {
const JSProfilerConstructor = WINDOW.Profiler;

if (!isJSProfilerSupported(JSProfilerConstructor)) {
if (__DEBUG_BUILD__) {
logger.log(
'[Profiling] Profiling is not supported by this browser, Profiler interface missing on window object.',
Expand All @@ -67,6 +76,14 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
return transaction;
}

// If constructor failed once, it will always fail, so we can early return.
if (PROFILING_CONSTRUCTOR_FAILED) {
if (__DEBUG_BUILD__) {
logger.log('[Profiling] Profiling has been disabled for the duration of the current user session.');
}
return transaction;
}

const client = getCurrentHub().getClient();
const options = client && client.getOptions();

Expand All @@ -91,7 +108,29 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
const samplingIntervalMS = 10;
// Start the profiler
const maxSamples = Math.floor(MAX_PROFILE_DURATION_MS / samplingIntervalMS);
const profiler = new JSProfiler({ sampleInterval: samplingIntervalMS, maxBufferSize: maxSamples });
let profiler: JSSelfProfiler | undefined;

// Attempt to initialize the profiler constructor, if it fails, we disable profiling for the current user session.
// This is likely due to a missing 'Document-Policy': 'js-profiling' header. We do not want to throw an error if this happens
// as we risk breaking the user's application, so just disable profiling and log an error.
try {
profiler = new JSProfilerConstructor({ sampleInterval: samplingIntervalMS, maxBufferSize: maxSamples });
} catch (e) {
if (__DEBUG_BUILD__) {
logger.log(
"[Profiling] Failed to initialize the Profiling constructor, this is likely due to a missing 'Document-Policy': 'js-profiling' header.",
);
logger.log('[Profiling] Disabling profiling for current user session.');
}
PROFILING_CONSTRUCTOR_FAILED = true;
}

// We failed to construct the profiler, fallback to original transaction - there is no need to log
// anything as we already did that in the try/catch block.
if (!profiler) {
return transaction;
}

if (__DEBUG_BUILD__) {
logger.log(`[Profiling] started profiling transaction: ${transaction.name || transaction.description}`);
}
Expand All @@ -118,6 +157,10 @@ function wrapTransactionWithProfiling(transaction: Transaction): Transaction {
if (!transaction) {
return;
}
// Satisfy the type checker, but profiler will always be defined here.
if (!profiler) {
return;
}
if (processedProfile) {
if (__DEBUG_BUILD__) {
logger.log(
Expand Down
6 changes: 3 additions & 3 deletions packages/browser/src/profiling/jsSelfProfiling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,21 @@ export interface ProcessedJSSelfProfile extends JSSelfProfile {

type BufferFullCallback = (trace: JSSelfProfile) => void;

interface JSSelfProfiler {
export interface JSSelfProfiler {
sampleInterval: number;
stopped: boolean;

stop: () => Promise<JSSelfProfile>;
addEventListener(event: 'samplebufferfull', callback: BufferFullCallback): void;
}

export declare const JSSelfProfiler: {
export declare const JSSelfProfilerConstructor: {
new (options: { sampleInterval: number; maxBufferSize: number }): JSSelfProfiler;
};

declare global {
interface Window {
Profiler: typeof JSSelfProfiler | undefined;
Profiler: typeof JSSelfProfilerConstructor | undefined;
}
}

Expand Down
91 changes: 91 additions & 0 deletions packages/browser/test/unit/profiling/hubextensions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { TextDecoder, TextEncoder } from 'util';
// @ts-ignore patch the encoder on the window, else importing JSDOM fails (deleted in afterAll)
const patchedEncoder = (!global.window.TextEncoder && (global.window.TextEncoder = TextEncoder)) || true;
// @ts-ignore patch the encoder on the window, else importing JSDOM fails (deleted in afterAll)
const patchedDecoder = (!global.window.TextDecoder && (global.window.TextDecoder = TextDecoder)) || true;

import { getCurrentHub } from '@sentry/core';
import type { Transaction } from '@sentry/types';
import { JSDOM } from 'jsdom';

import { onProfilingStartRouteTransaction } from '../../../src';

// @ts-ignore store a reference so we can reset it later
const globalDocument = global.document;
// @ts-ignore store a reference so we can reset it later
const globalWindow = global.window;
// @ts-ignore store a reference so we can reset it later
const globalLocation = global.location;

describe('BrowserProfilingIntegration', () => {
beforeEach(() => {
const dom = new JSDOM();
// @ts-ignore need to override global document
global.document = dom.window.document;
// @ts-ignore need to override global document
global.window = dom.window;
// @ts-ignore need to override global document
global.location = dom.window.location;

const hub = getCurrentHub();
const client: any = {
getDsn() {
return {};
},
getTransport() {
return {
send() {},
};
},
getOptions() {
return {
profilesSampleRate: 1,
}
}
};

hub.bindClient(client);
});

// Reset back to previous values
afterEach(() => {
// @ts-ignore need to override global document
global.document = globalDocument;
// @ts-ignore need to override global document
global.window = globalWindow;
// @ts-ignore need to override global document
global.location = globalLocation;
});
afterAll(() => {
// @ts-ignore patch the encoder on the window, else importing JSDOM fails
patchedEncoder && delete global.window.TextEncoder;
// @ts-ignore patch the encoder on the window, else importing JSDOM fails
patchedDecoder && delete global.window.TextDecoder;
});

it('does not throw if Profiler is not available', () => {
// @ts-ignore force api to be undefined
global.window.Profiler = undefined;
// set sampled to true so that profiling does not early return
const mockTransaction = {sampled: true} as Transaction
expect(() => onProfilingStartRouteTransaction(mockTransaction)).not.toThrow();
});
it('does not throw if constructor throws', () => {
const spy = jest.fn();

class Profiler {
constructor() {
spy();
throw new Error('Profiler constructor error');
}
}

// set sampled to true so that profiling does not early return
const mockTransaction = {sampled: true} as Transaction

// @ts-ignore override with our own constructor
global.window.Profiler = Profiler;
expect(() => onProfilingStartRouteTransaction(mockTransaction)).not.toThrow();
expect(spy).toHaveBeenCalled();
});
});

0 comments on commit 4d4b39c

Please sign in to comment.