Skip to content

Commit

Permalink
fix: unpin @types/node and account for new http.request signatures (#…
Browse files Browse the repository at this point in the history 
…1120)
  • Loading branch information
@kjin
kjin authored Sep 30, 2019
1 parent 41a92f7 commit bd9863b
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 58 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"@types/mocha": "^5.2.5",
"@types/mongoose": "^5.3.26",
"@types/nock": "^10.0.0",
"@types/node": "~10.7.2",
"@types/node": "^12.7.2",
"@types/node-fetch": "^2.5.0",
"@types/once": "^1.4.0",
"@types/proxyquire": "^1.3.28",
Expand Down
4 changes: 2 additions & 2 deletions scripts/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function promisifyChildProcess(childProcess: ChildProcess): Promise<void> {
});
}

export function spawnP(command: string, args?: string[], options?: SpawnOptions): Promise<void> {
export function spawnP(command: string, args: string[] = [], options: SpawnOptions = {}): Promise<void> {
const stringifiedCommand = `\`${command}${args ? (' ' + args.join(' ')) : ''}\``;
console.log(`> Running: ${stringifiedCommand}`);
return promisifyChildProcess(spawn(command, args, Object.assign({
Expand All @@ -53,7 +53,7 @@ export function spawnP(command: string, args?: string[], options?: SpawnOptions)
}, options)));
}

export function forkP(moduleName: string, args?: string[], options?: ForkOptions): Promise<void> {
export function forkP(moduleName: string, args: string[] = [], options: ForkOptions = {}): Promise<void> {
const stringifiedCommand = `\`${moduleName}${args ? (' ' + args.join(' ')) : ''}\``;
console.log(`> Running: ${stringifiedCommand}`);
return promisifyChildProcess(fork(moduleName, args, Object.assign({
Expand Down
74 changes: 43 additions & 31 deletions src/plugins/plugin-http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,11 @@
*/

import * as httpModule from 'http';
import {Agent, ClientRequest, ClientRequestArgs, get, request} from 'http';
import {Agent, ClientRequest, ClientRequestArgs, request} from 'http';
import * as httpsModule from 'https';
import * as is from 'is';
import * as semver from 'semver';
import * as shimmer from 'shimmer';
import * as url from 'url';
import {URL, parse as urlParse} from 'url';

import {Plugin, Tracer} from '../plugin-types';

Expand All @@ -31,16 +30,15 @@ type RequestFunction = typeof request;
const ERR_HTTP_HEADERS_SENT = 'ERR_HTTP_HEADERS_SENT';
const ERR_HTTP_HEADERS_SENT_MSG = "Can't set headers after they are sent.";

// tslint:disable:no-any
const isString = is.string as (value: any) => value is string;
// url.URL is used for type checking, but doesn't exist in Node <7.
// URL is used for type checking, but doesn't exist in Node <7.
// This function works around that.
// tslint:disable:no-any
const isURL = semver.satisfies(process.version, '>=7')
? (value: any): value is url.URL => value instanceof url.URL
: (value: any): value is url.URL => false;
? (value: any): value is URL => value instanceof URL
: (value: any): value is URL => false;
// tslint:enable:no-any

function getSpanName(options: ClientRequestArgs | url.URL) {
function getSpanName(options: ClientRequestArgs | URL) {
// c.f. _http_client.js ClientRequest constructor
return options.hostname || options.host || 'localhost';
}
Expand All @@ -51,7 +49,7 @@ function getSpanName(options: ClientRequestArgs | url.URL) {
* all-caps for simplicity purposes.
* @param options Options for http.request.
*/
function hasExpectHeader(options: ClientRequestArgs | url.URL): boolean {
function hasExpectHeader(options: ClientRequestArgs | URL): boolean {
return !!(
(options as ClientRequestArgs).headers &&
((options as ClientRequestArgs).headers!.Expect ||
Expand All @@ -61,7 +59,7 @@ function hasExpectHeader(options: ClientRequestArgs | url.URL): boolean {
}

function extractUrl(
options: ClientRequestArgs | url.URL,
options: ClientRequestArgs | URL,
fallbackProtocol: string
) {
let path;
Expand All @@ -88,7 +86,7 @@ function extractUrl(
}

// tslint:disable-next-line:no-any
function isTraceAgentRequest(options: any, api: Tracer) {
function isTraceAgentRequest(options: httpModule.RequestOptions, api: Tracer) {
return (
options &&
options.headers &&
Expand All @@ -103,38 +101,52 @@ function makeRequestTrace(
): RequestFunction {
// On Node 8+ we use the following function to patch both request and get.
// Here `request` may also happen to be `get`.
return function requestTrace(options, callback): ClientRequest {
if (!options) {
return request(options, callback);
return function requestTrace(
this: never,
url: httpModule.RequestOptions | string | URL,
options?:
| httpModule.RequestOptions
| ((res: httpModule.IncomingMessage) => void),
callback?: (res: httpModule.IncomingMessage) => void
): ClientRequest {
// These are error conditions; defer to http.request and don't trace.
if (!url || (typeof url === 'object' && typeof options === 'object')) {
return request.apply(this, arguments);
}

let urlString;
if (typeof url === 'string') {
// save the value of uri so we don't have to reconstruct it later
urlString = url;
url = urlParse(url);
}
if (typeof options === 'function') {
callback = options;
options = url;
} else {
options = Object.assign({}, url, options);
}

// Don't trace ourselves lest we get into infinite loops
// Note: this would not be a problem if we guarantee buffering
// of trace api calls. If there is no buffering then each trace is
// an http call which will get a trace which will be an http call
if (isTraceAgentRequest(options, api)) {
return request(options, callback);
}

let uri;
if (isString(options)) {
// save the value of uri so we don't have to reconstruct it later
uri = options;
options = url.parse(options);
return request.apply(this, arguments);
}

const span = api.createChildSpan({name: getSpanName(options)});
if (!api.isRealSpan(span)) {
return request(options, callback);
return request.apply(this, arguments);
}

if (!uri) {
uri = extractUrl(options, protocol);
if (!urlString) {
urlString = extractUrl(options, protocol);
}

const method = (options as ClientRequestArgs).method || 'GET';
span.addLabel(api.labels.HTTP_METHOD_LABEL_KEY, method);
span.addLabel(api.labels.HTTP_URL_LABEL_KEY, uri);
span.addLabel(api.labels.HTTP_URL_LABEL_KEY, urlString);

// If outgoing request headers contain the "Expect" header, the returned
// ClientRequest will throw an error if any new headers are added. For this
Expand Down Expand Up @@ -239,8 +251,8 @@ function patchHttp(http: HttpModule, api: Tracer) {
// v9), so we simply follow the latter.
// Ref:
// https://nodejs.org/dist/latest/docs/api/http.html#http_http_get_options_callback
return function getTrace(options, callback) {
const req = http.request(options, callback);
return function getTrace(this: never) {
const req = http.request.apply(this, arguments);
req.end();
return req;
};
Expand All @@ -255,8 +267,8 @@ function patchHttps(https: HttpsModule, api: Tracer) {
return makeRequestTrace('https:', request, api);
});
shimmer.wrap(https, 'get', function getWrap(): typeof httpsModule.get {
return function getTrace(options, callback) {
const req = https.request(options, callback);
return function getTrace(this: never) {
const req = https.request.apply(this, arguments);
req.end();
return req;
};
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/plugin-koa.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ function createMiddleware(api: PluginTypes.Tracer): koa_1.Middleware {
return function* middleware(this: koa_1.Context, next: IterableIterator<{}>) {
next = startSpanForRequest(api, this, (propagateContext: boolean) => {
if (propagateContext) {
next.next = api.wrap(next.next);
// TS Iterator definition clashes with @types/node.
// For some reason, this causes the next line to not pass type check.
// tslint:disable-next-line:no-any
next.next = api.wrap(next.next as any);
}
return next;
});
Expand Down
66 changes: 47 additions & 19 deletions test/plugins/test-trace-http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import * as httpModule from 'http';
import * as httpsModule from 'https';
import * as stream from 'stream';
import {URL} from 'url';
import * as semver from 'semver';

import {Constants} from '../../src/constants';
import {SpanKind, TraceSpan} from '../../src/trace';
Expand All @@ -31,11 +32,11 @@ import {Express4Secure} from '../web-frameworks/express-secure';

// This type describes (http|https).(get|request).
type HttpRequest = (
options:
| string
url: string | httpModule.RequestOptions | httpsModule.RequestOptions | URL,
options?:
| httpModule.RequestOptions
| httpsModule.RequestOptions
| URL,
| ((res: httpModule.IncomingMessage) => void),
callback?: (res: httpModule.IncomingMessage) => void
) => httpModule.ClientRequest;

Expand Down Expand Up @@ -122,6 +123,25 @@ for (const nodule of Object.keys(servers) as Array<keyof typeof servers>) {
await waitForResponse.done;
},
},
{
description: 'calling http.get with both url and options',
fn: async () => {
const waitForResponse = new WaitForResponse();
http.get(
'http://foo', // Fake hostname
{
protocol: `${nodule}:`,
hostname: 'localhost',
port,
rejectUnauthorized: false,
},
waitForResponse.handleResponse
);
await waitForResponse.done;
},
// http(url, options, cb) is not a recognized signature in Node 8.
versions: '>=10.x',
},
{
description: 'calling http.get and using return value',
fn: async () => {
Expand Down Expand Up @@ -213,22 +233,30 @@ for (const nodule of Object.keys(servers) as Array<keyof typeof servers>) {
});

for (const testCase of testCases) {
it(`creates spans with accurate timespans when ${testCase.description}`, async () => {
let recordedTime = 0;
await testTraceModule
.get()
.runInRootSpan({name: 'outer'}, async rootSpan => {
assert.ok(testTraceModule.get().isRealSpan(rootSpan));
recordedTime = Date.now();
await testCase.fn();
recordedTime = Date.now() - recordedTime;
rootSpan.endSpan();
});
const clientSpan = testTraceModule.getOneSpan(
span => span.kind === 'RPC_CLIENT'
);
assertSpanDuration(clientSpan, [recordedTime]);
});
const maybeIt =
!testCase.versions ||
semver.satisfies(process.version, testCase.versions)
? it
: it.skip;
maybeIt(
`creates spans with accurate timespans when ${testCase.description}`,
async () => {
let recordedTime = 0;
await testTraceModule
.get()
.runInRootSpan({name: 'outer'}, async rootSpan => {
assert.ok(testTraceModule.get().isRealSpan(rootSpan));
recordedTime = Date.now();
await testCase.fn();
recordedTime = Date.now() - recordedTime;
rootSpan.endSpan();
});
const clientSpan = testTraceModule.getOneSpan(
span => span.kind === 'RPC_CLIENT'
);
assertSpanDuration(clientSpan, [recordedTime]);
}
);
}
});

Expand Down
10 changes: 6 additions & 4 deletions test/test-cls-ah.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,29 +35,31 @@ maybeSkip(describe)('AsyncHooks-based CLS', () => {
before(() => {
asyncHooks = require('async_hooks') as AsyncHooksModule;
AsyncResource = class extends asyncHooks.AsyncResource {
// Polyfill for versions in which runInAsyncScope isn't defined.
// This can be removed when it's guaranteed that runInAsyncScope is
// present on AsyncResource.
// tslint:disable:no-any
runInAsyncScope<This, Result>(
fn: (this: This, ...args: any[]) => Result,
thisArg?: This
): Result {
// tslint:enable:no-any
// Polyfill for versions in which runInAsyncScope isn't defined
if (super.runInAsyncScope) {
return super.runInAsyncScope.apply(this, arguments);
} else {
// tslint:disable:deprecation
this.emitBefore();
(this as any).emitBefore();
try {
return fn.apply(
thisArg,
Array.prototype.slice.apply(arguments).slice(2)
);
} finally {
this.emitAfter();
(this as any).emitAfter();
}
// tslint:enable:deprecation
}
}
// tslint:enable:no-any
};
});

Expand Down

0 comments on commit bd9863b

Please sign in to comment.