Skip to content
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

ref(node): Partially remove dynamic require calls #7377

Merged
merged 6 commits into from
Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/node/src/declarations.d.ts
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
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
33 changes: 23 additions & 10 deletions packages/node/src/integrations/localvariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,35 @@ 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<typeof import('inspector')>;

/** 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 */
public configureAndConnect(
onPause: (message: InspectorNotification<Debugger.PausedEventDataType>) => 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
Copy link
Member

Choose a reason for hiding this comment

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

This seems more elegant/"safe" than throwing in the constructor anyhow to me 👍

.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 */
Expand All @@ -69,6 +78,10 @@ class AsyncSession implements DebugSession {
*/
private _getProperties(objectId: string): Promise<Runtime.PropertyDescriptor[]> {
return new Promise((resolve, reject) => {
if (!this._session) {
reject(new Error('Session is not available'));
return;
}
this._session.post(
'Runtime.getProperties',
{
Expand Down Expand Up @@ -192,7 +205,7 @@ export class LocalVariables implements Integration {
public constructor(
private readonly _options: Options = {},
private readonly _session: DebugSession | undefined = tryNewAsyncSession(),
) {}
) { }

/**
* @inheritDoc
Expand Down
4 changes: 2 additions & 2 deletions packages/node/src/transports/http.ts
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 * as createHttpsProxyAgent from 'https-proxy-agent';
mydea marked this conversation as resolved.
Show resolved Hide resolved
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)
? createHttpsProxyAgent(proxy)
: new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 });

const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent);
Expand Down