From d239f13591f40606a523c7f085596a75941e77e0 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Mar 2023 12:52:29 +0100 Subject: [PATCH 1/6] ref(node): Remove `require` calls --- packages/node/src/declarations.d.ts | 1 - packages/node/src/integrations/http.ts | 24 +++++--------- .../node/src/integrations/localvariables.ts | 33 +++++++++++++------ packages/node/src/transports/http.ts | 4 +-- 4 files changed, 33 insertions(+), 29 deletions(-) diff --git a/packages/node/src/declarations.d.ts b/packages/node/src/declarations.d.ts index 7d9d77b82bfb..843b79454518 100644 --- a/packages/node/src/declarations.d.ts +++ b/packages/node/src/declarations.d.ts @@ -1,2 +1 @@ -declare module 'https-proxy-agent'; declare module 'async-limiter'; diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index ea7a9eae173d..017929fbe0eb 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -8,8 +8,8 @@ import { parseSemver, stringMatchesSomePattern, } from '@sentry/utils'; -import type * as http from 'http'; -import type * as https from 'https'; +import * as http from 'http'; +import * as https from 'https'; import { LRUMap } from 'lru_map'; import type { NodeClient } from '../client'; @@ -101,25 +101,17 @@ export class Http implements Integration { // and we will no longer have to do this optional merge, we can just pass `this._tracing` directly. const tracingOptions = this._tracing ? { ...clientOptions, ...this._tracing } : undefined; - // eslint-disable-next-line @typescript-eslint/no-var-requires - const httpModule = require('http'); - const wrappedHttpHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, httpModule); - fill(httpModule, 'get', wrappedHttpHandlerMaker); - fill(httpModule, 'request', wrappedHttpHandlerMaker); + const wrappedHttpHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, http); + fill(http, 'get', wrappedHttpHandlerMaker); + fill(http, 'request', wrappedHttpHandlerMaker); // NOTE: Prior to Node 9, `https` used internals of `http` module, thus we don't patch it. // If we do, we'd get double breadcrumbs and double spans for `https` calls. // It has been changed in Node 9, so for all versions equal and above, we patch `https` separately. if (NODE_VERSION.major && NODE_VERSION.major > 8) { - // eslint-disable-next-line @typescript-eslint/no-var-requires - const httpsModule = require('https'); - const wrappedHttpsHandlerMaker = _createWrappedRequestMethodFactory( - this._breadcrumbs, - tracingOptions, - httpsModule, - ); - fill(httpsModule, 'get', wrappedHttpsHandlerMaker); - fill(httpsModule, 'request', wrappedHttpsHandlerMaker); + const wrappedHttpsHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, https); + fill(https, 'get', wrappedHttpsHandlerMaker); + fill(https, 'request', wrappedHttpsHandlerMaker); } } } diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index 2423cfa30c70..110327f69396 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -24,14 +24,16 @@ export interface DebugSession { * https://nodejs.org/docs/latest-v14.x/api/inspector.html */ class AsyncSession implements DebugSession { - private readonly _session: Session; + private _session: Session | undefined = undefined; + + // eslint-disable-next-line @typescript-eslint/consistent-type-imports + private readonly _inspectorModulePromise: Promise; /** 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(); + // @ts-ignore eslint-disable-next-line @typescript-eslint/no-var-requires + this._inspectorModulePromise = import('inspector'); } /** @inheritdoc */ @@ -39,11 +41,18 @@ class AsyncSession implements DebugSession { onPause: (message: InspectorNotification) => void, captureAll: boolean, ): void { - this._session.connect(); - this._session.on('Debugger.paused', onPause); - this._session.post('Debugger.enable'); - // We only want to pause on uncaught exceptions - this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' }); + this._inspectorModulePromise + .then(inspectorModule => { + this._session = new inspectorModule.Session(); + this._session.connect(); + this._session.on('Debugger.paused', onPause); + this._session.post('Debugger.enable'); + // We only want to pause on uncaught exceptions + this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' }); + }) + .catch(_ => { + /* ignoring, `inspector` isn't always available */ + }); } /** @inheritdoc */ @@ -69,6 +78,10 @@ class AsyncSession implements DebugSession { */ private _getProperties(objectId: string): Promise { return new Promise((resolve, reject) => { + if (!this._session) { + reject(new Error('Session is not available')); + return; + } this._session.post( 'Runtime.getProperties', { @@ -192,7 +205,7 @@ export class LocalVariables implements Integration { public constructor( private readonly _options: Options = {}, private readonly _session: DebugSession | undefined = tryNewAsyncSession(), - ) {} + ) { } /** * @inheritDoc diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index a16f78e79a6f..27872dac29db 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -8,6 +8,7 @@ import type { } from '@sentry/types'; import * as http from 'http'; import * as https from 'https'; +import * as createHttpsProxyAgent from 'https-proxy-agent'; import { Readable } from 'stream'; import { URL } from 'url'; import { createGzip } from 'zlib'; @@ -74,8 +75,7 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport { // TODO(v7): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node // versions(>= 8) as they had memory leaks when using it: #2555 const agent = proxy - ? // eslint-disable-next-line @typescript-eslint/no-var-requires - (new (require('https-proxy-agent'))(proxy) as http.Agent) + ? createHttpsProxyAgent(proxy) : new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 }); const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); From b434358e8ec642260fab3db9cbfbf14bfc44e799 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Mar 2023 14:20:41 +0100 Subject: [PATCH 2/6] test log --- packages/node/src/integrations/localvariables.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index 110327f69396..e5b642e3dc82 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -79,6 +79,8 @@ class AsyncSession implements DebugSession { private _getProperties(objectId: string): Promise { return new Promise((resolve, reject) => { if (!this._session) { + // eslint-disable-next-line no-console + console.error('Session is not available'); reject(new Error('Session is not available')); return; } @@ -205,7 +207,7 @@ export class LocalVariables implements Integration { public constructor( private readonly _options: Options = {}, private readonly _session: DebugSession | undefined = tryNewAsyncSession(), - ) { } + ) {} /** * @inheritDoc From 8594b29a2b7a4f5e42afe0ff0c5e6c8d85ad9ed6 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Mar 2023 15:20:53 +0100 Subject: [PATCH 3/6] fix failing tests --- packages/node/src/transports/http.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index 27872dac29db..eb04cc023fa9 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -8,7 +8,7 @@ import type { } from '@sentry/types'; import * as http from 'http'; import * as https from 'https'; -import * as createHttpsProxyAgent from 'https-proxy-agent'; +import * as httpsProxyAgent from 'https-proxy-agent'; import { Readable } from 'stream'; import { URL } from 'url'; import { createGzip } from 'zlib'; @@ -75,7 +75,8 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport { // TODO(v7): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node // versions(>= 8) as they had memory leaks when using it: #2555 const agent = proxy - ? createHttpsProxyAgent(proxy) + ? // @ts-ignore this actually is a constructor but the library exports it super weirdly + (new httpsProxyAgent(proxy) as http.Agent) : new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 }); const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); From ef38b7280270185bcf8fb650c3523e6bb69ab6ab Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Mar 2023 15:21:48 +0100 Subject: [PATCH 4/6] revert LocalVariables changes --- .../node/src/integrations/localvariables.ts | 33 +++++-------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index e5b642e3dc82..2423cfa30c70 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -24,16 +24,14 @@ export interface DebugSession { * https://nodejs.org/docs/latest-v14.x/api/inspector.html */ class AsyncSession implements DebugSession { - private _session: Session | undefined = undefined; - - // eslint-disable-next-line @typescript-eslint/consistent-type-imports - private readonly _inspectorModulePromise: Promise; + private readonly _session: Session; /** Throws is inspector API is not available */ public constructor() { // Node can be build without inspector support so this can throw - // @ts-ignore eslint-disable-next-line @typescript-eslint/no-var-requires - this._inspectorModulePromise = import('inspector'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { Session } = require('inspector'); + this._session = new Session(); } /** @inheritdoc */ @@ -41,18 +39,11 @@ class AsyncSession implements DebugSession { onPause: (message: InspectorNotification) => void, captureAll: boolean, ): void { - this._inspectorModulePromise - .then(inspectorModule => { - this._session = new inspectorModule.Session(); - this._session.connect(); - this._session.on('Debugger.paused', onPause); - this._session.post('Debugger.enable'); - // We only want to pause on uncaught exceptions - this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' }); - }) - .catch(_ => { - /* ignoring, `inspector` isn't always available */ - }); + this._session.connect(); + this._session.on('Debugger.paused', onPause); + this._session.post('Debugger.enable'); + // We only want to pause on uncaught exceptions + this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' }); } /** @inheritdoc */ @@ -78,12 +69,6 @@ class AsyncSession implements DebugSession { */ private _getProperties(objectId: string): Promise { return new Promise((resolve, reject) => { - if (!this._session) { - // eslint-disable-next-line no-console - console.error('Session is not available'); - reject(new Error('Session is not available')); - return; - } this._session.post( 'Runtime.getProperties', { From 99b35016c2739e277efcdeaeaad07ae630f65032 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Mar 2023 15:44:32 +0100 Subject: [PATCH 5/6] fix tests better --- packages/node/src/transports/http.ts | 5 ++-- packages/node/test/transports/http.test.ts | 29 +++++++++--------- packages/node/test/transports/https.test.ts | 33 +++++++++++---------- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index eb04cc023fa9..340bcf4800f0 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -8,7 +8,7 @@ import type { } from '@sentry/types'; import * as http from 'http'; import * as https from 'https'; -import * as httpsProxyAgent from 'https-proxy-agent'; +import { HttpsProxyAgent } from 'https-proxy-agent'; import { Readable } from 'stream'; import { URL } from 'url'; import { createGzip } from 'zlib'; @@ -75,8 +75,7 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport { // TODO(v7): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node // versions(>= 8) as they had memory leaks when using it: #2555 const agent = proxy - ? // @ts-ignore this actually is a constructor but the library exports it super weirdly - (new httpsProxyAgent(proxy) as http.Agent) + ? (new HttpsProxyAgent(proxy) as http.Agent) : new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 }); const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index d03e917c4f42..58b2710f1ac5 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -17,11 +17,7 @@ jest.mock('@sentry/core', () => { }; }); -// eslint-disable-next-line @typescript-eslint/no-var-requires -const httpProxyAgent = require('https-proxy-agent'); -jest.mock('https-proxy-agent', () => { - return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); -}); +import * as httpProxyAgent from 'https-proxy-agent'; const SUCCESS = 200; const RATE_LIMIT = 429; @@ -211,6 +207,11 @@ describe('makeNewHttpTransport()', () => { }); describe('proxy', () => { + const proxyAgentSpy = jest + .spyOn(httpProxyAgent, 'HttpsProxyAgent') + // @ts-ignore + .mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); + it('can be configured through option', () => { makeNodeTransport({ ...defaultOptions, @@ -218,8 +219,8 @@ describe('makeNewHttpTransport()', () => { proxy: 'http://example.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('http://example.com'); }); it('can be configured through env variables option', () => { @@ -229,8 +230,8 @@ describe('makeNewHttpTransport()', () => { url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('http://example.com'); delete process.env.http_proxy; }); @@ -242,8 +243,8 @@ describe('makeNewHttpTransport()', () => { proxy: 'http://bar.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://bar.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('http://bar.com'); delete process.env.http_proxy; }); @@ -255,7 +256,7 @@ describe('makeNewHttpTransport()', () => { proxy: 'http://example.com', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; }); @@ -269,7 +270,7 @@ describe('makeNewHttpTransport()', () => { url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; @@ -284,7 +285,7 @@ describe('makeNewHttpTransport()', () => { url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index d43b8306bed0..e63898a3b11e 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -19,11 +19,7 @@ jest.mock('@sentry/core', () => { }; }); -// eslint-disable-next-line @typescript-eslint/no-var-requires -const httpProxyAgent = require('https-proxy-agent'); -jest.mock('https-proxy-agent', () => { - return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); -}); +import * as httpProxyAgent from 'https-proxy-agent'; const SUCCESS = 200; const RATE_LIMIT = 429; @@ -185,6 +181,11 @@ describe('makeNewHttpsTransport()', () => { }); describe('proxy', () => { + const proxyAgentSpy = jest + .spyOn(httpProxyAgent, 'HttpsProxyAgent') + // @ts-ignore + .mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); + it('can be configured through option', () => { makeNodeTransport({ ...defaultOptions, @@ -193,8 +194,8 @@ describe('makeNewHttpsTransport()', () => { proxy: 'https://example.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com'); }); it('can be configured through env variables option (http)', () => { @@ -205,8 +206,8 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com'); delete process.env.http_proxy; }); @@ -218,8 +219,8 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com'); delete process.env.https_proxy; }); @@ -232,8 +233,8 @@ describe('makeNewHttpsTransport()', () => { proxy: 'https://bar.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://bar.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://bar.com'); delete process.env.https_proxy; }); @@ -246,7 +247,7 @@ describe('makeNewHttpsTransport()', () => { proxy: 'https://example.com', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; }); @@ -261,7 +262,7 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; @@ -277,7 +278,7 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; From adf47c487605ae9fdcfa0ff4cb2445c11123c7ff Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Mar 2023 16:00:00 +0100 Subject: [PATCH 6/6] add TODO note for LocalVariables integration --- packages/node/src/integrations/localvariables.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index 2423cfa30c70..648a9e88d3be 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -28,6 +28,19 @@ class AsyncSession implements DebugSession { /** Throws is inspector API is not available */ public constructor() { + /* + TODO: We really should get rid of this require statement below for a couple of reasons: + 1. It makes the integration unusable in the SvelteKit SDK, as it's not possible to use `require` + in SvelteKit server code (at least not by default). + 2. Throwing in a constructor is bad practice + + More context for a future attempt to fix this: + We already tried replacing it with import but didn't get it to work because of async problems. + We still called import in the constructor but assigned to a promise which we "awaited" in + `configureAndConnect`. However, this broke the Node integration tests as no local variables + were reported any more. We probably missed a place where we need to await the promise, too. + */ + // Node can be build without inspector support so this can throw // eslint-disable-next-line @typescript-eslint/no-var-requires const { Session } = require('inspector');