Skip to content

feat(fastify): add captureBody support #2681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 3, 2023
Merged

feat(fastify): add captureBody support #2681

merged 7 commits into from
May 3, 2023

Conversation

xxzefgh
Copy link
Contributor

@xxzefgh xxzefgh commented May 7, 2022

Closes: #1906
Closes: #3217

@cla-checker-service
Copy link

cla-checker-service bot commented May 7, 2022

💚 CLA has been signed

@github-actions github-actions bot added agent-nodejs Make available for APM Agents project planning. community triage labels May 7, 2022
@xxzefgh
Copy link
Contributor Author

xxzefgh commented May 7, 2022

have not tested this yet, will reopen later

@xxzefgh xxzefgh closed this May 7, 2022
@xxzefgh
Copy link
Contributor Author

xxzefgh commented May 8, 2022

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

@xxzefgh xxzefgh reopened this May 8, 2022
@apmmachine
Copy link
Contributor

apmmachine commented May 8, 2022

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

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

  • Duration: 39 min 26 sec

Test stats 🧪

Test Results
Failed 0
Passed 258182
Skipped 0
Total 258182

Steps errors 21

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

🤖 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!)

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xxzefgh This looks great! Thanks for starting this.

  1. Could you add a test for this to "test/instrumentation/modules/fastify/fastify.test.js"? captureBody support was recently added for hapi instrumentation. Perhaps you could mostly copy how the equivalent test was added for that PR: https://github.com/elastic/apm-agent-nodejs/pull/2618/files#diff-ce2dc92ded3295986f31e8f4178788ae10a651ec2badfda8b8d92fc1b2d2cd7bR3-R118 If you need help understanding how the test code works there, please ask. I'm happy to help.

  2. There is also a commented out test in "test/sanitize-field-names/fastify.test.js" that could be added now that you've implemented this. You could apply this diff to enable it:

--- a/test/sanitize-field-names/fastify.test.js
+++ b/test/sanitize-field-names/fastify.test.js
@@ -3,6 +3,7 @@ const { createAgentConfig } = require('./_shared')
 const agent = require('../..').start(createAgentConfig())
 const {
   resetAgent,
+  assertFormsWithFixture,
   assertRequestHeadersWithFixture,
   assertResponseHeadersWithFixture
 } = require('./_shared')
@@ -27,9 +28,7 @@ function runTest (
     const transaction = data.transactions.pop()
     assertRequestHeadersWithFixture(transaction, expected, t)
     assertResponseHeadersWithFixture(transaction, expected, t)
-    // TODO: uncomment once we fix
-    // https://github.com/elastic/apm-agent-nodejs/issues/1906
-    // assertFormsWithFixture(transaction, expected, t)
+    assertFormsWithFixture(transaction, expected, t)
   })

   // register request handler
  1. You could add Fixes: #1906 to your PR description to mark that this issue fixes the already open issue we had to add captureBody support for fastify.

@trentm trentm removed the triage label May 9, 2022
@trentm
Copy link
Member

trentm commented May 9, 2022

run module tests for fastify

@xxzefgh
Copy link
Contributor Author

xxzefgh commented May 10, 2022

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

@trentm
Copy link
Member

trentm commented May 10, 2022

@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
Copy link
Contributor Author

xxzefgh commented May 31, 2022

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
Copy link
Member

trentm commented May 31, 2022

@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.

apm-agent-nodejs/.tav.yml

Lines 475 to 500 in 7e9b9e3

# https://www.fastify.io/docs/latest/LTS/
# - #1086 suggests [email protected] was a broken release, skip it.
fastify-v1:
name: fastify
versions: '1.x'
node: '>=6 <12'
commands:
- 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
fastify-v2:
name: fastify
versions: '>=2.0.0 <2.4.0 || >2.4.0 <3'
node: '>=6 <15'
commands:
- 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
fastify:
name: fastify
versions: '>=3.0.0'
node: '>=10'
commands:
- 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
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
Copy link
Member

trentm commented May 31, 2022

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
Copy link
Contributor Author

xxzefgh commented May 31, 2022

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
Copy link
Member

trentm commented May 31, 2022

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
Copy link
Member

trentm commented May 31, 2022

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
Copy link
Member

trentm commented May 31, 2022

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:

// fastify >=3 only supports node >=10.
const fastifyVer = require('../../node_modules/fastify/package.json').version
const semver = require('semver')
if (semver.gte(fastifyVer, '3.0.0') && semver.lt(process.version, '10.0.0')) {
console.log(`# SKIP fastify@${fastifyVer} does not support node ${process.version}`)
process.exit()
}

@trentm
Copy link
Member

trentm commented Jun 1, 2022

run module tests for fastify

@trentm
Copy link
Member

trentm commented Jun 1, 2022

@elasticmachine, run elasticsearch-ci/docs

@trentm
Copy link
Member

trentm commented Jun 2, 2022

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
Copy link
Member

trentm commented Aug 8, 2022

@elasticmachine, run elasticsearch-ci/docs

@trentm
Copy link
Member

trentm commented Aug 8, 2022

run module tests for fastify

@trentm trentm added this to the 8.9 milestone Apr 24, 2023
@trentm
Copy link
Member

trentm commented May 2, 2023

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

@trentm trentm linked an issue May 2, 2023 that may be closed by this pull request
3 tasks
@trentm
Copy link
Member

trentm commented May 2, 2023

@elasticmachine, run elasticsearch-ci/docs

@trentm trentm self-assigned this May 2, 2023
@trentm
Copy link
Member

trentm commented May 2, 2023

@elasticmachine, run elasticsearch-ci/docs

@trentm
Copy link
Member

trentm commented May 2, 2023

@elasticmachine, run elasticsearch-ci/docs

@trentm trentm merged commit ca1ede1 into elastic:main May 3, 2023
v1v added a commit to v1v/apm-agent-nodejs that referenced this pull request May 8, 2023
…re/support-specific-modules

* 'main' of github.com:elastic/apm-agent-nodejs: (54 commits)
  chore: fix dev-utils/ci-tav-slow-jobs.sh (elastic#3319)
  test: reduce TAV test matrix for slowest jobs (elastic#3321)
  chore: sync package-lock so 'npm ci' can work (elastic#3318)
  docs: document `useElasticTraceparentHeader` config var (elastic#3316)
  chore, test: test driver improvements (elastic#3293)
  test: drop node 14 from RC tests now that it is EOL (elastic#3315)
  test: fix running fastify.test.js with node v8 (elastic#3317)
  feat: add @apollo/server@4 support (elastic#3203)
  chore: update nvm (elastic#3309)
  tests: stop testing 'express-graphql' instrumentation (elastic#3304)
  chore: fix bitrot.js dev util for recent changes (elastic#3308)
  test: restore testing of Azure Functions on node >=18.x (elastic#3307)
  fix: support Lambda instrumentation for `contextManager: 'patch'`; refactor Lambda tests (elastic#3305)
  test: fix fastify TAV test failures (elastic#3314)
  test: fix @aws-sdk/client-s3 TAV test failures (elastic#3312)
  feat: add instrumentation for aws-sdk S3 client (elastic#3287)
  feat(fastify): add captureBody support (elastic#2681)
  feat: mysql2@3 support (elastic#3301)
  chore(deps): bump @opentelemetry/exporter-prometheus from 0.37.0 to 0.38.0 in /test/opentelemetry-metrics/fixtures (elastic#3295)
  chore(deps-dev): bump fastify from 4.16.3 to 4.17.0 (elastic#3296)
  ...
trentm added a commit that referenced this pull request May 16, 2023
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.9-candidate agent-nodejs Make available for APM Agents project planning. community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fastify wont capture body - fix proposed Fastify Instrumentation does not Capture Request Body
3 participants