test: fix tests for restify >=5 (open-telemetry#1191)

* fix(restify): upgrade package to fix type problem * fix(restify): updated so types are not leaking through open-telemetry#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 <[email protected]>
dynatrace-oss-contrib · Sep 23, 2022 · 8873057 · 8873057
1 parent 6920a55
commit 8873057
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 40 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<any> {
   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<typeof restify>(
+    const module = new InstrumentationNodeModuleDefinition<any>(
       constants.MODULE_NAME,
       constants.SUPPORTED_VERSIONS,
       (moduleExports, moduleVersion) => {
@@ -55,7 +55,7 @@ export class RestifyInstrumentation extends InstrumentationBase<
     );
 
     module.files.push(
-      new InstrumentationNodeModuleFile<typeof restify>(
+      new InstrumentationNodeModuleFile<any>(
         '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
@@ -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
@@ -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<void>(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();