Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Warning: This pull request is touching the following templated files:

• README.md - README.md is managed by synthtool. However, a partials file can be used to update the README, e.g.: https://github.com/googleapis/nodejs-storage/blob/main/.readme-partials.yaml

@6utt3rfly, thanks for the contribution. Please take a look at the test failures. For example,

  1) Web framework tracing
       Tracing restify@9
         "before all" hook for "accurately measures get time (1 handler)":

      AssertionError [ERR_ASSERTION]: Handler [handler-0 on GET /one-handler] accepts a third argument (the 'next" callback) but is also an async function. Middleware handlers can be either async/await or callback-based. Async handler functions should accept maximum of 2 arguments: (req, res). Non-async handlers should accept three arguments: (req, res, next).
      + expected - actual


      at Chain.add (test/plugins/fixtures/restify9/node_modules/restify/lib/chain.js:91:16)
      at forEach (test/plugins/fixtures/restify9/node_modules/restify/lib/router.js:211:15)
      at Array.forEach (<anonymous>)
      at Router.mount (test/plugins/fixtures/restify9/node_modules/restify/lib/router.js:203:14)
      at Server.serverMethod [as get] (test/plugins/fixtures/restify9/node_modules/restify/lib/server.js:1750:33)
      at _a.addHandler (/home/runner/work/cloud-trace-nodejs/cloud-trace-nodejs/test/web-frameworks/restify.ts:38:19)
      at Context.<anonymous> (/home/runner/work/cloud-trace-nodejs/cloud-trace-nodejs/test/test-trace-web-frameworks.ts:113:22)
      at processImmediate (node:internal/timers:466:21)
      at process.topLevelDomainCallback (node:domain:161:15)
      at process.callbackTrampoline (node:internal/async_hooks:128:24)

Could you confirm whether you've tested this change locally, and with what versions?

@punya - I had trouble running local unit tests until I saw this PR's build log and realized I needed docker images running (or TRACE_TEST_EXCLUDE_INTEGRATION set). It might be worth adding something to the CONTRIBUTING.md file?

The existing restify plugin works if I manually patch the SUPPORTED_VERSIONS. I've pushed a commit to fix the failing unit test by avoiding async (req, res, next), which breaks in restify 9+

Thanks for looking into it @6utt3rfly! I have a couple of small suggestions, but otherwise this looks good to me.

I actually hadn't noticed .then accepts both resolve and reject, and thought it was only in new Promise(resolve, reject), so I like your suggestion!

I see some lint failures that should be easy to fix:

/home/runner/work/cloud-trace-nodejs/cloud-trace-nodejs/test/web-frameworks/restify.ts
Warning:   20:3   warning  'WebFrameworkResponse' is defined but never used  @typescript-eslint/no-unused-vars
Error:   41:17  error    Replace `(response)` with `response`              prettier/prettier

But there are also several failing tests (starting at https://github.com/googleapis/cloud-trace-nodejs/actions/runs/4694210640/jobs/8322181339?pr=1489#step:7:2986) that I don't understand.

Reading the Restify docs in some more detail, I'm not sure that we're supposed to use 3-argument handlers with Promises (regardless of whether we use async/await syntax to generate the Promise).

I wonder if it might be necessary to have totally different code paths for Restify <= v8 and Restify >= v9.

@punya - Cannot find node module 'node:process' seems to indicate a node compatibility issue with restify (even though their release notes indicate node 10 and 12 support). Are you okay with restricting the unit tests to node 14+ (see commit)?

@punya - I'm not sure why the release action hanged (5h 58m!), because it looks like everything passed, but then it didn't get the coverage

vs the PR test:

... maybe worth just retrying that job?