Skip to content
21 changes: 15 additions & 6 deletions packages/node/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sent
import * as fs from 'fs';
import * as http from 'http';
import * as https from 'https';
import * as url from 'url';
import { URL } from 'url';

import { SDK_NAME } from '../version';

Expand All @@ -30,7 +30,7 @@ export interface HTTPModule {
* @param callback Callback when request is finished
*/
request(
options: http.RequestOptions | https.RequestOptions | string | url.URL,
options: http.RequestOptions | https.RequestOptions | string | URL,
callback?: (res: http.IncomingMessage) => void,
): http.ClientRequest;

Expand All @@ -39,12 +39,15 @@ export interface HTTPModule {
// versions:

// request(
// url: string | url.URL,
// url: string | URL,
// options: http.RequestOptions | https.RequestOptions,
// callback?: (res: http.IncomingMessage) => void,
// ): http.ClientRequest;
}

export type URLParts = Pick<URL, 'hostname' | 'pathname' | 'port' | 'protocol'>;
export type UrlParser = (url: string) => URLParts;

const CATEGORY_MAPPING: {
[key in SentryRequestType]: string;
} = {
Expand All @@ -62,6 +65,9 @@ export abstract class BaseTransport implements Transport {
/** The Agent used for corresponding transport */
public client?: http.Agent | https.Agent;

/** The function used to parse URLs */
public urlParser?: UrlParser;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we actually make this non-optional, and move the default utl => new URL(url) here? Otherwise it may be a breaking change for people using custom transports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kamilogorek before I jump into this rabbit hole, is this what you mean?

diff --git packages/node/src/transports/base.ts packages/node/src/transports/base.ts
index 1b4977de..3413cbb8 100644
--- packages/node/src/transports/base.ts
+++ packages/node/src/transports/base.ts
@@ -65,9 +65,6 @@ export abstract class BaseTransport implements Transport {
   /** The Agent used for corresponding transport */
   public client?: http.Agent | https.Agent;
 
-  /** The function used to parse URLs */
-  public urlParser?: UrlParser;
-
   /** API object */
   protected _api: API;
 
@@ -82,6 +79,9 @@ export abstract class BaseTransport implements Transport {
     this._api = new API(options.dsn, options._metadata);
   }
 
+  /** Default function used to parse URLs */
+  public urlParser: UrlParser = url => new URL(url);
+
   /**
    * @inheritDoc
    */
@@ -230,9 +230,6 @@ export abstract class BaseTransport implements Transport {
         if (!this.module) {
           throw new SentryError('No module available');
         }
-        if (!this.urlParser) {
-          throw new SentryError('No URL parser configured');
-        }
         const options = this._getRequestOptions(this.urlParser(sentryReq.url));
         const req = this.module.request(options, (res: http.IncomingMessage) => {
           const statusCode = res.statusCode || 500;

I also feel like URL parser can belong as an (optional) option in TransportOptions. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, exactly that. Tbh I don't see anyone ever wanting to override this other than use cases like yours, where completely custom transport is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kamilogorek gotcha, done.


/** API object */
protected _api: API;

Expand Down Expand Up @@ -119,12 +125,12 @@ export abstract class BaseTransport implements Transport {
}

/** Returns a build request option object used by request */
protected _getRequestOptions(uri: url.URL): http.RequestOptions | https.RequestOptions {
protected _getRequestOptions(urlParts: URLParts): http.RequestOptions | https.RequestOptions {
const headers = {
...this._api.getRequestHeaders(SDK_NAME, SDK_VERSION),
...this.options.headers,
};
const { hostname, pathname, port, protocol } = uri;
const { hostname, pathname, port, protocol } = urlParts;
// See https://github.com/nodejs/node/blob/38146e717fed2fabe3aacb6540d839475e0ce1c6/lib/internal/url.js#L1268-L1290
// We ignore the query string on purpose
const path = `${pathname}`;
Expand Down Expand Up @@ -224,7 +230,10 @@ export abstract class BaseTransport implements Transport {
if (!this.module) {
throw new SentryError('No module available');
}
const options = this._getRequestOptions(new url.URL(sentryReq.url));
if (!this.urlParser) {
throw new SentryError('No URL parser configured');
}
const options = this._getRequestOptions(this.urlParser(sentryReq.url));
const req = this.module.request(options, (res: http.IncomingMessage) => {
const statusCode = res.statusCode || 500;
const status = Status.fromHttpCode(statusCode);
Expand Down
2 changes: 2 additions & 0 deletions packages/node/src/transports/http.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core';
import { Event, Response, Session, SessionAggregates, TransportOptions } from '@sentry/types';
import * as http from 'http';
import { URL } from 'url';

import { BaseTransport } from './base';

Expand All @@ -10,6 +11,7 @@ export class HTTPTransport extends BaseTransport {
public constructor(public options: TransportOptions) {
super(options);
const proxy = this._getProxy('http');
this.urlParser = url => new URL(url);
this.module = http;
this.client = proxy
? (new (require('https-proxy-agent'))(proxy) as http.Agent)
Expand Down
2 changes: 2 additions & 0 deletions packages/node/src/transports/https.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core';
import { Event, Response, Session, SessionAggregates, TransportOptions } from '@sentry/types';
import * as https from 'https';
import { URL } from 'url';

import { BaseTransport } from './base';

Expand All @@ -10,6 +11,7 @@ export class HTTPSTransport extends BaseTransport {
public constructor(public options: TransportOptions) {
super(options);
const proxy = this._getProxy('https');
this.urlParser = url => new URL(url);
this.module = https;
this.client = proxy
? (new (require('https-proxy-agent'))(proxy) as https.Agent)
Expand Down
34 changes: 34 additions & 0 deletions packages/node/test/transports/custom/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { SentryError } from '@sentry/utils';

import { CustomUrlTransport, NoUrlTransport } from './transports';

describe('Custom transport', () => {
describe('url container support', () => {
const noop = () => null;
const sampleDsn = 'https://[email protected]/path/1';

test("reject with SentryError when a transport doesn't define a URL container", () => {
const transport = new NoUrlTransport({
dsn: sampleDsn,
});

return expect(transport.sendEvent({})).rejects.toEqual(new SentryError('No URL parser configured'));
});

test('use URL property constructor for sendEvent() method', async () => {
const urlParser = jest.fn();
const transport = new CustomUrlTransport({ dsn: sampleDsn }, urlParser);
await transport.sendEvent({}).catch(noop);

expect(urlParser).toHaveBeenCalled();
});

test('use URL property constructor for sendSession() method', async () => {
const urlParser = jest.fn();
const transport = new CustomUrlTransport({ dsn: sampleDsn }, urlParser);
await transport.sendSession({ aggregates: [] }).then(noop, noop);

expect(urlParser).toHaveBeenCalled();
});
});
});
33 changes: 33 additions & 0 deletions packages/node/test/transports/custom/transports.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { eventToSentryRequest } from '@sentry/core';
import { Event, Response, TransportOptions } from '@sentry/types';
import * as http from 'http';

import { BaseTransport, HTTPTransport } from '../../../src/transports';
import { UrlParser } from '../../../src/transports/base';

export class CustomUrlTransport extends HTTPTransport {
public constructor(public options: TransportOptions, urlParser: UrlParser) {
super(options);
this.urlParser = urlParser;
}
}

export class NoUrlTransport extends BaseTransport {
public constructor(public options: TransportOptions) {
super(options);

const proxy = this._getProxy('http');
this.module = http;
this.client = proxy
? (new (require('https-proxy-agent'))(proxy) as http.Agent)
: new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 });
}

public sendEvent(event: Event): Promise<Response> {
return this._send(eventToSentryRequest(event, this._api), event);
}

public sendSession(): Promise<Response> {
throw new Error('NOT_NEEDED');
}
}