Add support for renamed hapi => @hapi/hapi module #1098

brad-gardner · 2019-05-29T12:54:54Z

Added support for HapiJS modules updated location at @hapi/hapi

I copied the existing hapi implementations and trimmed out most of the semver checks as they don't apply to the new module paths. Copied and trimmed the tests as well. This is working in my tests so far but hoping to get code review and suggestion process started.

Closes #1097

Checklist

Implement code
Add tests
Update documentation

watson · 2019-05-29T14:30:26Z

Thank for letting us know about this and creating the PR 💯

I'd prefer if we could find a way to avoid duplicating the files as that will be hard to keep in sync when fixing bugs, though that will require quite a bit of refactoring. But I'll be happy to help you land this PR if you have the time 😃

To achieve this, one approach would be to allow a module to have zero or more "aliases", described in an array of strings where the first element in the array is also the file path for the patched module on disk:

diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js
index acbe95aa..f0748654 100644
--- a/lib/instrumentation/index.js
+++ b/lib/instrumentation/index.js
@@ -23,7 +23,7 @@ var MODULES = [
   'generic-pool',
   'graphql',
   'handlebars',
-  'hapi',
+  ['hapi', '@hapi/hapi'],
   'http',
   'https',
   'http2',
@@ -70,12 +70,17 @@ function Instrumentation (agent) {
   this._patches = new NamedArray()
 
   for (let mod of MODULES) {
-    this.addPatch(mod, (exports, name, version, enabled) => {
-      // Lazy require so that we don't have to use `require.resolve` which
-      // would fail in combination with Webpack. For more info see:
-      // https://github.com/elastic/apm-agent-nodejs/pull/957
-      const patch = require(`./modules/${mod}`)
-      return patch(exports, name, version, enabled)
+    if (!Array.isArray(mod)) mod = [mod]
+    const pathName = mod[0]
+
+    mod.forEach(mod => {
+      this.addPatch(mod, (exports, name, version, enabled) => {
+        // Lazy require so that we don't have to use `require.resolve` which
+        // would fail in combination with Webpack. For more info see:
+        // https://github.com/elastic/apm-agent-nodejs/pull/957
+        const patch = require(`./modules/${pathName}`)
+        return patch(exports, name, version, enabled)
+      })
     })
   }
 }

To avoid duplicating the tests, we could just wrap the basic.js test file in a loop that essentially runs the tests twice once for each module name.

Duplicating the set-framework.js test file is the easiest I think as it needs a clean state of the process when it runs. And it doesn't contain that many lines anyway, so I'm ok with that.

You'd also need to duplicate the hapi section in the .tav.yml file and add @hapi/hapi to the TAV tests in both .travis.yml and test/.jenkins_tav.yml.

brad-gardner · 2019-05-29T14:35:01Z

That's exactly the conversation I was hoping for :)

I was thinking about some sort of alias as well, as this is unlikely to be the only time this will happen so it'll be good to have a model going forward.

I'll continue updating the PR to refactor if that works best. I think I'm pretty comfortable with the module itself but may need some help making sure I have everything in place for the test infrastructure as it's a bit outside of my normal toolset and comfort zone. Thanks for the callouts to the travis/jenkins files!

watson · 2019-05-29T14:37:23Z

Glad I can help. Forgot to mention that you probably want to add @hapi/hapi to docs/supported-technologies.asciidoc as well.

brad-gardner · 2019-05-29T17:24:25Z

OK, I think it's closer now. I'm not sure exactly what to do with .tav.yml, it doesn't seem like an update would be necessary?

Let me know what you think when you have a moment.

I'm currently having difficulty executing the test suite locally, the cassandra container won't stay up for me yet. I can run the hapi tests directly though and they are all passing, so moving the right direction.

watson · 2019-06-03T13:14:07Z

What I was thinking regarding .tav.yml was something like this:

diff --git a/.tav.yml b/.tav.yml
index 5c131e8e..1c8a0fc7 100644
--- a/.tav.yml
+++ b/.tav.yml
@@ -211,6 +211,10 @@ hapi-async-await:
   node: '>=8.2'
   versions: '>=17.0.0'
   commands: node test/instrumentation/modules/hapi/basic.js
+'@hapi/hapi':
+  node: '>=8.2'
+  versions: '>=17.0.0'
+  commands: node test/instrumentation/modules/hapi/basic.js
 
 tedious:
   name: tedious

However, I just realized I hadn't thought it all the way through when it comes to having test/instrumentation/modules/hapi/basic.js run the tests for both hapi and @hapi/hapi as this means that the tests are not separated when running these TAV tests.

Either we just ignore it and accept that some tests are run multiple times, or we could improve the basic.js script slightly to take an argument, e.g:

diff --git a/.tav.yml b/.tav.yml
index 5c131e8e..a587a0f6 100644
--- a/.tav.yml
+++ b/.tav.yml
@@ -205,12 +205,16 @@ pug:
 hapi-no-async-await:
   name: hapi
   versions: '>=9.0.1 <17.0.0'
-  commands: node test/instrumentation/modules/hapi/basic.js
+  commands: node test/instrumentation/modules/hapi/basic.js hapi
 hapi-async-await:
   name: hapi
   node: '>=8.2'
   versions: '>=17.0.0'
-  commands: node test/instrumentation/modules/hapi/basic.js
+  commands: node test/instrumentation/modules/hapi/basic.js hapi
+'@hapi/hapi':
+  node: '>=8.2'
+  versions: '>=17.0.0'
+  commands: node test/instrumentation/modules/hapi/basic.js @hapi/hapi
 
 tedious:
   name: tedious

Then inside of test/instrumentation/modules/hapi/basic.js you could check if process.argv[2] was set. If it was you could use that for the content of moduleNames, if not, you'd default to both modules.

What do you think of that approach? Is it too "magic"?

Qard · 2019-06-03T23:20:14Z

Could maybe split the basic test into a shared file that just exports everything as a function which accepts the module name and have two separate test files that use it.

// shared file
module.exports = (name) => {
   // ...
}

// hapi/basic.js
require('./shared-file')('hapi')

// @hapi/hapi/basic.js
require('./shared-file')('@hapi/hapi')

brad-gardner · 2019-06-30T13:44:37Z

Finally getting back to this, sorry for the delay!

I updated the .tav.yml and refactored the basic.js file based on the above suggestions, splitting it in 2 and using a shared file.

Qard

Looks great! Just a minor thing I want to get some feedback from @watson on and then it looks good to merge. :)

lib/instrumentation/index.js

watson

Looking great!

I like your approach with having a shared file with all the tests 👍

I can see that there are some linting errors in one of the files regarding indentation. You can run npm run lint to see them (or view on Travis), and you can run npm run lint:fix to try and have them automatically corrected.

Besides that, there's just one thing missing from the .tav.yml file (see comment) and then I think @Qard's suggestion about adding support for arrays as the argument to addPatch is good. So if we are up for it, you can add that as well. If not, that's ok, then just fix linting errors and the one comment in the .tav.yml file and then we should be 👌

lib/instrumentation/index.js

.tav.yml

…the existing hapi implementations

…es, updated tests

brad-gardner · 2019-07-01T13:04:32Z

I believe I have it all, let me know what you think!

brad-gardner · 2019-07-01T13:04:49Z

Looking at API docs now

Qard

Some small nits that we can fix, if necessary. Other than that, LGTM.

Qard · 2019-07-01T13:20:13Z

lib/instrumentation/index.js

+    }
+
+    this._patches.add(mod, handler)
+    this._startHook()


The call to this._startHook() could be moved out of the loop as it doesn't need to be called between patches being added.

Qard · 2019-07-01T13:20:48Z

lib/instrumentation/index.js


-  this._patches.add(name, handler)
-  this._startHook()
+  modules.forEach(mod => {


This could be for (let mod of modules) {.

Qard · 2019-07-01T13:21:42Z

lib/instrumentation/index.js


  for (let mod of MODULES) {
+    if (!Array.isArray(mod)) mod = [mod]
+    const pathName = mod[0]


Maybe const pathName = Array.isArray(mod) ? mod[0] : mod, so you don't need to build an array you don't actually need.

Qard · 2019-07-01T13:22:51Z

lib/instrumentation/index.js

+
+  modules.forEach(mod => {
+    this._patches.delete(mod, handler)
+    this._startHook()


Again, the call to this._startHook() doesn't need to be made between each change, you could just put it after the loop. Also, for/let loop too, please. :)

watson

There's an issue with the tests on older Node.js versions because @hapi/hapi uses the spread operator which isn't supported on those old versions. We already took care of this for the hapi package, so we just need to include the @hapi/hapi package in that exemption.

The test file in question is test/config.js, and you just need to duplicate this line for @hapi/hapi:

apm-agent-nodejs/test/config.js

Line 594 in 3b7fb1b

var hapiVersion = require('hapi/package.json').version

And these lines as well:

apm-agent-nodejs/test/config.js

Lines 605 to 607 in 3b7fb1b

    
           if (semver.lt(process.version, '8.9.0') && semver.gte(hapiVersion, '17.0.0')) { 
        
             modules.delete('hapi') 
        
           }

Also, during some refactoring with master I think we lost the line that added @hapi/hapi to this file: https://github.com/elastic/apm-agent-nodejs/blob/master/.ci/.jenkins_tav.yml

watson added the [zube]: In Progress label May 29, 2019

brad-gardner force-pushed the master branch from 28d03f3 to 543f8f6 Compare May 29, 2019 16:32

brad-gardner force-pushed the master branch from 88039d5 to d593cfd Compare May 29, 2019 17:45

watson changed the title ~~Closes #1097~~ Add support for renamed hapi => @hapi/hapi module May 31, 2019

brad-gardner force-pushed the master branch from bf47d11 to a195ac4 Compare June 30, 2019 13:04

Qard reviewed Jul 1, 2019

View reviewed changes

lib/instrumentation/index.js Outdated Show resolved Hide resolved

watson reviewed Jul 1, 2019

View reviewed changes

lib/instrumentation/index.js Outdated Show resolved Hide resolved

.tav.yml Outdated Show resolved Hide resolved

brad-gardner added 10 commits July 1, 2019 09:03

added @hapi/hapi module for instrumentation, copied and slimmed down …

1173d00

…the existing hapi implementations

added @hapi/hapi to tests

6760e2b

refactored modules to load with aliases, updated supported technologi…

567022e

…es, updated tests

test updates

ccae340

linting corrections

329f5bc

updated config test

97324d6

split hapi basic file into 2 with a shared implementation

08b13c9

refactored addPatch and removePatch to allow lists

71c0c6f

added set framework command for @Hapi

8c1a80f

linting fix

a5224f8

brad-gardner force-pushed the master branch from a195ac4 to a5224f8 Compare July 1, 2019 13:03

doc updates

82d71e5

Qard approved these changes Jul 1, 2019

View reviewed changes

watson reviewed Jul 1, 2019

View reviewed changes

Qard mentioned this pull request Jul 29, 2019

chore: make hapi instrumentation support @hapi/hapi rename #1246

Merged

watson closed this in #1246 Aug 7, 2019

zube bot added [zube]: Done and removed [zube]: In Progress labels Aug 7, 2019

lreuven removed the [zube]: Done label Aug 15, 2019

	if (semver.lt(process.version, '8.9.0') && semver.gte(hapiVersion, '17.0.0')) {
	modules.delete('hapi')
	}

Add support for renamed hapi => @hapi/hapi module #1098

Add support for renamed hapi => @hapi/hapi module #1098

Uh oh!

Conversation

brad-gardner commented May 29, 2019 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

watson commented May 29, 2019

Uh oh!

brad-gardner commented May 29, 2019

Uh oh!

watson commented May 29, 2019

Uh oh!

brad-gardner commented May 29, 2019

Uh oh!

watson commented Jun 3, 2019

Uh oh!

Qard commented Jun 3, 2019

Uh oh!

brad-gardner commented Jun 30, 2019

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

watson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brad-gardner commented Jul 1, 2019

Uh oh!

brad-gardner commented Jul 1, 2019

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Qard Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

Qard Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

Qard Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

Qard Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

watson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brad-gardner commented May 29, 2019 •

edited by zube bot

Loading

watson left a comment •

edited

Loading