From 88730577d8d997896e5a430dbd3a8157c15eb68a Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Fri, 23 Sep 2022 10:26:40 +0300 Subject: [PATCH] test: fix tests for restify >=5 (#1191) * fix(restify): upgrade package to fix type problem * fix(restify): updated so types are not leaking through #1132 * fix(restify): lint fix * fix(restify): final fix up for linter * style: avoid checking in manually edited package.json * test: require restify again for every test * test: differenciate between thorwing and failing gracefully * test: use custom error * feat: support restify@5 * feat: support restify@7 * feat: support restify@8 * test: cleanup * test: anyfy arguments on the anonymous handler * chore: add tav + update supported versions * fix: turn off tests on node@18 which is not supported for restify * refactor: remove the import to after setting up instrumentation Co-authored-by: jscherer92 --- .github/workflows/unit-test.yml | 2 +- .../.tav.yml | 3 + .../README.md | 2 +- .../package.json | 18 +++-- .../src/constants.ts | 2 +- .../src/instrumentation.ts | 20 +++--- .../src/types.ts | 2 +- .../test/restify.test.ts | 72 ++++++++++++++----- 8 files changed, 81 insertions(+), 40 deletions(-) create mode 100644 plugins/node/opentelemetry-instrumentation-restify/.tav.yml diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index a3f83977c04..75048b0277c 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -11,11 +11,11 @@ jobs: matrix: node: ["14", "16", "18"] include: - # tests fail on node@18, incompatibility with nock? - node: "18" lerna-extra-args: >- --ignore @opentelemetry/resource-detector-alibaba-cloud --ignore @opentelemetry/instrumentation-fastify + --ignore @opentelemetry/instrumentation-restify runs-on: ubuntu-latest services: memcached: diff --git a/plugins/node/opentelemetry-instrumentation-restify/.tav.yml b/plugins/node/opentelemetry-instrumentation-restify/.tav.yml new file mode 100644 index 00000000000..62556552fbc --- /dev/null +++ b/plugins/node/opentelemetry-instrumentation-restify/.tav.yml @@ -0,0 +1,3 @@ +restify: + versions: "4.3.4 || 5.2.0 || 6.4.0 || 7.7.0 || ^8.4.0" + commands: npm run test diff --git a/plugins/node/opentelemetry-instrumentation-restify/README.md b/plugins/node/opentelemetry-instrumentation-restify/README.md index 00ddb638e47..9ef35ea4da1 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/README.md +++ b/plugins/node/opentelemetry-instrumentation-restify/README.md @@ -17,7 +17,7 @@ npm install --save @opentelemetry/instrumentation-restify ### Supported Versions -- `>=4.0.0` +- `>=4.0.0 <9` ## Usage diff --git a/plugins/node/opentelemetry-instrumentation-restify/package.json b/plugins/node/opentelemetry-instrumentation-restify/package.json index 6f064e83973..0a3c8b99bfa 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/package.json +++ b/plugins/node/opentelemetry-instrumentation-restify/package.json @@ -6,16 +6,17 @@ "types": "build/src/index.d.ts", "repository": "open-telemetry/opentelemetry-js-contrib", "scripts": { - "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts'", - "tdd": "yarn test -- --watch-extensions ts --watch", "clean": "rimraf build/*", + "compile": "tsc -p .", "lint": "eslint . --ext .ts", "lint:fix": "eslint . --ext .ts --fix", "precompile": "tsc --version && lerna run version:update --scope @opentelemetry/instrumentation-restify --include-dependencies", + "prepare": "npm run compile", "prewatch": "npm run precompile", + "tdd": "yarn test -- --watch-extensions ts --watch", + "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts'", + "test-all-versions": "tav", "version:update": "node ../../../scripts/version-update.js", - "compile": "tsc -p .", - "prepare": "npm run compile", "watch": "tsc -w" }, "keywords": [ @@ -51,19 +52,22 @@ "@opentelemetry/sdk-trace-node": "^1.3.1", "@types/mocha": "7.0.2", "@types/node": "16.11.21", + "@types/restify": "4.3.8", + "@types/semver": "^7.3.12", "gts": "3.1.0", "mocha": "7.2.0", "nyc": "15.1.0", - "restify": "4.3.4", + "restify": "8.6.1", "rimraf": "3.0.2", + "semver": "^7.3.7", + "test-all-versions": "^5.0.1", "ts-mocha": "10.0.0", "typescript": "4.3.5" }, "dependencies": { "@opentelemetry/core": "^1.0.0", "@opentelemetry/instrumentation": "^0.32.0", - "@opentelemetry/semantic-conventions": "^1.0.0", - "@types/restify": "4.3.8" + "@opentelemetry/semantic-conventions": "^1.0.0" }, "homepage": "https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-restify#readme" } diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts b/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts index 24a21e62510..0ba23dccf5f 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts @@ -24,4 +24,4 @@ export const RESTIFY_METHODS = [ 'patch', ]; export const MODULE_NAME = 'restify'; -export const SUPPORTED_VERSIONS = ['>=4.0.0']; +export const SUPPORTED_VERSIONS = ['>=4.0.0 <9']; diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts index 89b39ad4237..baa98469a1a 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts @@ -14,10 +14,12 @@ * limitations under the License. */ +import type * as types from './types'; +import type * as restify from 'restify'; + import * as api from '@opentelemetry/api'; -import * as restify from 'restify'; import { Server } from 'restify'; -import * as types from './types'; +import { LayerType } from './types'; import * as AttributeNames from './enums/AttributeNames'; import { VERSION } from './version'; import * as constants from './constants'; @@ -34,9 +36,7 @@ import { getRPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core'; const { diag } = api; -export class RestifyInstrumentation extends InstrumentationBase< - typeof restify -> { +export class RestifyInstrumentation extends InstrumentationBase { constructor(config: InstrumentationConfig = {}) { super(`@opentelemetry/instrumentation-${constants.MODULE_NAME}`, VERSION); } @@ -45,7 +45,7 @@ export class RestifyInstrumentation extends InstrumentationBase< private _isDisabled = false; init() { - const module = new InstrumentationNodeModuleDefinition( + const module = new InstrumentationNodeModuleDefinition( constants.MODULE_NAME, constants.SUPPORTED_VERSIONS, (moduleExports, moduleVersion) => { @@ -55,7 +55,7 @@ export class RestifyInstrumentation extends InstrumentationBase< ); module.files.push( - new InstrumentationNodeModuleFile( + new InstrumentationNodeModuleFile( 'restify/lib/server.js', constants.SUPPORTED_VERSIONS, (moduleExports, moduleVersion) => { @@ -113,7 +113,7 @@ export class RestifyInstrumentation extends InstrumentationBase< return original.call( this, instrumentation._handlerPatcher( - { type: types.LayerType.MIDDLEWARE, methodName }, + { type: LayerType.MIDDLEWARE, methodName }, handler ) ); @@ -131,7 +131,7 @@ export class RestifyInstrumentation extends InstrumentationBase< this, path, ...instrumentation._handlerPatcher( - { type: types.LayerType.REQUEST_HANDLER, path, methodName }, + { type: LayerType.REQUEST_HANDLER, path, methodName }, handler ) ); @@ -168,7 +168,7 @@ export class RestifyInstrumentation extends InstrumentationBase< const fnName = handler.name || undefined; const spanName = - metadata.type === types.LayerType.REQUEST_HANDLER + metadata.type === LayerType.REQUEST_HANDLER ? `request handler - ${route}` : `middleware - ${fnName || 'anonymous'}`; const attributes = { diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts index 89da9fd49cb..d6280a6bda6 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ import { Span } from '@opentelemetry/api'; -import * as restify from 'restify'; +import type * as restify from 'restify'; export enum LayerType { MIDDLEWARE = 'middleware', diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index ba8845216c8..27c35b3fb66 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import * as restify from 'restify'; import { context, trace } from '@opentelemetry/api'; import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; @@ -28,10 +27,19 @@ import RestifyInstrumentation from '../src'; import * as types from '../src/types'; const plugin = new RestifyInstrumentation(); +import * as semver from 'semver'; import * as assert from 'assert'; import * as http from 'http'; import { AddressInfo } from 'net'; +import * as restify from 'restify'; +const LIB_VERSION = require('restify/package.json').version; + +const assertIsVersion = (str: any) => { + assert.strictEqual(typeof str, 'string'); + assert(/^[0-9]+\.[0-9]+\.[0-9]+$/.test(str)); +}; + const httpRequest = { get: (options: http.ClientRequestArgs | string) => { return new Promise((resolve, reject) => { @@ -65,6 +73,12 @@ const defer = (): { return { promise, resolve, reject }; }; +class AppError extends Error { + toJSON() { + return { message: this.message }; + } +} + const useHandler: restify.RequestHandler = (req, res, next) => { // only run if route was found next(); @@ -73,7 +87,10 @@ const getHandler: restify.RequestHandler = (req, res, next) => { res.send({ route: req?.params?.param }); }; const throwError: restify.RequestHandler = (req, res, next) => { - throw new Error('NOK'); + throw new AppError('NOK'); +}; +const returnError: restify.RequestHandler = (req, res, next) => { + next(new AppError('NOK')); }; const createServer = async (setupRoutes?: Function) => { @@ -83,14 +100,15 @@ const createServer = async (setupRoutes?: Function) => { setupRoutes(server); } else { // to force an anonymous fn for testing - server.pre((req, res, next) => { + server.pre((res: any, req: any, next: any) => { // run before routing next(); }); server.use(useHandler); server.get('/route/:param', getHandler); - server.get('/failing', throwError); + server.get('/thowing', throwError); + server.get('/erroring', returnError); } await new Promise(resolve => server.listen(0, resolve)); @@ -150,7 +168,7 @@ describe('Restify Instrumentation', () => { assert.strictEqual(span.attributes['restify.method'], 'pre'); assert.strictEqual(span.attributes['restify.type'], 'middleware'); assert.strictEqual(span.attributes['restify.name'], undefined); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } { // span from use @@ -160,7 +178,7 @@ describe('Restify Instrumentation', () => { assert.strictEqual(span.attributes['restify.method'], 'use'); assert.strictEqual(span.attributes['restify.type'], 'middleware'); assert.strictEqual(span.attributes['restify.name'], 'useHandler'); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } { // span from get @@ -173,7 +191,7 @@ describe('Restify Instrumentation', () => { 'request_handler' ); assert.strictEqual(span.attributes['restify.name'], 'getHandler'); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } } ); @@ -199,7 +217,7 @@ describe('Restify Instrumentation', () => { assert.strictEqual(span.attributes['restify.method'], 'pre'); assert.strictEqual(span.attributes['restify.type'], 'middleware'); assert.strictEqual(span.attributes['restify.name'], undefined); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } assert.strictEqual( res, @@ -209,16 +227,32 @@ describe('Restify Instrumentation', () => { ); }); - it('should create a span for an endpoint that threw', async () => { + it('should create a span for an endpoint that called done(error)', async () => { const rootSpan = tracer.startSpan('clientSpan'); await context.with( trace.setSpan(context.active(), rootSpan), async () => { - await httpRequest.get(`http://localhost:${port}/failing`); + const result = await httpRequest.get( + `http://localhost:${port}/erroring` + ); rootSpan.end(); assert.strictEqual(memoryExporter.getFinishedSpans().length, 4); + if (semver.satisfies(LIB_VERSION, '>=8')) { + assert.deepEqual( + result, + '{"code":"Internal","message":"Error: NOK"}' + ); + } else if (semver.satisfies(LIB_VERSION, '>=7 <8')) { + assert.deepEqual( + result, + '{"code":"Internal","message":"caused by Error: NOK"}' + ); + } else { + assert.deepEqual(result, '{"message":"NOK"}'); + } + { // span from pre const span = memoryExporter.getFinishedSpans()[0]; @@ -227,30 +261,30 @@ describe('Restify Instrumentation', () => { assert.strictEqual(span.attributes['restify.method'], 'pre'); assert.strictEqual(span.attributes['restify.type'], 'middleware'); assert.strictEqual(span.attributes['restify.name'], undefined); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } { // span from use const span = memoryExporter.getFinishedSpans()[1]; assert.notStrictEqual(span, undefined); - assert.strictEqual(span.attributes['http.route'], '/failing'); + assert.strictEqual(span.attributes['http.route'], '/erroring'); assert.strictEqual(span.attributes['restify.method'], 'use'); assert.strictEqual(span.attributes['restify.type'], 'middleware'); assert.strictEqual(span.attributes['restify.name'], 'useHandler'); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } { // span from get const span = memoryExporter.getFinishedSpans()[2]; assert.notStrictEqual(span, undefined); - assert.strictEqual(span.attributes['http.route'], '/failing'); + assert.strictEqual(span.attributes['http.route'], '/erroring'); assert.strictEqual(span.attributes['restify.method'], 'get'); assert.strictEqual( span.attributes['restify.type'], 'request_handler' ); - assert.strictEqual(span.attributes['restify.name'], 'throwError'); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assert.strictEqual(span.attributes['restify.name'], 'returnError'); + assertIsVersion(span.attributes['restify.version']); } } ); @@ -318,7 +352,7 @@ describe('Restify Instrumentation', () => { 'request_handler' ); assert.strictEqual(span.attributes['restify.name'], 'getHandler'); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } assert.strictEqual(res, '{"route":"hello"}'); } finally { @@ -376,7 +410,7 @@ describe('Restify Instrumentation', () => { 'request_handler' ); assert.strictEqual(span.attributes['restify.name'], 'asyncHandler'); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } } finally { testLocalServer.close(); @@ -441,7 +475,7 @@ describe('Restify Instrumentation', () => { span.attributes['restify.name'], 'promiseReturningHandler' ); - assert.strictEqual(span.attributes['restify.version'], 'n/a'); + assertIsVersion(span.attributes['restify.version']); } } finally { testLocalServer.close();