From 4d324620a2b111acd83a165b8924d9be68240a3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jordi=20Guti=C3=A9rrez=20Hermoso?= Date: Thu, 3 Nov 2022 10:06:09 -0400 Subject: [PATCH 1/3] NEWRELIC-4422: modify tests to not check for __NR_original Instead, we will either check the name of wrapped methods or compare the method with original, unwrapped methods to see if they've been wrapped. Also removed a copy-paste test for both the koa-router and @koa/router modules. --- lib/instrumentation/koa/tests/unit/koa.tap.js | 24 +-- .../koa/tests/unit/route.tap.js | 6 +- .../koa/tests/unit/router.tap.js | 138 +++++++----------- 3 files changed, 72 insertions(+), 96 deletions(-) diff --git a/lib/instrumentation/koa/tests/unit/koa.tap.js b/lib/instrumentation/koa/tests/unit/koa.tap.js index e4e1f82cfa..2d259f9569 100644 --- a/lib/instrumentation/koa/tests/unit/koa.tap.js +++ b/lib/instrumentation/koa/tests/unit/koa.tap.js @@ -5,28 +5,34 @@ 'use strict' -var tap = require('tap') -var utils = require('@newrelic/test-utilities') +const tap = require('tap') +const utils = require('@newrelic/test-utilities') utils.tap tap.test('Koa instrumentation', function (t) { - var helper = utils.TestAgent.makeInstrumented() + const wrapped = ['createContext', 'use', 'emit'] + const notWrapped = ['handleRequest', 'listen', 'toJSON', 'inspect', 'callback', 'onerror'] + + // Save the original methods, to compare with wrapped ones below + const origKoa = require('koa') + const origMethods = Object.fromEntries( + wrapped.concat(notWrapped).map((method) => [method, origKoa.prototype[method]]) + ) + + const helper = utils.TestAgent.makeInstrumented() helper.registerInstrumentation({ moduleName: 'koa', type: 'web-framework', onRequire: require('../../lib/instrumentation') }) - var Koa = require('koa') - - var wrapped = ['createContext', 'use', 'emit'] - var notWrapped = ['handleRequest', 'listen', 'toJSON', 'inspect', 'callback', 'onerror'] + const Koa = require('koa') wrapped.forEach(function (method) { - t.ok(Koa.prototype[method].__NR_original, method + ' is wrapped, as expected') + t.not(Koa.prototype[method], origMethods[method], method + ' is wrapped, as expected') }) notWrapped.forEach(function (method) { - t.notOk(Koa.prototype[method].__NR_original, method + ' is not wrapped, as expected') + t.equal(Koa.prototype[method], origMethods[method], method + ' is not wrapped, as expected') }) helper && helper.unload() diff --git a/lib/instrumentation/koa/tests/unit/route.tap.js b/lib/instrumentation/koa/tests/unit/route.tap.js index df63c3527c..9a542bb478 100644 --- a/lib/instrumentation/koa/tests/unit/route.tap.js +++ b/lib/instrumentation/koa/tests/unit/route.tap.js @@ -10,7 +10,7 @@ const utils = require('@newrelic/test-utilities') const { METHODS } = require('../../lib/http-methods') tap.test('koa-route', function (t) { - var helper = utils.TestAgent.makeInstrumented() + const helper = utils.TestAgent.makeInstrumented() t.teardown(function () { helper.unload() @@ -23,9 +23,9 @@ tap.test('koa-route', function (t) { }) t.test('methods', function (t) { - var route = require('koa-route') + const route = require('koa-route') METHODS.forEach(function checkWrapped(method) { - t.type(route[method].__NR_original, 'function', method + ' should be wrapped') + t.ok(route[method].name.startsWith('wrapped'), method + ' should be wrapped') }) t.end() }) diff --git a/lib/instrumentation/koa/tests/unit/router.tap.js b/lib/instrumentation/koa/tests/unit/router.tap.js index 19979ff620..56a9f18b6b 100644 --- a/lib/instrumentation/koa/tests/unit/router.tap.js +++ b/lib/instrumentation/koa/tests/unit/router.tap.js @@ -21,96 +21,66 @@ const UNWRAPPED_METHODS = METHODS.concat([ ]) const UNWRAPPED_STATIC_METHODS = ['url'] -tap.test('koa-router', function tests(t) { - var helper = utils.TestAgent.makeInstrumented() - t.teardown(function () { - helper.unload() - }) - helper.registerInstrumentation({ - type: 'web-framework', - moduleName: 'koa-router', - onRequire: instrumentation - }) +const koaRouterMods = ['koa-router', '@koa/router'] - t.test('mounting paramware', function (t) { - var Router = require('koa-router') - var router = new Router() - router.param('second', function () {}) - t.type(router.params.second.__NR_original, 'function', 'param function should be wrapped') - t.end() - }) +koaRouterMods.forEach((koaRouterMod) => { + tap.test(koaRouterMod, function tests(t) { + // Save the original methods, to compare with wrapped ones below + const origRouter = require(koaRouterMod) + const origMethods = Object.fromEntries( + [...WRAPPED_METHODS, ...UNWRAPPED_METHODS].map((method) => [ + method, + origRouter.prototype[method] + ]) + ) + const origStaticMethods = Object.fromEntries( + UNWRAPPED_STATIC_METHODS.map((method) => [method, origRouter[method]]) + ) - t.test('methods', function (t) { - var Router = require('koa-router') - WRAPPED_METHODS.forEach(function checkWrapped(method) { - t.type( - Router.prototype[method].__NR_original, - 'function', - method + ' should be a wrapped method on the prototype' - ) - }) - UNWRAPPED_METHODS.forEach(function checkUnwrapped(method) { - t.type( - Router.prototype[method].__NR_original, - 'undefined', - method + ' should be a unwrapped method on the prototype' - ) + var helper = utils.TestAgent.makeInstrumented() + t.teardown(function () { + helper.unload() }) - UNWRAPPED_STATIC_METHODS.forEach(function checkUnwrappedStatic(method) { - t.type( - Router[method].__NR_original, - 'undefined', - method + ' should be an unwrapped static method' - ) - }) - t.end() - }) - t.autoend() -}) -tap.test('@koa/router', function tests(t) { - var helper = utils.TestAgent.makeInstrumented() - t.teardown(function () { - helper.unload() - }) - helper.registerInstrumentation({ - type: 'web-framework', - moduleName: '@koa/router', - onRequire: instrumentation - }) - - t.test('mounting paramware', function (t) { - var Router = require('@koa/router') - var router = new Router() - router.param('second', function () {}) - t.type(router.params.second.__NR_original, 'function', 'param function should be wrapped') - t.end() - }) - - t.test('methods', function (t) { - var Router = require('@koa/router') - WRAPPED_METHODS.forEach(function checkWrapped(method) { - t.type( - Router.prototype[method].__NR_original, - 'function', - method + ' should be a wrapped method on the prototype' - ) + helper.registerInstrumentation({ + type: 'web-framework', + moduleName: koaRouterMod, + onRequire: instrumentation }) - UNWRAPPED_METHODS.forEach(function checkUnwrapped(method) { - t.type( - Router.prototype[method].__NR_original, - 'undefined', - method + ' should be a unwrapped method on the prototype' - ) + + t.test('mounting paramware', function (t) { + var Router = require(koaRouterMod) + var router = new Router() + router.param('second', function () {}) + t.ok(router.params.second.name.startsWith('wrap'), 'param function should be wrapped') + t.end() }) - UNWRAPPED_STATIC_METHODS.forEach(function checkUnwrappedStatic(method) { - t.type( - Router[method].__NR_original, - 'undefined', - method + ' should be an unwrapped static method' - ) + + t.test('methods', function (t) { + var Router = require(koaRouterMod) + WRAPPED_METHODS.forEach(function checkWrapped(method) { + t.not( + Router.prototype[method], + origMethods[method], + method + ' should be a wrapped method on the prototype' + ) + }) + UNWRAPPED_METHODS.forEach(function checkUnwrapped(method) { + t.equal( + Router.prototype[method], + origMethods[method], + method + ' should be a unwrapped method on the prototype' + ) + }) + UNWRAPPED_STATIC_METHODS.forEach(function checkUnwrappedStatic(method) { + t.equal( + Router[method], + origStaticMethods[method], + method + ' should be an unwrapped static method' + ) + }) + t.end() }) - t.end() + t.autoend() }) - t.autoend() }) From 0e0024f5c7a187fa1b2aa6e680879a88c7c1758f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jordi=20Guti=C3=A9rrez=20Hermoso?= Date: Thu, 3 Nov 2022 10:19:50 -0400 Subject: [PATCH 2/3] NEWRELIC-4422: remove __NR prefixes in favour of symbols This was done completely automatically by executing the following script: #!/bin/bash # Let's make this script idempotent, undo all work before starting. hg revert --all --quiet regex='__NR_[\w_]+\b' # Which files need to be modified? fnames=$(hg grep '\.'$regex -l --include 'glob:**.js') # What are the symbols to change? symbols=$(grep -Poh $regex $fnames | sort -u | sed "s/__NR_\(.*\)/ \1: Symbol('\1\'),/") echo "Symbols to modify:" echo "$symbols" # So modify them. sed -i 's,\.__NR_\([[:alpha:]]\+\),[symbols.\1],g' $fnames # Which files are missing a `require` statement? fnames=$(grep -LP 'require\(.*symbols.*\)' $fnames) # So add the require to those files. for fname in $fnames; do require="const symbols = require('$(realpath lib/symbols.js --relative-to=$(dirname $fname) | sed s,.js,, | sed s,^symbols,./symbols,)')"; # Look for last require or after use strict if no require is found last_line_number=$( (grep -nP '^(const|var).* = require' $fname || grep -nP "'use strict'" $fname) | tail -1 | cut -f 1 -d:) insert_line_number=$((last_line_number+1)) echo "inserting $require at line $insert_line_number for $fname" sed -i "${insert_line_number}i $require" $fname done --- .../koa/lib/instrumentation.js | 27 ++++++++++--------- .../koa/lib/router-instrumentation.js | 3 ++- lib/instrumentation/koa/lib/symbols.js | 13 +++++++++ 3 files changed, 29 insertions(+), 14 deletions(-) create mode 100644 lib/instrumentation/koa/lib/symbols.js diff --git a/lib/instrumentation/koa/lib/instrumentation.js b/lib/instrumentation/koa/lib/instrumentation.js index 2452ced922..fd8e874a6b 100644 --- a/lib/instrumentation/koa/lib/instrumentation.js +++ b/lib/instrumentation/koa/lib/instrumentation.js @@ -4,6 +4,7 @@ */ 'use strict' +const symbols = require('./symbols') module.exports = function initialize(shim, Koa) { if (!shim || !Koa || Object.keys(Koa.prototype).length > 1) { @@ -68,23 +69,23 @@ function wrapCreateContext(shim, fn, fnName, context) { // The `context.body` and `context.response.body` properties are how users set // the response contents. It is roughly equivalent to `res.send()` in Express. // Under the hood, these set the `_body` property on the `context.response`. - context.__NR_body = context.response.body - context.__NR_bodySet = false + context[symbols.body] = context.response.body + context[symbols.bodySet] = false Object.defineProperty(context.response, '_body', { - get: () => context.__NR_body, + get: () => context[symbols.body], set: function setBody(val) { - if (!context.__NR_koaRouter) { + if (!context[symbols.koaRouter]) { shim.savePossibleTransactionName(context.req) } - context.__NR_body = val - context.__NR_bodySet = true + context[symbols.body] = val + context[symbols.bodySet] = true } }) - context.__NR_matchedRoute = null - context.__NR_koaRouter = false + context[symbols.matchedRoute] = null + context[symbols.koaRouter] = false Object.defineProperty(context, '_matchedRoute', { - get: () => context.__NR_matchedRoute, + get: () => context[symbols.matchedRoute], set: (val) => { const match = getLayerForTransactionName(context) @@ -98,7 +99,7 @@ function wrapCreateContext(shim, fn, fnName, context) { if (currentSegment) { const transaction = currentSegment.transaction - if (context.__NR_matchedRoute) { + if (context[symbols.matchedRoute]) { transaction.nameState.popPath() } @@ -107,10 +108,10 @@ function wrapCreateContext(shim, fn, fnName, context) { } } - context.__NR_matchedRoute = val + context[symbols.matchedRoute] = val // still true if somehow match is undefined because we are // using koa-router naming and don't want to allow default naming - context.__NR_koaRouter = true + context[symbols.koaRouter] = true } }) @@ -130,7 +131,7 @@ function wrapCreateContext(shim, fn, fnName, context) { Object.defineProperty(context.response, 'status', { get: () => statusDescriptor.get.call(context.response), set: function setStatus(val) { - if (!context.__NR_bodySet && !context.__NR_koaRouter) { + if (!context[symbols.bodySet] && !context[symbols.koaRouter]) { shim.savePossibleTransactionName(context.req) } return statusDescriptor.set.call(this, val) diff --git a/lib/instrumentation/koa/lib/router-instrumentation.js b/lib/instrumentation/koa/lib/router-instrumentation.js index 99a6a1e20f..6623e98281 100644 --- a/lib/instrumentation/koa/lib/router-instrumentation.js +++ b/lib/instrumentation/koa/lib/router-instrumentation.js @@ -4,6 +4,7 @@ */ 'use strict' +const symbols = require('./symbols') module.exports = function instrumentRouter(shim, Router) { shim.setFramework(shim.KOA) @@ -76,7 +77,7 @@ function wrapAllowedMethods(shim, fn, name, allowedMethodsMiddleware) { function wrapAllowedMethodsMiddleware(shim, original) { return function setRouteHandledOnContextWrapper() { const [ctx] = shim.argsToArray.apply(shim, arguments) - ctx.__NR_koaRouter = true + ctx[symbols.koaRouter] = true return original.apply(this, arguments) } diff --git a/lib/instrumentation/koa/lib/symbols.js b/lib/instrumentation/koa/lib/symbols.js new file mode 100644 index 0000000000..cf6168cdf2 --- /dev/null +++ b/lib/instrumentation/koa/lib/symbols.js @@ -0,0 +1,13 @@ +/* + * Copyright 2022 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +module.exports = { + body: Symbol('body'), + bodySet: Symbol('bodySet'), + koaRouter: Symbol('koaRouter'), + matchedRoute: Symbol('matchedRoute') +} From 0e2af072dc6361151b3e651942e27a14271e83c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jordi=20Guti=C3=A9rrez=20Hermoso?= Date: Mon, 7 Nov 2022 16:22:36 -0500 Subject: [PATCH 3/3] fix tests to use shim.isWrapped instead --- lib/instrumentation/koa/tests/unit/koa.tap.js | 11 ++----- .../koa/tests/unit/route.tap.js | 4 ++- .../koa/tests/unit/router.tap.js | 33 +++++-------------- 3 files changed, 14 insertions(+), 34 deletions(-) diff --git a/lib/instrumentation/koa/tests/unit/koa.tap.js b/lib/instrumentation/koa/tests/unit/koa.tap.js index 2d259f9569..b23b403c4c 100644 --- a/lib/instrumentation/koa/tests/unit/koa.tap.js +++ b/lib/instrumentation/koa/tests/unit/koa.tap.js @@ -14,12 +14,6 @@ tap.test('Koa instrumentation', function (t) { const wrapped = ['createContext', 'use', 'emit'] const notWrapped = ['handleRequest', 'listen', 'toJSON', 'inspect', 'callback', 'onerror'] - // Save the original methods, to compare with wrapped ones below - const origKoa = require('koa') - const origMethods = Object.fromEntries( - wrapped.concat(notWrapped).map((method) => [method, origKoa.prototype[method]]) - ) - const helper = utils.TestAgent.makeInstrumented() helper.registerInstrumentation({ moduleName: 'koa', @@ -27,12 +21,13 @@ tap.test('Koa instrumentation', function (t) { onRequire: require('../../lib/instrumentation') }) const Koa = require('koa') + const shim = helper.getShim() wrapped.forEach(function (method) { - t.not(Koa.prototype[method], origMethods[method], method + ' is wrapped, as expected') + t.ok(shim.isWrapped(Koa.prototype[method]), method + ' is wrapped, as expected') }) notWrapped.forEach(function (method) { - t.equal(Koa.prototype[method], origMethods[method], method + ' is not wrapped, as expected') + t.not(shim.isWrapped(Koa.prototype[method]), method + ' is not wrapped, as expected') }) helper && helper.unload() diff --git a/lib/instrumentation/koa/tests/unit/route.tap.js b/lib/instrumentation/koa/tests/unit/route.tap.js index 9a542bb478..de8b91aead 100644 --- a/lib/instrumentation/koa/tests/unit/route.tap.js +++ b/lib/instrumentation/koa/tests/unit/route.tap.js @@ -22,10 +22,12 @@ tap.test('koa-route', function (t) { onRequire: require('../../lib/route-instrumentation.js') }) + const shim = helper.getShim() + t.test('methods', function (t) { const route = require('koa-route') METHODS.forEach(function checkWrapped(method) { - t.ok(route[method].name.startsWith('wrapped'), method + ' should be wrapped') + t.ok(shim.isWrapped(route[method]), method + ' should be wrapped') }) t.end() }) diff --git a/lib/instrumentation/koa/tests/unit/router.tap.js b/lib/instrumentation/koa/tests/unit/router.tap.js index 56a9f18b6b..1ba222c950 100644 --- a/lib/instrumentation/koa/tests/unit/router.tap.js +++ b/lib/instrumentation/koa/tests/unit/router.tap.js @@ -25,22 +25,11 @@ const koaRouterMods = ['koa-router', '@koa/router'] koaRouterMods.forEach((koaRouterMod) => { tap.test(koaRouterMod, function tests(t) { - // Save the original methods, to compare with wrapped ones below - const origRouter = require(koaRouterMod) - const origMethods = Object.fromEntries( - [...WRAPPED_METHODS, ...UNWRAPPED_METHODS].map((method) => [ - method, - origRouter.prototype[method] - ]) - ) - const origStaticMethods = Object.fromEntries( - UNWRAPPED_STATIC_METHODS.map((method) => [method, origRouter[method]]) - ) - - var helper = utils.TestAgent.makeInstrumented() + const helper = utils.TestAgent.makeInstrumented() t.teardown(function () { helper.unload() }) + const shim = helper.getShim() helper.registerInstrumentation({ type: 'web-framework', @@ -52,32 +41,26 @@ koaRouterMods.forEach((koaRouterMod) => { var Router = require(koaRouterMod) var router = new Router() router.param('second', function () {}) - t.ok(router.params.second.name.startsWith('wrap'), 'param function should be wrapped') + t.ok(shim.isWrapped(router.params.second), 'param function should be wrapped') t.end() }) t.test('methods', function (t) { var Router = require(koaRouterMod) WRAPPED_METHODS.forEach(function checkWrapped(method) { - t.not( - Router.prototype[method], - origMethods[method], + t.ok( + shim.isWrapped(Router.prototype[method]), method + ' should be a wrapped method on the prototype' ) }) UNWRAPPED_METHODS.forEach(function checkUnwrapped(method) { - t.equal( - Router.prototype[method], - origMethods[method], + t.not( + shim.isWrapped(Router.prototype[method]), method + ' should be a unwrapped method on the prototype' ) }) UNWRAPPED_STATIC_METHODS.forEach(function checkUnwrappedStatic(method) { - t.equal( - Router[method], - origStaticMethods[method], - method + ' should be an unwrapped static method' - ) + t.not(shim.isWrapped(Router[method]), method + ' should be an unwrapped static method') }) t.end() })