chore: add support for hapi 18.

[aorinevo]

CHANGE LOG

• add support for hapi 18 by adding new scoped package name to list of instrumentations in lib/instrumentations.js

INTERNAL LINKS

• chore: fix vulnerabilities #288

NOTES

• Change is low risk since existing consumers of New Relic on Hapi 17 require the framework via require('hapi') whereas Hapi 18+ users require the framework via require('@hapi/hapi'). Hence, no chance of collision.
• NPM has recently added support for aliases ( couldn't find docs for this but implementation is same as yarn's; see https://yarnpkg.com/lang/en/docs/cli/add/#toc-yarn-add-alias ) So if consumer implements a migration strategy from Hapi 17 to Hapi 18 by aliasing the scoped version onto hapi module name, then no change is required.
• Without aliasing or adding support for the new scoped package, some of NR functionality will fail gracefully. For example, NR's metrics grouping will group almost all transactions under /*. To fix this, users can set theNEW_RELIC_NAMING_RULES env var with appropriate rules objects (see https://docs.newrelic.com/docs/agents/nodejs-agent/installation-configuration/nodejs-agent-configuration#rules_config ).
• Started discussion thread here: https://discuss.newrelic.com/t/add-support-for-hapi-18/77795
• Also opened support ticket
• hapi 18 release notes: 18.0.0 Release Notes hapijs/hapi#3871

Tests

• Security:
  • There are some vulnerabilities in dependencies that need to be resolved.
  • chore: fix vulnerabilities #288
• Versioned:
  • Need more unit tests to cover @hapi/hapi similar to pre/post hapi 17 unit tests although much more lightweight.

[aorinevo]

@NatalieWolfe, @psvet: Any feedback on this PR? :)

[lykkin]

Hey @aorinevo, thanks for the PR. In order for this to get shipped, I'd like to see testing at least in parity with the versions of hapi <18. This can be done by adjusting the versions the versioned tests will run on here. This respects the usual version range syntax that npm expects.

There will also need to be some code changes for how hapi is being loaded (not the only place, but many of the tests go through this load path). This will test the new version up to parity with the older versions. It'll miss any new features, and test any deprecated/removed features (which will hopefully cause some test failures, but still something to be weary of).

[aorinevo]

With v18 of hapi, request.parameters.* is not respected when passed in attributes.include array. That is, when present, url parameters are not populated in attributes object as request.parameters.{key}, where key is a url parameter.

For example,

    server.route({
      method: 'GET',
      path: '/test/{id}/',
      handler: function(request, reply) {
        // request.params.id = 1337
        t.ok(agent.getTransaction(), 'transaction is available')
        return { status: 'ok' }
      }
    })

    agent.on('transactionFinished', function(tx) {
      var attributes = tx.trace.attributes.get(DESTINATIONS.TRANS_TRACE)
      // attributes['request.parameters.id'] is undefined
    })

    server.start().then(function() {
      port = server.info.port
      request( 'http://localhost:' + port + '/test/1337/');
    })

I couldn't Identify the root cause of why this is failing for hapi 18 but not hapi 17.

Any thoughts?

Notes

• addRequestParameters is not called ( see

  node-newrelic/lib/transaction/index.js

  Line 1011 in e447ac4

  Transaction.prototype.addRequestParameters = addRequestParameters

  )

[aorinevo]

Resolved issue above.

The root cause is a pathing issue with the new scope for Hapi v18. That is, files were attempted to be loaded from instrumentation/@hapi/hapi where files actually live in instrumentations/hapi.

One way to resolve this issue is to copy over the files to the new path.

[aorinevo]

I'm thinking we should have a separate test folder for hapi 18.

Thoughts?

[aorinevo]

@lykkin, got the tests running and passing for Hapi 18.

I took a decision to copy hapi 17 unit tests to another directory for hapi-18 and rename hapi-post-17 to hapi-post-17-pre-18.

Let me know what you think.

[aorinevo]

One resolution is to copy over files to new path.

Instead, created /lib/instrumentation/@hapi/hapi.js which exports require('../hapi') and eliminates the need to copy over any files.

[lykkin]

@aorinevo Thanks for taking the time to update the tests! It mostly looks good, just a nit and a couple questions.

Nit: hapi-post-17-pre-18 can be renamed to hapi-17 for clarity.
Question 1: The versions being tested don't cover the first two minor releases of v18, is that intended?
Question 2: This PR brings the hapi v18 support up to parity with the older versions of hapi, but ignores any possible new functionality. Are there any features/changes that might be important to support in v18 that would be worth documenting as TODO?

[aorinevo]

Nit: hapi-post-17-pre-18 can be renamed to hapi-17 for clarity.
Question 1: The versions being tested don't cover the first two minor releases of v18, is that intended?
Question 2: This PR brings the hapi v18 support up to parity with the older versions of hapi, but ignores any possible new functionality. Are there any features/changes that might be important to support in v18 that would be worth documenting as TODO?

Thanks for the review @lykkin!

• I changed hapi-post-17-pre-18 to hapi-17.
• Changed test version range from >=18.3.1 to ^18.3.1.
• Added a link to the release notes for Hapi 18. I'll review those more closely to see if there is anything worth documenting.

Is this sufficient to move forward with the PR?

[lykkin]

@aorinevo Yep! Looks like everything is in order at this point. I'll open this up as a PR against our internal repo and we can get it out in the next couple of releases.

Thanks again for putting all this together!

[mattcphillips]

Just found this PR after troubleshooting the same issue for a while. Thanks for doing this! Do you have a rough estimate of when this might get merged and published? It'd be really great to have this fixed.

[aorinevo]

Hi @mattcphillips, this should be going out in the next couple of releases, although no set date has been shared.

In the interim, you might want to consider creating a scoped version of this fork or installing off of a github url. If the latter, I would recommend forking first and installing off of that.

We decided to go with a private scoped version of this fork for stability and security reasons.

Hope this helps :)