Skip to content

Commit

Permalink
ref(node): Partially remove dynamic require calls (#7377)
Browse files Browse the repository at this point in the history 
Remove a few dynamic `require` calls from our Node SDK in favor of making them top-level imports.

* `Http` integration: `http` and `https` are now top-level imports.
* Transport: `https-proxy-agent` is now a top-level import as v5 no longer produces side effects when importing.
 
We need this change for the SvelteKit SDK, as SvelteKit doesn't accept `require` calls in server code by default.
  • Loading branch information
@Lms24
Lms24 authored Mar 8, 2023
1 parent a1dab3b commit 17f31c6
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 49 deletions.
1 change: 0 additions & 1 deletion packages/node/src/declarations.d.ts
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
declare module 'https-proxy-agent';
declare module 'async-limiter';
24 changes: 8 additions & 16 deletions packages/node/src/integrations/http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions packages/node/src/integrations/localvariables.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions packages/node/src/transports/http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
} from '@sentry/types';
import * as http from 'http';
import * as https from 'https';
import { HttpsProxyAgent } from 'https-proxy-agent';
import { Readable } from 'stream';
import { URL } from 'url';
import { createGzip } from 'zlib';
Expand Down Expand Up @@ -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)
? (new HttpsProxyAgent(proxy) as http.Agent)
: new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 });

const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent);
Expand Down
29 changes: 15 additions & 14 deletions packages/node/test/transports/http.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -211,15 +207,20 @@ 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,
url: 'http://[email protected]:8989/mysubpath/50622',
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', () => {
Expand All @@ -229,8 +230,8 @@ describe('makeNewHttpTransport()', () => {
url: 'http://[email protected]: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;
});

Expand All @@ -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;
});

Expand All @@ -255,7 +256,7 @@ describe('makeNewHttpTransport()', () => {
proxy: 'http://example.com',
});

expect(httpProxyAgent).not.toHaveBeenCalled();
expect(proxyAgentSpy).not.toHaveBeenCalled();

delete process.env.no_proxy;
});
Expand All @@ -269,7 +270,7 @@ describe('makeNewHttpTransport()', () => {
url: 'http://[email protected]:8989/mysubpath/50622',
});

expect(httpProxyAgent).not.toHaveBeenCalled();
expect(proxyAgentSpy).not.toHaveBeenCalled();

delete process.env.no_proxy;
delete process.env.http_proxy;
Expand All @@ -284,7 +285,7 @@ describe('makeNewHttpTransport()', () => {
url: 'http://[email protected]:8989/mysubpath/50622',
});

expect(httpProxyAgent).not.toHaveBeenCalled();
expect(proxyAgentSpy).not.toHaveBeenCalled();

delete process.env.no_proxy;
delete process.env.http_proxy;
Expand Down
33 changes: 17 additions & 16 deletions packages/node/test/transports/https.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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)', () => {
Expand All @@ -205,8 +206,8 @@ describe('makeNewHttpsTransport()', () => {
url: 'https://[email protected]: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;
});

Expand All @@ -218,8 +219,8 @@ describe('makeNewHttpsTransport()', () => {
url: 'https://[email protected]: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;
});

Expand All @@ -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;
});

Expand All @@ -246,7 +247,7 @@ describe('makeNewHttpsTransport()', () => {
proxy: 'https://example.com',
});

expect(httpProxyAgent).not.toHaveBeenCalled();
expect(proxyAgentSpy).not.toHaveBeenCalled();

delete process.env.no_proxy;
});
Expand All @@ -261,7 +262,7 @@ describe('makeNewHttpsTransport()', () => {
url: 'https://[email protected]:8989/mysubpath/50622',
});

expect(httpProxyAgent).not.toHaveBeenCalled();
expect(proxyAgentSpy).not.toHaveBeenCalled();

delete process.env.no_proxy;
delete process.env.http_proxy;
Expand All @@ -277,7 +278,7 @@ describe('makeNewHttpsTransport()', () => {
url: 'https://[email protected]:8989/mysubpath/50622',
});

expect(httpProxyAgent).not.toHaveBeenCalled();
expect(proxyAgentSpy).not.toHaveBeenCalled();

delete process.env.no_proxy;
delete process.env.http_proxy;
Expand Down

0 comments on commit 17f31c6

Please sign in to comment.