From bd9863b7702abc90c5000571790ee23fe0d9ed7c Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 30 Sep 2019 10:13:41 -0700 Subject: [PATCH] fix: unpin @types/node and account for new http.request signatures (#1120) --- package.json | 2 +- scripts/utils.ts | 4 +- src/plugins/plugin-http.ts | 74 +++++++++++++++++++-------------- src/plugins/plugin-koa.ts | 5 ++- test/plugins/test-trace-http.ts | 66 ++++++++++++++++++++--------- test/test-cls-ah.ts | 10 +++-- 6 files changed, 103 insertions(+), 58 deletions(-) diff --git a/package.json b/package.json index 9512ed841..bd88ad634 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/scripts/utils.ts b/scripts/utils.ts index 33f94a3a7..c33245258 100644 --- a/scripts/utils.ts +++ b/scripts/utils.ts @@ -44,7 +44,7 @@ function promisifyChildProcess(childProcess: ChildProcess): Promise { }); } -export function spawnP(command: string, args?: string[], options?: SpawnOptions): Promise { +export function spawnP(command: string, args: string[] = [], options: SpawnOptions = {}): Promise { const stringifiedCommand = `\`${command}${args ? (' ' + args.join(' ')) : ''}\``; console.log(`> Running: ${stringifiedCommand}`); return promisifyChildProcess(spawn(command, args, Object.assign({ @@ -53,7 +53,7 @@ export function spawnP(command: string, args?: string[], options?: SpawnOptions) }, options))); } -export function forkP(moduleName: string, args?: string[], options?: ForkOptions): Promise { +export function forkP(moduleName: string, args: string[] = [], options: ForkOptions = {}): Promise { const stringifiedCommand = `\`${moduleName}${args ? (' ' + args.join(' ')) : ''}\``; console.log(`> Running: ${stringifiedCommand}`); return promisifyChildProcess(fork(moduleName, args, Object.assign({ diff --git a/src/plugins/plugin-http.ts b/src/plugins/plugin-http.ts index f403a8333..d63c3b06b 100644 --- a/src/plugins/plugin-http.ts +++ b/src/plugins/plugin-http.ts @@ -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'; @@ -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'; } @@ -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 || @@ -61,7 +59,7 @@ function hasExpectHeader(options: ClientRequestArgs | url.URL): boolean { } function extractUrl( - options: ClientRequestArgs | url.URL, + options: ClientRequestArgs | URL, fallbackProtocol: string ) { let path; @@ -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 && @@ -103,9 +101,30 @@ 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 @@ -113,28 +132,21 @@ function makeRequestTrace( // 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 @@ -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; }; @@ -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; }; diff --git a/src/plugins/plugin-koa.ts b/src/plugins/plugin-koa.ts index 7c141f389..c1e87048f 100644 --- a/src/plugins/plugin-koa.ts +++ b/src/plugins/plugin-koa.ts @@ -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; }); diff --git a/test/plugins/test-trace-http.ts b/test/plugins/test-trace-http.ts index a549704fb..c4973dac5 100644 --- a/test/plugins/test-trace-http.ts +++ b/test/plugins/test-trace-http.ts @@ -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'; @@ -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; @@ -122,6 +123,25 @@ for (const nodule of Object.keys(servers) as Array) { 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 () => { @@ -213,22 +233,30 @@ for (const nodule of Object.keys(servers) as Array) { }); 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]); + } + ); } }); diff --git a/test/test-cls-ah.ts b/test/test-cls-ah.ts index fbf56aae7..df73befe0 100644 --- a/test/test-cls-ah.ts +++ b/test/test-cls-ah.ts @@ -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( 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 }; });