Skip to content

Commit 44f54b8

Browse files
author
Luca Forstner
committed
Unify http and https transport
1 parent 47caf0b commit 44f54b8

File tree

5 files changed

+83
-94
lines changed

5 files changed

+83
-94
lines changed

‎packages/node/src/backend.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,7 @@ import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry
33
import { makeDsn, resolvedSyncPromise } from '@sentry/utils';
44

55
import { eventFromMessage, eventFromUnknownInput } from './eventbuilder';
6-
import {
7-
HTTPSTransport,
8-
HTTPTransport,
9-
makeNewHttpsTransport,
10-
makeNewHttpTransport,
11-
NodeTransportOptions,
12-
} from './transports';
6+
import { HTTPSTransport, HTTPTransport, makeNodeTransport, NodeTransportOptions } from './transports';
137
import { NodeOptions } from './types';
148

159
/**
@@ -67,11 +61,11 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
6761
caCerts: transportOptions.caCerts,
6862
};
6963

64+
this._newTransport = makeNodeTransport(newTransportOptions);
65+
7066
if (dsn.protocol === 'http') {
71-
this._newTransport = makeNewHttpTransport(newTransportOptions);
7267
return new HTTPTransport(transportOptions);
7368
}
74-
this._newTransport = makeNewHttpsTransport(newTransportOptions);
7569
return new HTTPSTransport(transportOptions);
7670
}
7771
}

‎packages/node/src/transports/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
export { BaseTransport } from './base';
22
export { HTTPTransport } from './http';
33
export { HTTPSTransport } from './https';
4-
export { makeNewHttpTransport, makeNewHttpsTransport, NodeTransportOptions } from './new';
4+
export { makeNodeTransport, NodeTransportOptions } from './new';

‎packages/node/src/transports/new.ts

Lines changed: 18 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { URL } from 'url';
1414
import { HTTPModule } from './base/http-module';
1515

1616
// TODO(v7):
17-
// - Rename this file "transports.ts"
17+
// - Rename this file "transport.ts"
1818
// - Move this file one folder upwards
1919
// - Delete "transports" folder
2020
// OR
@@ -27,41 +27,30 @@ export interface NodeTransportOptions extends BaseTransportOptions {
2727
proxy?: string;
2828
/** HTTPS proxy CA certificates */
2929
caCerts?: string | Buffer | Array<string | Buffer>;
30-
/** Custom HTTP module */
30+
/** Custom HTTP module. Defaults to the native 'http' and 'https' modules. */
3131
httpModule?: HTTPModule;
3232
}
3333

3434
/**
35-
* Creates a Transport that uses http to send events to Sentry.
35+
* Creates a Transport that uses native the native 'http' and 'https' modules to send events to Sentry.
3636
*/
37-
export function makeNewHttpTransport(options: NodeTransportOptions): NewTransport {
38-
// Proxy prioritization: http => `options.proxy` | `process.env.http_proxy`
39-
const proxy = applyNoProxyOption(options.url, options.proxy || process.env.http_proxy);
37+
export function makeNodeTransport(options: NodeTransportOptions): NewTransport {
38+
const urlSegments = new URL(options.url);
39+
const isHttps = urlSegments.protocol === 'https:';
4040

41-
const httpModule = options.httpModule ?? http;
42-
43-
const agent = proxy
44-
? (new (require('https-proxy-agent'))(proxy) as http.Agent)
45-
: new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 });
46-
47-
const requestExecutor = createRequestExecutor(options, httpModule, agent);
48-
return createTransport({ bufferSize: options.bufferSize }, requestExecutor);
49-
}
50-
51-
/**
52-
* Creates a Transport that uses https to send events to Sentry.
53-
*/
54-
export function makeNewHttpsTransport(options: NodeTransportOptions): NewTransport {
5541
// Proxy prioritization: https => `options.proxy` | `process.env.https_proxy` | `process.env.http_proxy`
56-
const proxy = applyNoProxyOption(options.url, options.proxy || process.env.https_proxy || process.env.http_proxy);
42+
const proxy = applyNoProxyOption(
43+
urlSegments,
44+
options.proxy || (isHttps ? process.env.https_proxy : undefined) || process.env.http_proxy,
45+
);
5746

58-
const httpsModule = options.httpModule ?? https;
47+
const nativeHttpModule = isHttps ? https : http;
5948

6049
const agent = proxy
61-
? (new (require('https-proxy-agent'))(proxy) as https.Agent)
62-
: new https.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 });
50+
? (new (require('https-proxy-agent'))(proxy) as http.Agent)
51+
: new nativeHttpModule.Agent({ keepAlive: true, maxSockets: 30, timeout: 2000 });
6352

64-
const requestExecutor = createRequestExecutor(options, httpsModule, agent);
53+
const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent);
6554
return createTransport({ bufferSize: options.bufferSize }, requestExecutor);
6655
}
6756

@@ -72,16 +61,16 @@ export function makeNewHttpsTransport(options: NodeTransportOptions): NewTranspo
7261
* @param proxy The client configured proxy.
7362
* @returns A proxy the transport should use.
7463
*/
75-
function applyNoProxyOption(transportUrl: string, proxy: string | undefined): string | undefined {
64+
function applyNoProxyOption(transportUrlSegments: URL, proxy: string | undefined): string | undefined {
7665
const { no_proxy } = process.env;
7766

78-
const { host: transportUrlHost, hostname: transportUrlHostname } = new URL(transportUrl);
79-
8067
const urlIsExemptFromProxy =
8168
no_proxy &&
8269
no_proxy
8370
.split(',')
84-
.some(exemption => transportUrlHost.endsWith(exemption) || transportUrlHostname.endsWith(exemption));
71+
.some(
72+
exemption => transportUrlSegments.host.endsWith(exemption) || transportUrlSegments.hostname.endsWith(exemption),
73+
);
8574

8675
if (urlIsExemptFromProxy) {
8776
return undefined;

‎packages/node/test/transports/new/http.test.ts

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { EventEnvelope, EventItem } from '@sentry/types';
33
import { createEnvelope, serializeEnvelope } from '@sentry/utils';
44
import * as http from 'http';
55

6-
import { makeNewHttpTransport } from '../../../src/transports/new';
6+
import { makeNodeTransport } from '../../../src/transports/new';
77

88
jest.mock('@sentry/core', () => {
99
const actualCore = jest.requireActual('@sentry/core');
@@ -48,16 +48,19 @@ function setupTestServer(
4848

4949
res.writeHead(options.statusCode, options.responseHeaders);
5050
res.end();
51+
52+
// also terminate socket because keepalive hangs connection a bit
53+
res.connection.end();
5154
});
5255

53-
testServer.listen(8099);
56+
testServer.listen(18099);
5457

5558
return new Promise(resolve => {
5659
testServer?.on('listening', resolve);
5760
});
5861
}
5962

60-
const TEST_SERVER_URL = 'http://localhost:8099';
63+
const TEST_SERVER_URL = 'http://localhost:18099';
6164

6265
const EVENT_ENVELOPE = createEnvelope<EventEnvelope>({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [
6366
[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem,
@@ -78,7 +81,7 @@ describe('makeNewHttpTransport()', () => {
7881
it('should correctly return successful server response', async () => {
7982
await setupTestServer({ statusCode: SUCCESS });
8083

81-
const transport = makeNewHttpTransport({ url: TEST_SERVER_URL });
84+
const transport = makeNodeTransport({ url: TEST_SERVER_URL });
8285
const transportResponse = await transport.send(EVENT_ENVELOPE);
8386

8487
expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' }));
@@ -90,7 +93,7 @@ describe('makeNewHttpTransport()', () => {
9093
expect(body).toBe(SERIALIZED_EVENT_ENVELOPE);
9194
});
9295

93-
const transport = makeNewHttpTransport({ url: TEST_SERVER_URL });
96+
const transport = makeNodeTransport({ url: TEST_SERVER_URL });
9497
await transport.send(EVENT_ENVELOPE);
9598
});
9699

@@ -105,7 +108,7 @@ describe('makeNewHttpTransport()', () => {
105108
);
106109
});
107110

108-
const transport = makeNewHttpTransport({
111+
const transport = makeNodeTransport({
109112
url: TEST_SERVER_URL,
110113
headers: {
111114
'X-Some-Custom-Header-1': 'value1',
@@ -123,7 +126,7 @@ describe('makeNewHttpTransport()', () => {
123126
])('should correctly reject bad server response (status %i)', async (serverStatusCode, expectedStatus) => {
124127
await setupTestServer({ statusCode: serverStatusCode });
125128

126-
const transport = makeNewHttpTransport({ url: TEST_SERVER_URL });
129+
const transport = makeNodeTransport({ url: TEST_SERVER_URL });
127130
await expect(transport.send(EVENT_ENVELOPE)).rejects.toEqual(expect.objectContaining({ status: expectedStatus }));
128131
});
129132

@@ -136,7 +139,7 @@ describe('makeNewHttpTransport()', () => {
136139
},
137140
});
138141

139-
const transport = makeNewHttpTransport({ url: TEST_SERVER_URL });
142+
const transport = makeNodeTransport({ url: TEST_SERVER_URL });
140143
const transportResponse = await transport.send(EVENT_ENVELOPE);
141144

142145
expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' }));
@@ -151,7 +154,7 @@ describe('makeNewHttpTransport()', () => {
151154
},
152155
});
153156

154-
const transport = makeNewHttpTransport({ url: TEST_SERVER_URL });
157+
const transport = makeNodeTransport({ url: TEST_SERVER_URL });
155158
const transportResponse = await transport.send(EVENT_ENVELOPE);
156159

157160
expect(transportResponse).toEqual(expect.objectContaining({ status: 'success' }));
@@ -160,7 +163,7 @@ describe('makeNewHttpTransport()', () => {
160163

161164
describe('proxy', () => {
162165
it('can be configured through option', () => {
163-
makeNewHttpTransport({
166+
makeNodeTransport({
164167
url: 'http://[email protected]:8989/mysubpath/50622',
165168
proxy: 'http://example.com',
166169
});
@@ -171,7 +174,7 @@ describe('makeNewHttpTransport()', () => {
171174

172175
it('can be configured through env variables option', () => {
173176
process.env.http_proxy = 'http://example.com';
174-
makeNewHttpTransport({
177+
makeNodeTransport({
175178
url: 'http://[email protected]:8989/mysubpath/50622',
176179
});
177180

@@ -182,7 +185,7 @@ describe('makeNewHttpTransport()', () => {
182185

183186
it('client options have priority over env variables', () => {
184187
process.env.http_proxy = 'http://foo.com';
185-
makeNewHttpTransport({
188+
makeNodeTransport({
186189
url: 'http://[email protected]:8989/mysubpath/50622',
187190
proxy: 'http://bar.com',
188191
});
@@ -194,7 +197,7 @@ describe('makeNewHttpTransport()', () => {
194197

195198
it('no_proxy allows for skipping specific hosts', () => {
196199
process.env.no_proxy = 'sentry.io';
197-
makeNewHttpTransport({
200+
makeNodeTransport({
198201
url: 'http://[email protected]:8989/mysubpath/50622',
199202
proxy: 'http://example.com',
200203
});
@@ -208,7 +211,7 @@ describe('makeNewHttpTransport()', () => {
208211
process.env.http_proxy = 'http://example.com:8080';
209212
process.env.no_proxy = 'sentry.io:8989';
210213

211-
makeNewHttpTransport({
214+
makeNodeTransport({
212215
url: 'http://[email protected]:8989/mysubpath/50622',
213216
});
214217

@@ -222,7 +225,7 @@ describe('makeNewHttpTransport()', () => {
222225
process.env.http_proxy = 'http://example.com:8080';
223226
process.env.no_proxy = 'example.com,sentry.io,wat.com:1337';
224227

225-
makeNewHttpTransport({
228+
makeNodeTransport({
226229
url: 'http://[email protected]:8989/mysubpath/50622',
227230
});
228231

@@ -242,7 +245,7 @@ describe('makeNewHttpTransport()', () => {
242245
},
243246
});
244247

245-
makeNewHttpTransport({ url: TEST_SERVER_URL });
248+
makeNodeTransport({ url: TEST_SERVER_URL });
246249
const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1];
247250

248251
const executorResult = registeredRequestExecutor({
@@ -266,7 +269,7 @@ describe('makeNewHttpTransport()', () => {
266269
statusCode: SUCCESS,
267270
});
268271

269-
makeNewHttpTransport({ url: TEST_SERVER_URL });
272+
makeNodeTransport({ url: TEST_SERVER_URL });
270273
const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1];
271274

272275
const executorResult = registeredRequestExecutor({
@@ -294,7 +297,7 @@ describe('makeNewHttpTransport()', () => {
294297
},
295298
});
296299

297-
makeNewHttpTransport({ url: TEST_SERVER_URL });
300+
makeNodeTransport({ url: TEST_SERVER_URL });
298301
const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1];
299302

300303
const executorResult = registeredRequestExecutor({
@@ -322,7 +325,7 @@ describe('makeNewHttpTransport()', () => {
322325
},
323326
});
324327

325-
makeNewHttpTransport({ url: TEST_SERVER_URL });
328+
makeNodeTransport({ url: TEST_SERVER_URL });
326329
const registeredRequestExecutor = (createTransport as jest.Mock).mock.calls[0][1];
327330

328331
const executorResult = registeredRequestExecutor({

0 commit comments

Comments
 (0)