Skip to content

fix(node): Handle node build without inspector in LocalVariables integration #6780

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 2 commits into from
Jan 16, 2023
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
46 changes: 35 additions & 11 deletions packages/node/src/integrations/localvariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import type {
StackFrame,
StackParser,
} from '@sentry/types';
import type { Debugger, InspectorNotification, Runtime } from 'inspector';
import { Session } from 'inspector';
import type { Debugger, InspectorNotification, Runtime, Session } from 'inspector';
import { LRUMap } from 'lru_map';

export interface DebugSession {
Expand All @@ -28,14 +27,24 @@ export interface DebugSession {
* https://nodejs.org/docs/latest-v19.x/api/inspector.html#promises-api
* https://nodejs.org/docs/latest-v14.x/api/inspector.html
*/
class AsyncSession extends Session implements DebugSession {
class AsyncSession implements DebugSession {
private readonly _session: Session;

/** Throws is inspector API is not available */
public constructor() {
// Node can be build without inspector support so this can throw
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { Session } = require('inspector');
this._session = new Session();
}

/** @inheritdoc */
public configureAndConnect(onPause: (message: InspectorNotification<Debugger.PausedEventDataType>) => void): void {
this.connect();
this.on('Debugger.paused', onPause);
this.post('Debugger.enable');
this._session.connect();
this._session.on('Debugger.paused', onPause);
this._session.post('Debugger.enable');
// We only want to pause on uncaught exceptions
this.post('Debugger.setPauseOnExceptions', { state: 'uncaught' });
this._session.post('Debugger.setPauseOnExceptions', { state: 'uncaught' });
}

/** @inheritdoc */
Expand All @@ -61,7 +70,7 @@ class AsyncSession extends Session implements DebugSession {
*/
private _getProperties(objectId: string): Promise<Runtime.PropertyDescriptor[]> {
return new Promise((resolve, reject) => {
this.post(
this._session.post(
'Runtime.getProperties',
{
objectId,
Expand Down Expand Up @@ -103,6 +112,18 @@ class AsyncSession extends Session implements DebugSession {
}
}

/**
* When using Vercel pkg, the inspector module is not available.
* https://github.com/getsentry/sentry-javascript/issues/6769
*/
function tryNewAsyncSession(): AsyncSession | undefined {
try {
return new AsyncSession();
} catch (e) {
return undefined;
}
}

// Add types for the exception event data
type PausedExceptionEvent = Debugger.PausedEventDataType & {
data: {
Expand Down Expand Up @@ -162,7 +183,10 @@ export class LocalVariables implements Integration {

private readonly _cachedFrames: LRUMap<string, Promise<FrameVariables[]>> = new LRUMap(20);

public constructor(_options: Options = {}, private readonly _session: DebugSession = new AsyncSession()) {}
public constructor(
_options: Options = {},
private readonly _session: DebugSession | undefined = tryNewAsyncSession(),
) {}

/**
* @inheritDoc
Expand All @@ -176,7 +200,7 @@ export class LocalVariables implements Integration {
addGlobalEventProcessor: (callback: EventProcessor) => void,
clientOptions: ClientOptions | undefined,
): void {
if (clientOptions?._experiments?.includeStackLocals) {
if (this._session && clientOptions?._experiments?.includeStackLocals) {
this._session.configureAndConnect(ev =>
this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>),
);
Expand Down Expand Up @@ -212,7 +236,7 @@ export class LocalVariables implements Integration {
return { function: fn };
}

const vars = await this._session.getLocalVariables(localScope.object.objectId);
const vars = await this._session?.getLocalVariables(localScope.object.objectId);

return { function: fn, vars };
});
Expand Down
20 changes: 19 additions & 1 deletion packages/node/test/integrations/localvariables.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,25 @@ describe('LocalVariables', () => {
expect(eventProcessor).toBeUndefined();
});

it.only('Should cache identical uncaught exception events', async () => {
Copy link
Collaborator Author

@timfish timfish Jan 16, 2023

Choose a reason for hiding this comment

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

Ooops, this only shouldn't have been there!

Copy link
Member

Choose a reason for hiding this comment

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

good catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there's a lint for this 🤔

it('Should not initialize when inspector not loaded', async () => {
expect.assertions(1);

const localVariables = new LocalVariables({}, undefined);
const options = getDefaultNodeClientOptions({
stackParser: defaultStackParser,
_experiments: { includeStackLocals: false },
});

let eventProcessor: EventProcessor | undefined;

(localVariables as unknown as LocalVariablesPrivate)._setup(callback => {
eventProcessor = callback;
}, options);

expect(eventProcessor).toBeUndefined();
});

it('Should cache identical uncaught exception events', async () => {
expect.assertions(1);

const session = new MockDebugSession({
Expand Down