Skip to content

Commit

Permalink
feat(node): allow keepAlive override (#6161)
Browse files Browse the repository at this point in the history
We want to reduce the network pressure during profiling and reuse the connection if possible so that we dont block test execution when profiling tests. This change allows us to leverage http connection keepalive by overriding the default SDK value (false). Since the tests that we profile run on node 18, we should not see any memory leaks and it is probably a good practice to allow an override here anyways.
  • Loading branch information
JonasBa committed Nov 9, 2022
1 parent ccb3ad6 commit c59c70b
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
5 changes: 4 additions & 1 deletion packages/node/src/transports/http.ts
Expand Up @@ -23,6 +23,8 @@ export interface NodeTransportOptions extends BaseTransportOptions {
caCerts?: string | Buffer | Array<string | Buffer>;
/** Custom HTTP module. Defaults to the native 'http' and 'https' modules. */
httpModule?: HTTPModule;
/** Allow overriding connection keepAlive, defaults to false */
keepAlive?: boolean;
}

// Estimated maximum size for reasonable standalone event
Expand Down Expand Up @@ -56,12 +58,13 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport {
);

const nativeHttpModule = isHttps ? https : http;
const keepAlive = options.keepAlive === undefined ? false : options.keepAlive;

// 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
? (new (require('https-proxy-agent'))(proxy) as http.Agent)
: new nativeHttpModule.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 });
: new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 });

const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent);
return createTransport(options, requestExecutor);
Expand Down
14 changes: 14 additions & 0 deletions packages/node/test/transports/http.test.ts
Expand Up @@ -108,6 +108,20 @@ describe('makeNewHttpTransport()', () => {
await transport.send(EVENT_ENVELOPE);
});

it('allows overriding keepAlive', async () => {
await setupTestServer({ statusCode: SUCCESS }, req => {
expect(req.headers).toEqual(
expect.objectContaining({
// node http module lower-cases incoming headers
connection: 'keep-alive',
}),
);
});

const transport = makeNodeTransport({ keepAlive: true, ...defaultOptions });
await transport.send(EVENT_ENVELOPE);
});

it('should correctly send user-provided headers to server', async () => {
await setupTestServer({ statusCode: SUCCESS }, req => {
expect(req.headers).toEqual(
Expand Down

0 comments on commit c59c70b

Please sign in to comment.