fix: Removed deprecated properties usage in Fastify instrumentation

[seidelmartin]

Which problem is this PR solving?

Instrumentation has been using deprecated properties producing errors like FastifyDeprecation: Request#context property access is deprecated. Please use "Request#routeConfig" or "Request#routeSchema" instead for accessing Route settings. The "Request#context" will be removed in fastify@5.
#1275

Short description of the changes

I've added getting deprecated properties from newly introduced routeOptions object in fastify request. It supports also old versions that doesn't have this object exposed. It has been introduced in fastify https://github.com/fastify/fastify/releases/tag/v4.23.0

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: seidelmartin / name: Martin Seidel (931bc30, 469135c, 6f9e4e7, 93735c7)
• ✅ login: pichlermarc / name: Marc Pichler (ba66df0)

[codecov]

Codecov Report

Merging #1679 (93735c7) into main (2d36152) will decrease coverage by 0.04%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 93735c7 differs from pull request most recent head ba66df0. Consider uploading reports for the commit ba66df0 to get more accurate results

@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
- Coverage   91.76%   91.72%   -0.04%     
==========================================
  Files         139      139              
  Lines        7125     7128       +3     
  Branches     1431     1433       +2     
==========================================
  Hits         6538     6538              
- Misses        587      590       +3     

Files	Coverage Δ
...try-instrumentation-fastify/src/instrumentation.ts	96.87% <100.00%> (-0.73%)	⬇️
...opentelemetry-instrumentation-fastify/src/utils.ts	82.50% <100.00%> (-5.00%)	⬇️

[seidelmartin]

Hi @pichlermarc. I'm confused by the GH workflow failure. Is it something that fails globally or just in my PR? I've tried main version locally and lerna boostrap fails for me as well...

[pichlermarc]

@seidelmartin I'm looking into it, I think it may be both. Trying it on this branch makes it fail on the fastify intrumentation's prepare script, but trying it on main makes it fail for the AWS SDK instrumentation for me. 🤔 I'm not sure yet what the cause for it is, yet; I'll keep you posted.

[pichlermarc]

@seidelmartin looks like I had an old revision checked out and main is actually compiling fine when re-installing dependencies. I think the problem here could be that the types from @types/mongodb hoisted to the project root no longer compile with typescript 5 (we align typescript for this reason). Would you mind trying with the lowest possible typescript version that still compiles with fastify? 🤔 Otherwise we'll have to add --nohoist='@types/mongodb' when running lerna bootstrap.

[seidelmartin]

@pichlermarc I'll try that. 👍🏻

[seidelmartin]

@pichlermarc good news, after downgrading typescript to 4.7.4 all checks pass.

[pichlermarc]

@pichlermarc good news, after downgrading typescript to 4.7.4 all checks pass.

Glad to hear. 🙂 Looks like now it's only missing CLA (see #1679 (comment)) and the lint step 🙂

[seidelmartin]

Alright, all 🟢.

[seidelmartin]

Hi @pichlermarc, what will be the next step here to publish it as new version?

[pichlermarc]

Looks good, thanks @seidelmartin 🙂
Once this is merged, a release PR is created automatically.

@blumamir do we have any specific limitations on bumping typescript for a single package in this repo? 🤔 Since this package is not 1.0 yet, we can make a breaking change like bumping it for @opentelemetry/instrumentation-fastify (it's needed for new versions of the package anyway). Thinking about it closer though, it'll likely affect @opentelemetry/auto-instrumentations-node too by possibly requiring a later version for consumers.

[blumamir]

Looks good, thanks @seidelmartin 🙂 Once this is merged, a release PR is created automatically.

@blumamir do we have any specific limitations on bumping typescript for a single package in this repo? 🤔 Since this package is not 1.0 yet, we can make a breaking change like bumping it for @opentelemetry/instrumentation-fastify (it's needed for new versions of the package anyway). Thinking about it closer though, it'll likely affect @opentelemetry/auto-instrumentations-node too by possibly requiring a later version for consumers.

To my latest understanding, if we use a newer typescript, we can break existing users with incompatible versions. But @Flarna will probably know best

[Flarna]

Newer typescript versions may generated .d.ts files which are incompatible with an older typescript version.
I don't have in mind which ts version includes such problematic changes but it's not bound to major versions only. I guess a search in the issue tracker of core repo has more details.

While typescript brings a lot of benefits it adds also some compatibility burden...

An update of typescript will for sure happen with SDK 2.0.

[blumamir]

Why do we need to bump the typescript version in the first place? is it so we can use "fastify": "4.23.0" instead of 4.18.0 to verify the tests assert this fix?

Perhaps we can keep all current dependencies as they are and use test-all-versions script to cover all versions instrumentation supports?

[seidelmartin]

Why do we need to bump the typescript version in the first place? is it so we can use "fastify": "4.23.0" instead of 4.18.0 to verify the tests assert this fix?

Perhaps we can keep all current dependencies as they are and use test-all-versions script to cover all versions instrumentation supports?

I've updated fastify to v4.23.0 because it exposes new properties that instrumentation uses. Otherwise it still produces a lot of deprecation warnings. I can still downgrade it but then I wouldn't be able to test my changes.

[Fernandomr88]

Guys, this is generating tons of errors in production, what's the ETA of this PR?

[pichlermarc]

Thanks to everyone for the feedback on the PR here.

I've created a PR to add a test-all-versions script that will allow us to keep fastify@4.18.0/typescript@4.4.4, while at the same time also allowing us to test against fastify@4.23.2/typescript@4.7.4 (#1710). This way we won't run into compatibility issues with users of typescript@4.4.4.

@seidelmartin would you mind changing the dependencies back to fastify@4.18.0/typescript@4.4.4? Once #1710 is merged we'll then automatically test against fastify@4.23.2 when the pkg:instrumentation-fastify label is applied - and once that passes we can merge this PR. 🙂

[seidelmartin]

Yes, I'll do that.

[seidelmartin]

@pichlermarc I've put back the version of those dependencies.

[markrzen]

@seidelmartin You may want to rebase to main as it appears you are out-of-date.

[seidelmartin]

@markrzen I've rebased to latest version. Can we proceed with it? What will be next steps?

[markrzen]

@seidelmartin I don't have access to approve, I am just another contributor like yourself.

@pichlermarc Anything else to get this PR approved?

[Fernandomr88]

Guys, this is generating tons of errors in production, what's the ETA of this PR?

Is anything missing to get this approved @pichlermarc? 😅

[pichlermarc]

Was just waiting for #1710 so that this does not get merged without the test-all-versions script running. I merged main again here, once the build passes I'll merge this PR 🙂

[Fernandomr88]

awesome! When will this be available @ Npm?

[pichlermarc]

@Fernandomr88 we released it earlier today: https://github.com/open-telemetry/opentelemetry-js-contrib/releases/tag/instrumentation-fastify-v0.32.3