fix(instrumentation-fastify): fix span attributes and avoid FSTDEP017 FastifyDeprecation warning for 404 request

[trentm]

For a 404, request.routeOptions.url is undefined. Since fastify@4.10.0
when routeOptions was added, we shouldn't fallback to the deprecated
request.routerPath.

This also corrects the assumption that the handler name is "bound ..."
in all cases. E.g. for a 404 it is Fastify's core "basic404" internal
function.

Fixes: #1757

[trentm]

@pichlermarc I have a side question about the .tav.yml file for instrumentation-fastify (added in #1710). Is it the intention to only test with a single explicit version of fastify ("4.23.2")?

Oh, or do I need to not update the fastify version in "package.json" in this PR, because versions after 4.18.0 require a typescript greater than 4.4.4? I'll see if CI fails and then move it back to "fastify": "4.18.0" if needed.

If we want test-all-versions to test with recent releases of fastify as they come out, we could have a .tav.yml like this:

--- a/plugins/node/opentelemetry-instrumentation-fastify/.tav.yml
+++ b/plugins/node/opentelemetry-instrumentation-fastify/.tav.yml
@@ -1,5 +1,5 @@
-"fastify":
-  - versions: "4.23.2"
+fastify:
+  - versions: "4.0.0 || >4.24.3 <5"
     commands: npm run test
 "typescript":
   - versions: "4.7.4"

This would sanity test with the first 4.0.0 release and then with any releases at or after the current latest. This does require occasionally bump that >4.24.3 if/when there are so many fastify releases that it becomes slow/expensive to test them all.

[trentm]

Oh, or do I need to not update the fastify version in "package.json" in this PR, because versions after 4.18.0 require a typescript greater than 4.4.4? I'll see if CI fails and then move it back to "fastify": "4.18.0" if needed.

Indeed it did fail the npm run compile. I've reverted the fastify version back to 4.18.0.

[codecov]

Codecov Report

Merging #1763 (fd51b5e) into main (b39c96c) will decrease coverage by 0.03%.
The diff coverage is 77.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1763      +/-   ##
==========================================
- Coverage   91.50%   91.48%   -0.03%     
==========================================
  Files         144      144              
  Lines        7369     7374       +5     
  Branches     1467     1471       +4     
==========================================
+ Hits         6743     6746       +3     
- Misses        626      628       +2     

Files	Coverage Δ
...try-instrumentation-fastify/src/instrumentation.ts	95.48% <77.77%> (-1.39%)	⬇️

[trentm]

Regarding the codecov warning on this line and below: TAV tests could cover line 101, but currently they won't because it doesn't go back to an early-enough fastify version. Shall I add an // istanbul ignore next directive here?

Update: Actually, perhaps an istanbul directive won't help, because that is what nyc is using. I don't know what codecov is using. When I run the tests locally, I get different lines that are uncovered:

---------------------|---------|----------|---------|---------|-------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------|---------|----------|---------|---------|-------------------
All files            |   93.91 |    75.24 |   94.73 |   93.91 |
 src                 |   93.43 |    73.68 |   94.28 |   93.43 |
  constants.ts       |     100 |      100 |     100 |     100 |
  instrumentation.ts |   96.07 |       80 |     100 |   96.07 | 153-157,161,235
  utils.ts           |   84.84 |       56 |   71.42 |   84.84 | 107-113,119
 src/enums           |     100 |      100 |     100 |     100 |
  AttributeNames.ts  |     100 |      100 |     100 |     100 |
---------------------|---------|----------|---------|---------|-------------------

[pichlermarc]

Interesting; AFAIK codecov uses the reports generated by istanbul, so adding the directive should help. Looks like the uncovered lines above are actually not tested at the moment (see full file on the codecov report - not sure if you have access). We could also add an earlier fastify version to TAV if that fixes the issue.

I've just recently added TAV to this package, having more versions tested would likely be a good idea anyway.

[trentm]

I've added testing of fastify@4.0.0 plus the current latest and any subsequent fastify 4.x releases to the .tav.yml.
I don't know if coverage data from TAV runs get included in the codecov summary.

[pichlermarc]

Ah right, it looks like reports from TAV are not included.

IMO just having the version tested is good enough for now, we may look into adding coverage from TAV in the future, but we've been having problems with codecov for a while now, so I think this will likely be a larger task.

[pichlermarc]

Oh, or do I need to not update the fastify version in "package.json" in this PR, because versions after 4.18.0 require a typescript greater than 4.4.4? I'll see if CI fails and then move it back to "fastify": "4.18.0" if needed.

Indeed it did fail the npm run compile. I've reverted the fastify version back to 4.18.0.

Sorry, I was out of office so I wasn't able to respond. This is exactly the reason. We can't bump beyond 4.4.4 because the fastify instrumentation is included by @opentelemetry/auto-instrumentations-node which may break users if we go beyond 4.4.4 with one of the packages, so this is the workaround for now (until we can bump typescript).

[trentm]

@pichlermarc The TAV for PR / tav / Run test-all-versions (14) job is failing for this PR currently because of some funkiness with the npm install layout:

@opentelemetry/instrumentation-fastify: -- installing ["fastify@4.24.3"]
@opentelemetry/instrumentation-fastify: -- running test "npm run test" with fastify (env: {})
@opentelemetry/instrumentation-fastify: > @opentelemetry/instrumentation-fastify@0.32.3 test /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-fastify
@opentelemetry/instrumentation-fastify: > nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'
@opentelemetry/instrumentation-fastify: Error: Cannot find module '@opentelemetry/contrib-test-utils'
@opentelemetry/instrumentation-fastify: Require stack:
@opentelemetry/instrumentation-fastify: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts

I was inclined to wait for #1771 to go in first to get some improvements to the npm install handling. When that is merged, I'll update this PR and dig into the test failure if it still exists.
(I can make this a draft PR, if that helps.)

[trentm]

Ready for review, modulo the package-lock.json update discussion in #1806

[pichlermarc]

Looks good, thanks for fixing this 👍

[pichlermarc]

Ah right, it looks like reports from TAV are not included.

IMO just having the version tested is good enough for now, we may look into adding coverage from TAV in the future, but we've been having problems with codecov for a while now, so I think this will likely be a larger task.