feat(instrumentation-hapi): support v21

[nlochschmidt]

Which problem is this PR solving?

• Support hapi v21 #1383

Short description of the changes

Continuation of stale #1491

• Updates Hapi dependency to v21
• Uses types shipped with library instead of @types/hapi__hapi
• Adds Hapi v21 to the list of tested versions
• Supports tracing when route.options is a function

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.47%. Comparing base (dfb2dff) to head (94804e0).
Report is 90 commits behind head on main.

❗ Current head 94804e0 differs from pull request most recent head 9db0505. Consider uploading reports for the commit 9db0505 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1985      +/-   ##
==========================================
- Coverage   90.97%   90.47%   -0.51%     
==========================================
  Files         146      147       +1     
  Lines        7492     7590      +98     
  Branches     1502     1575      +73     
==========================================
+ Hits         6816     6867      +51     
- Misses        676      723      +47     

Files	Coverage Δ
...emetry-instrumentation-hapi/src/instrumentation.ts	99.31% <100.00%> (+<0.01%)	⬆️
...lemetry-instrumentation-hapi/src/internal-types.ts	100.00% <ø> (ø)
...de/opentelemetry-instrumentation-hapi/src/utils.ts	100.00% <100.00%> (ø)

... and 27 files with indirect coverage changes

[pichlermarc]

I need to try this out still, but looks good at first glance - thanks for taking care of this 🙂
Two comments for now:

[nlochschmidt]

@pichlermarc did you have a chance to look at this yet?

[nlochschmidt]

@trentm given your experience with hapi, maybe you could review this PR? 🙏

[trentm]

LGTM, just the .tav.yml change that I think should be added, and a Q about dropping the @types/hapi__hapi dep. Tests do pass, so I don't think my Q about the types needs to hold this up.

[trentm]

Do you understand why this statement was added in #1595 originally? I would guess it was left over debugging code, but I don't know.

[nlochschmidt]

That's what I thought. Since the tests are passing I assumed it was just a left-over. If not I would expect a comment here explaining why this is needed.

[pichlermarc]

@nlochschmidt please resolve the conflicts if you have some time. 🙂
Doing so will make the workflow run and that'll help allow us to see if the tests are all passing. 🙂

[nlochschmidt]

@pichlermarc good to know that tests only run when there are no conflicts 💡

I had to bring back the joi dev dependency to make the tests pass (see this comment)

[trentm]

LGTM

I defer to @pichlermarc or others if skipLibCheck:true on auto-instrumentations-node might be a preferable change.

[nlochschmidt]

I defer to @pichlermarc or others if skipLibCheck:true on auto-instrumentations-node might be a preferable change.

My 2c: I think adding joi as a devDependency is reasonable here, because joi comes from the hapi project. I consider this similar to other plugins (e.g. koa) bringing in multiple dev dependencies from the project it's instrumenting.

Disabling lib check on auto-instrumentation-node seems like a bigger discussion, and I wonder if it's worth waiting for a decision before merging this PR?

[trentm]

Just reproducing it myself, the build failure:

$ npm run compile
...
       > @opentelemetry/auto-instrumentations-node@0.43.0 compile
       > tsc -p .

       ../../node_modules/@hapi/hapi/lib/types/route.d.ts:2:68 - error TS2307: Cannot find module 'joi' or its corresponding type declarations.

       2 import { ObjectSchema, ValidationOptions, SchemaMap, Schema } from 'joi';
                                                                            ~~~~~

       ../../node_modules/@hapi/hapi/lib/types/server/server.d.ts:4:22 - error TS2307: Cannot find module 'joi' or its corresponding type declarations.

       4 import { Root } from 'joi';
                              ~~~~~


       Found 2 errors.

       npm ERR! Lifecycle script `compile` failed with error:
       npm ERR! Error: command failed
       npm ERR!   in workspace: @opentelemetry/auto-instrumentations-node@0.43.0
       npm ERR!   at location: /Users/trentm/tm/opentelemetry-js-contrib4/metapackages/auto-instrumentations-node
...

Note: we already have skipLibCheck on a subset of packages:

% rg skipLibCheck
plugins/node/opentelemetry-instrumentation-hapi/tsconfig.json:    "skipLibCheck": true
plugins/node/opentelemetry-instrumentation-mongodb/tsconfig.json:    "skipLibCheck": true
plugins/node/instrumentation-lru-memoizer/tsconfig.json:    "skipLibCheck": true
plugins/node/instrumentation-mongoose/tsconfig.json:    "skipLibCheck": true
plugins/node/instrumentation-cucumber/tsconfig.json:    "skipLibCheck": true
metapackages/auto-instrumentations-web/tsconfig.json:    "skipLibCheck": true
metapackages/auto-instrumentations-web/tsconfig.esm.json:    "skipLibCheck": true,
metapackages/auto-instrumentations-web/tsconfig.esnext.json:    "skipLibCheck": true,
plugins/web/opentelemetry-instrumentation-user-interaction/tsconfig.json:    "skipLibCheck": true
plugins/web/opentelemetry-instrumentation-user-interaction/tsconfig.esm.json:    "skipLibCheck": true,
plugins/web/opentelemetry-instrumentation-user-interaction/tsconfig.esnext.json:    "skipLibCheck": true,

My 2c ;), I am fine with either:

1. taking this PR as is, and separately considering using skipLibCheck:true in auto-instrumentations-node; or
2. adding skipLibCheck to auto-instrumentations-node now. (I think we should eventually.)

@pichlermarc Did you have a particular opinion?

[nlochschmidt]

I would love to see this PR getting merged. @trentm @pichlermarc can I do anything to make that happen?

[toddtarsi]

@nlochschmidt - Thanks for carrying this forward! I second that, if there's anything I could do to help here as well, I would be glad to.

[pichlermarc]

My 2c ;), I am fine with either:

1. taking this PR as is, and separately considering using `skipLibCheck:true` in auto-instrumentations-node; or

2. adding skipLibCheck to auto-instrumentations-node now. (I think we should eventually.)

@pichlermarc Did you have a particular opinion?

Sorry for taking so long to come back to this PR. 😨

In my initial review I was just very confused that joi is part of the public @hapi/hapi types, but not listed as a dependency or peerDependency anywhere in the @hapi/hapi package. As a result importing @hapi/hapi with TS seems to break by default. (I've never encountered that before as a deliberate choice)

My guess is this choice was maybe made as it can be used without joi in plain JS (?)

Anyway, I'm fine to

• keep the joi package as a devDependency for compilation
• this means we can remove skipLibCheck: true again (locally this compiles fine with joi installed, also tested that in CI at [test] hapi-theory #2159)

I'd prefer a joi devDependency over having skipLibCheck: true added anywhere. IMO we should only add it if there's no other way.

[trentm]

Thanks, Marc.

@nlochschmidt Are you able to drop the change to "plugins/node/opentelemetry-instrumentation-hapi/tsconfig.json" and deal with merge conflicts now? (Let me know if you want a hand with that.)

[nlochschmidt]

@trentm @pichlermarc PR is up-to-date again.

Note: Sorry about the force-push. I know you don't like that. It was just an accidentally pushed commit to HEAD that I immediately replaced with the correct one. I hope that's alright 🤞

[pichlermarc]

Thank you for working on this 👍