feat(fastify): add captureBody support

[xxzefgh]

trafficstars

Closes: #1906

[cla-checker-service[bot]]

💚 CLA has been signed

[xxzefgh]

have not tested this yet, will reopen later

[xxzefgh]

Tested with [email protected], http.request.body.original field is populated with request body

[apmmachine]

:broken_heart: Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-08-08T19:32:33.500+0000

• Duration: 39 min 26 sec

Test stats :test_tube:

Test	Results
Failed	0
Passed	258182
Skipped	0
Total	258182

Steps errors

Expand to view the steps failures

Show only the first 10 steps failures

Run Tests

• Took 1 min 8 sec . View more details here
• Description: .ci/scripts/test.sh -b "release" -t "fastify" "12"

Run Tests

• Took 0 min 27 sec . View more details here
• Description: .ci/scripts/test.sh -b "release" -t "fastify" "12"

Archive the artifacts

• Took 0 min 1 sec . View more details here
• Description: [2022-08-08T20:03:25.853Z] Archiving artifacts [2022-08-08T20:03:26.596Z] ‘test_output/*.tap’ d

Run Tests

• Took 1 min 55 sec . View more details here
• Description: .ci/scripts/test.sh -b "release" -t "fastify" "10"

Run Tests

• Took 0 min 25 sec . View more details here
• Description: .ci/scripts/test.sh -b "release" -t "fastify" "10"

Archive the artifacts

• Took 0 min 1 sec . View more details here
• Description: [2022-08-08T20:03:09.062Z] Archiving artifacts [2022-08-08T20:03:09.788Z] ‘test_output/*.tap’ d

Run Tests

• Took 5 min 18 sec . View more details here
• Description: .ci/scripts/test.sh -b "release" -t "fastify" "8"

Run Tests

• Took 5 min 43 sec . View more details here
• Description: .ci/scripts/test.sh -b "release" -t "fastify" "8"

Archive the artifacts

• Took 0 min 1 sec . View more details here
• Description: [2022-08-08T20:11:55.276Z] Archiving artifacts [2022-08-08T20:11:55.933Z] ‘test_output/*.tap’ d

Error signal

• Took 0 min 0 sec . View more details here
• Description: hudson.AbortException: script returned exit code 1

:robot: GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

• run benchmark tests : Run the benchmark test only.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[trentm]

run module tests for fastify

[xxzefgh]

@trentm thanks for the input, it will take me probably 1 or 2 weeks to make this a proper PR.

[trentm]

@xxzefgh Okay. If you need help, let me know. You should be able to run that one test file directly to test your changes:

node test/instrumentation/modules/fastify/fastify.test.js

[xxzefgh]

Hi @trentm, I've added tests as you requested. One issue is that tests are run against v2 (required in devDependencies), which was already supported. I did locally install v3 and it passed, but I'm not sure how tests can be run against multiple versions.

[trentm]

@xxzefgh Thanks!

One issue is that tests are run against v2 (required in devDependencies), which was already supported. I did locally install v3 and it passed, but I'm not sure how tests can be run against multiple versions.

We just recently updated devDeps to fastify@3 in #2738

However, we also use test-all-versions (TAV for short) to test all/most supported versions of modules that we instrument. https://github.com/elastic/apm-agent-nodejs/blob/7e9b9e31d20c5170854ac62319bea6818ba2a81c/.tav.yml#L475-L500 is the current block that defines the fastify versions we test against. I'll kick off those "TAV" tests for this PR after the regular tests run.

[trentm]

Actually we could stand to add test/sanitize-field-names/fastify.test.js to the fastify .tav.yml test blocks.

Would you be willing to add this patch to your PR?

diff --git a/.tav.yml b/.tav.yml
index d24137a2..dde25443 100644
--- a/.tav.yml
+++ b/.tav.yml
@@ -484,6 +484,7 @@ fastify-v1:
     - node test/instrumentation/modules/fastify/fastify.test.js
     - node test/instrumentation/modules/fastify/async-await.test.js
     - node test/instrumentation/modules/fastify/set-framework.test.js
+    - node test/sanitize-field-names/fastify.test.js
 fastify-v2:
   name: fastify
   versions: '>=2.0.0 <2.4.0 || >2.4.0 <3'
@@ -492,6 +493,7 @@ fastify-v2:
     - node test/instrumentation/modules/fastify/fastify.test.js
     - node test/instrumentation/modules/fastify/async-await.test.js
     - node test/instrumentation/modules/fastify/set-framework.test.js
+    - node test/sanitize-field-names/fastify.test.js
 fastify:
   name: fastify
   versions: '>=3.0.0'
@@ -500,6 +502,7 @@ fastify:
     - node test/instrumentation/modules/fastify/fastify.test.js
     - node test/instrumentation/modules/fastify/async-await.test.js
     - node test/instrumentation/modules/fastify/set-framework.test.js
+    - node test/sanitize-field-names/fastify.test.js

 finalhandler:
   versions: '*'

[xxzefgh]

Ok, I think I was wrong on v2 comment, as I forgot to comment out my patches for v2 before testing 😐

Rebased and added changes to .tav.yml

[trentm]

I'm not yet sure what is going on with the node test/instrumentation/modules/fastify/fastify.test.js test failure with node 8.6 (in the GitHub Actions-based test above). I cannot reproduce that failure using node 8.6 on my mac. I'll take a look later when I have a chance.

[trentm]

Ah, yes, I can reproduce locally: this happens when using fastify@3 (now in our package-lock.json file) and node 8.6. Fastify@3 only supports node v10 and later. So, we need to add a guard on top of the relevant fastify test files to just skip the test if the node version and fastify version aren't supported.

@xxzefgh Can you please add this patch to your PR:

diff --git a/test/instrumentation/modules/fastify/fastify.test.js b/test/instrumentation/modules/fastify/fastify.test.js
index f9f294ab..ae343585 100644
--- a/test/instrumentation/modules/fastify/fastify.test.js
+++ b/test/instrumentation/modules/fastify/fastify.test.js
@@ -8,6 +8,17 @@ const agent = require('../../../..').start({
   captureBody: 'all'
 })

+const fastifyVer = require('../../../../node_modules/fastify/package.json').version
+const semver = require('semver')
+if (
+  (semver.satisfies(fastifyVer, '1.x') && !semver.satisfies(process.version, '>=6 <12')) ||
+  (semver.satisfies(fastifyVer, '2.x') && !semver.satisfies(process.version, '>=6 <15')) ||
+  (semver.satisfies(fastifyVer, '3.x') && !semver.satisfies(process.version, '>=10'))
+) {
+  console.log(`# SKIP fastify@${fastifyVer} does not support node ${process.version}`)
+  process.exit()
+}
+
 const http = require('http')

 const Fastify = require('fastify')

This reproduces the same version ranges as in the .tav.yml section for fastify.

[trentm]

It is possible we'll need to add the same guard to the other test files that test fastify, but let's see. We already have a form of that guard in one of those test files: https://github.com/elastic/apm-agent-nodejs/blob/7e9b9e31d20c5170854ac62319bea6818ba2a81c/test/sanitize-field-names/fastify.test.js#L5-L11

[trentm]

run module tests for fastify

[trentm]

@elasticmachine, run elasticsearch-ci/docs

[trentm]

The "test all versions" tests failed on the captureBody test with fastify@1 and node@10. For some reason that I didn't dig into, the preHandler life-cycle hook doesn't run with fastify@1 (though the hook is listed at https://github.com/fastify/fastify/blob/1.x/docs/Lifecycle.md). However, fastify@1 is long since EOL, so we don't need to support new features for it. Here is a suggested patch for skipping that test for this combination (and a couple other doc tweaks):

diff --git a/docs/supported-technologies.asciidoc b/docs/supported-technologies.asciidoc
index b961ccfc..6b0b684c 100644
--- a/docs/supported-technologies.asciidoc
+++ b/docs/supported-technologies.asciidoc
@@ -46,7 +46,7 @@ These are the frameworks that we officially support:
 router information for full support. We currently support the most popular Koa router called
 https://github.com/alexmingoia/koa-router[koa-router].
 |<<restify,Restify>> |>=5.2.0
-|<<fastify,Fastify>> |>=1.0.0; see also https://www.fastify.io/docs/latest/LTS/[Fastify's own LTS documentation]
+|<<fastify,Fastify>> |>=1.0.0; see also https://www.fastify.io/docs/latest/Reference/LTS/[Fastify's own LTS documentation]
 |<<lambda,AWS Lambda>> |N/A
 |=======================================================================
 
diff --git a/lib/instrumentation/modules/fastify.js b/lib/instrumentation/modules/fastify.js
index 7646822e..cd757ca7 100644
--- a/lib/instrumentation/modules/fastify.js
+++ b/lib/instrumentation/modules/fastify.js
@@ -61,6 +61,7 @@ module.exports = function (fastify, agent, { version, enabled }) {
     })
 
     // Save the parsed req body to be picked up by getContextFromRequest().
+    // (This doesn't work for fastify@1, but that is now EOL.)
     _fastify.addHook('preHandler', (req, reply, next) => {
       req.raw.body = req.body
       next()
diff --git a/test/instrumentation/modules/fastify/fastify.test.js b/test/instrumentation/modules/fastify/fastify.test.js
index ae343585..df05c5bb 100644
--- a/test/instrumentation/modules/fastify/fastify.test.js
+++ b/test/instrumentation/modules/fastify/fastify.test.js
@@ -65,55 +65,58 @@ test('transaction name', function (t) {
   })
 })
 
-test('captureBody', function (t) {
-  t.plan(9)
-
-  const postData = JSON.stringify({ foo: 'bar' })
-
-  resetAgent(data => {
-    assert(t, data, { name: 'POST /postSomeData', method: 'POST' })
-    t.equal(data.transactions[0].context.request.body, postData,
-      'body was captured to trans.context.request.body')
-    fastify.close()
-  })
-
-  var fastify = Fastify()
+// Fastify `captureBody` support is only supported for [email protected] and later.
+if (semver.gte(fastifyVer, '2.0.0')) {
+  test('captureBody', function (t) {
+    t.plan(9)
+
+    const postData = JSON.stringify({ foo: 'bar' })
+
+    resetAgent(data => {
+      assert(t, data, { name: 'POST /postSomeData', method: 'POST' })
+      t.equal(data.transactions[0].context.request.body, postData,
+        'body was captured to trans.context.request.body')
+      fastify.close()
+    })
 
-  fastify.post('/postSomeData', (request, reply) => {
-    reply.send('your data has been posted')
-  })
+    var fastify = Fastify()
 
-  fastify.listen(0, function (err) {
-    t.error(err)
+    fastify.post('/postSomeData', (request, reply) => {
+      reply.send('your data has been posted')
+    })
 
-    // build the URL manually as older versions of fastify doesn't supply it as
-    // an argument to the callback
-    const port = fastify.server.address().port
-    const cReq = http.request(
-      'http://localhost:' + port + '/postSomeData',
-      {
-        method: 'POST',
-        hostname: 'localhost',
-        port,
-        headers: {
-          'Content-Type': 'application/json',
-          'Content-Length': Buffer.byteLength(postData)
+    fastify.listen(0, function (err) {
+      t.error(err)
+
+      // build the URL manually as older versions of fastify doesn't supply it as
+      // an argument to the callback
+      const port = fastify.server.address().port
+      const cReq = http.request(
+        'http://localhost:' + port + '/postSomeData',
+        {
+          method: 'POST',
+          hostname: 'localhost',
+          port,
+          headers: {
+            'Content-Type': 'application/json',
+            'Content-Length': Buffer.byteLength(postData)
+          }
+        },
+        function (res) {
+          t.strictEqual(res.statusCode, 200)
+          res.on('data', function (chunk) {
+            t.strictEqual(chunk.toString(), 'your data has been posted')
+          })
+          res.on('end', function () {
+            agent.flush()
+          })
         }
-      },
-      function (res) {
-        t.strictEqual(res.statusCode, 200)
-        res.on('data', function (chunk) {
-          t.strictEqual(chunk.toString(), 'your data has been posted')
-        })
-        res.on('end', function () {
-          agent.flush()
-        })
-      }
-    )
-    cReq.write(postData)
-    cReq.end()
+      )
+      cReq.write(postData)
+      cReq.end()
+    })
   })
-})
+}
 
 function resetAgent (cb) {
   agent._instrumentation.testReset()

@xxzefgh If you are good adding that to your PR, I can re-run the tests. Thanks for persisting through the slog of testing.

[trentm]

@elasticmachine, run elasticsearch-ci/docs

[trentm]

run module tests for fastify

[trentm]

This has languished. I'm adding some small commits to get this over the line.

[trentm]

@elasticmachine, run elasticsearch-ci/docs

[trentm]

@elasticmachine, run elasticsearch-ci/docs

[trentm]

@elasticmachine, run elasticsearch-ci/docs