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

trentm · 2023-11-01T23:01:26Z

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

…ion warning for 404 request 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: open-telemetry#1757

trentm · 2023-11-01T23:12:29Z

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

…er typescript than 4.4.4

trentm · 2023-11-01T23:21:20Z

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 · 2023-11-01T23:32:25Z

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 · 2023-11-06T18:47:06Z

plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts

-        anyRequest.routeOptions?.config?.url || request.routerPath;
+      const routeName = anyRequest.routeOptions
+        ? anyRequest.routeOptions.url
+        : request.routerPath;


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

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.

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.

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 · 2023-11-07T13:32:12Z

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 · 2023-11-09T17:05:03Z

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

…pen-telemetry#1723 changes.

trentm · 2023-11-17T18:27:00Z

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

pichlermarc

Looks good, thanks for fixing this 👍

pichlermarc · 2023-11-29T15:57:57Z

plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts

-        anyRequest.routeOptions?.config?.url || request.routerPath;
+      const routeName = anyRequest.routeOptions
+        ? anyRequest.routeOptions.url
+        : request.routerPath;


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.

trentm requested a review from a team November 1, 2023 23:01

github-actions bot assigned pichlermarc Nov 1, 2023

github-actions bot requested a review from pichlermarc November 1, 2023 23:01

need to limit to fastify 4.18.0, because later versions require a new…

94553e4

…er typescript than 4.4.4

trentm changed the title ~~fix(fastify): fix span attributes and avoid FSTDEP017 FastifyDeprecation warning for 404 request~~ fix(instrumentation-fastify): fix span attributes and avoid FSTDEP017 FastifyDeprecation warning for 404 request Nov 6, 2023

trentm commented Nov 6, 2023

View reviewed changes

trentm added 2 commits November 6, 2023 10:47

Merge branch 'main' into tm-fastify-404

bd91541

hopefully helpful comment on when routeOptions.url became available

03a26ac

pichlermarc added bug Something isn't working pkg:instrumentation-fastify priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization labels Nov 7, 2023

trentm and others added 6 commits November 7, 2023 12:17

add some more versions to fastify test-all-versions tests

839838f

Merge branch 'main' into tm-fastify-404

b009011

fix compile

18832b4

poke to re-run tests to see if some failures are spurious

4dffa45

Merge branch 'main' into tm-fastify-404

342d976

Merge branch 'main' into tm-fastify-404

6094158

trentm marked this pull request as draft November 9, 2023 17:05

trentm added 3 commits November 9, 2023 09:07

Merge branch 'main' into tm-fastify-404

99de149

Merge branch 'main' into tm-fastify-404

ab85a10

sync package-lock file. However this is also undoing part of recent o…

b0b74e0

…pen-telemetry#1723 changes.

trentm mentioned this pull request Nov 16, 2023

handling package-lock.json updates #1806

Closed

add an ESM test

d8ff26e

trentm marked this pull request as ready for review November 17, 2023 18:26

trentm added 4 commits November 21, 2023 13:38

Merge branch 'main' into tm-fastify-404

d957004

bump to latest contrib-test-utils ver

bda679b

Merge branch 'main' into tm-fastify-404

48b6471

update to latest contrib-test-utils release, sync package-lock

9ed84de

pichlermarc approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'main' into tm-fastify-404

fd51b5e

trentm merged commit 18ae75c into open-telemetry:main Nov 29, 2023

trentm deleted the tm-fastify-404 branch November 29, 2023 18:19

dyladan mentioned this pull request Nov 28, 2023

chore: release main #1829

Merged

This was referenced Jan 2, 2024

fix(fastify): avoid the FSTDEP017 and FSTDEP018 deprecation warnings elastic/apm-agent-nodejs#3814

Merged

fix fastify deprecated routerMethod、routerPath elastic/apm-agent-nodejs#3789

Closed

trentm mentioned this pull request Feb 15, 2024

coordinate first pass at ESM support for all instrumentations #1942

Open

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

trentm commented Nov 1, 2023

trentm commented Nov 1, 2023

trentm commented Nov 1, 2023

codecov bot commented Nov 1, 2023 •

edited

Loading

trentm Nov 6, 2023 •

edited

Loading

pichlermarc Nov 7, 2023

trentm Nov 7, 2023

pichlermarc Nov 29, 2023

pichlermarc commented Nov 7, 2023 •

edited

Loading

trentm commented Nov 9, 2023

trentm commented Nov 17, 2023

pichlermarc left a comment

pichlermarc Nov 29, 2023

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

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

Conversation

trentm commented Nov 1, 2023

trentm commented Nov 1, 2023

trentm commented Nov 1, 2023

codecov bot commented Nov 1, 2023 • edited Loading

Codecov Report

trentm Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

pichlermarc Nov 7, 2023

Choose a reason for hiding this comment

trentm Nov 7, 2023

Choose a reason for hiding this comment

pichlermarc Nov 29, 2023

Choose a reason for hiding this comment

pichlermarc commented Nov 7, 2023 • edited Loading

trentm commented Nov 9, 2023

trentm commented Nov 17, 2023

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc Nov 29, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2023 •

edited

Loading

trentm Nov 6, 2023 •

edited

Loading

pichlermarc commented Nov 7, 2023 •

edited

Loading