fix: update hapi bindings to v21 #1491

toddtarsi · 2023-05-09T14:18:25Z

Which problem is this PR solving?

Support hapi v21 #1383

Short description of the changes

Change compat to v21 and up and change the type libraries to use hapi internal

linux-foundation-easycla · 2023-05-09T14:18:31Z

The committers listed above are authorized under a signed CLA.

✅ login: toddtarsi / name: Todd Tarsi (41a45e5)

Flarna · 2023-05-09T14:55:17Z

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

@@ -57,11 +57,15 @@ export class HapiInstrumentation extends InstrumentationBase {
  protected init() {
    return new InstrumentationNodeModuleDefinition<typeof Hapi>(
      HapiComponentName,
-      ['>=17 <21'],
+      ['>=21'],


I guess the desupport of hapi 17 to 20 should be mentioned somewhere.

Lol thats fair. I honestly don't know if we can't keep the support. Problem is not knowing isn't very useful.

Some instrumentations have tests for more versions via test all versions. I never used it by myself so can't provide a lot details. e.g. here is a config file holding a version list.
There are also github actions. e.g. here

But not sure if you are willing to take the task to setup tav for hapi.

>=21 is also quite the promise. Can we be sure 22, 23, 30, 50 are all supported?

Towerism · 2023-05-09T19:35:28Z

Firstly, thank you @toddtarsi!

I ran npm run compile so I could test this in our own project. However, it errored. Here is the output:

npm run compile

> @opentelemetry/instrumentation-hapi@0.31.2 precompile
> tsc --version && lerna run version:update --scope @opentelemetry/instrumentation-hapi --include-dependencies

Version 4.4.4
lerna notice cli v5.5.2
lerna info versioning independent
lerna notice filter including "@opentelemetry/instrumentation-hapi"
lerna notice filter including dependencies
lerna info filter [ '@opentelemetry/instrumentation-hapi' ]
lerna info Executing command in 1 package: "npm run version:update"
lerna info run Ran npm script 'version:update' in '@opentelemetry/instrumentation-hapi' in 1.2s:

> @opentelemetry/instrumentation-hapi@0.31.2 version:update
> node ../../../scripts/version-update.js

lerna success run Ran npm script 'version:update' in 1 package in 1.2s:
lerna success - @opentelemetry/instrumentation-hapi

> @opentelemetry/instrumentation-hapi@0.31.2 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.

toddtarsi · 2023-05-15T14:20:21Z

Just noting, I was looking at it this weekend and I definitely just need to do the taz thing. Without that up, I am just going in circles a bit, sorry all!

github-actions · 2023-07-31T06:31:29Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

IAmVisco · 2023-08-01T14:20:02Z

@toddtarsi sorry to bother, are you planning on finishing this one? I was working on something of my own and noticed that installing open-telemetry would downgrade hapi to v20. I can pick it up and take a look.

toddtarsi · 2023-08-01T14:21:48Z

@IAmVisco - Nah. I'm not sure what the taz stuff is tbh.

IAmVisco · 2023-08-01T14:23:48Z

@IAmVisco - Nah. I'm not sure what the taz stuff is tbh.

Sorry, mind linking what taz is? I don't see anything in the PR and you mentioned it first in your earlier comment.

toddtarsi · 2023-08-01T14:24:37Z

@IAmVisco - Nah. I'm not sure what the taz stuff is tbh.

Sorry, mind linking what taz is? I don't see anything in the PR and you mentioned it first in your earlier comment.

#1491 (comment)

IAmVisco · 2023-08-01T14:45:45Z

@toddtarsi Just tested some stuff locally - if you do npm i -D joi (it comes with types already) and lower >=21 to just >=17 (hapi site still supports v17) it should work fine. Mind giving it a try? If all else fails I will fork and fiddle with it in my own PR tomorrow-ish. Cheers!

P.S. I don't see any issues with TAV on the first glance, but might be wrong.

mjsalinger · 2023-09-19T14:36:48Z

Is there any chance this can get released?

arimus · 2023-11-05T05:14:00Z

Looks like 2 tests failing. My guess it that this needs to be fixed, but also that there may need to be a new major or minor version to release a new version that depends on Hapi 21. One build can not unfortunately easily work for multiple versions of Hapi without a lot of casting to essentially using un-typed JS to get the job done, so not sure how TAV applies here.

Trying to debug the errors, but the monkey patching is a little complicated.

  23 passing (262ms)
  2 failing

  1) Hapi Instrumentation - Core Tests
       Instrumenting Hapi Routes
         should update rpcMetadata.route information:
     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ undefined
- '/users/{userId}'
      at /Users/arimus/workspace/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-hapi/test/hapi.test.ts:407:18
      at processTicksAndRejections (internal/process/task_queues.js:95:5)
      at Context.<anonymous> (test/hapi.test.ts:394:7)

  2) Hapi Instrumentation - Core Tests
       Instrumenting Hapi Routes
         when handler is in route.options level
           should create a child span for single routes:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:

1 !== 2

      + expected - actual

      -1
      +2

      at /Users/arimus/workspace/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-hapi/test/hapi.test.ts:136:20
      at processTicksAndRejections (internal/process/task_queues.js:95:5)
      at Context.<anonymous> (test/hapi.test.ts:126:9)

I found a fix that allows the test to pass (instrumentation.ts, line:393), although I'm not positive what the implications are on a broader level.

        const rpcMetadata = getRPCMetadata(api.context.active());
        if (rpcMetadata?.type === RPCType.HTTP) {
          // @ts-ignore
          rpcMetadata?.route = route.path; // <<<<<< ADDED - set the route path onto the rpcMetadata object

          const rootSpanMetadata = getRootSpanMetadata(route);
          rpcMetadata.span.updateName(rootSpanMetadata.name);
          rpcMetadata.span.setAttributes(rootSpanMetadata.attributes);
        }

And I haven't yet determined why the span completion count is only 1 instead of 2 in this other test. Happy to help test if @toddtarsi wants to update this pull request and get the tests passing.

Some guidance from the author(s) would be good on what is necessary to get this merged outside of getting tests passing (e.g. versioning, etc.) Worst case, I guess a new NPM module can be created to support Hapi v21 in the meantime, but keeping things here would be ideal.

sguimont · 2023-12-21T15:37:13Z

Hi, @arimus. We try to correct and run a PATCHED version of the HAPI plugin to runs correctly with HAPI 21. In your last comment, you provided a fix but we don't know how you have implemented the getRootSpanMetadata(route). Could you provide the code of that function please? It will help us a lot.

mjsalinger · 2024-01-22T19:32:24Z

@here Is there anything that can be done to move this issue along? Anything I could do to help?

nlochschmidt · 2024-02-25T11:34:27Z

@toddtarsi It looks to me as if the major hurdles came from upgrading type dependencies to hapi 21. What do you think about staying on @types/hapi__hapi for a little longer, but moving it to devDependencies as to not cause issues for folks?

This looks like it would work: nlochschmidt@a299190. I'd be happy to provide an alternative PR for hapi v21 support based on this.

nlochschmidt · 2024-02-28T00:07:26Z

@toddtarsi I had a closer look at your PR today and I think it's possible to restore support for versions 17 to 20. My solution requires a few more any, but with tav in place now, I think that might be worth it.

main...nlochschmidt:opentelemetry-js-contrib:update-hapi-to-v21

I added joi as a dev dependency as mentioned by @IAmVisco. I think the alternative is to use skipLibCheck: true in the hapi local tsconfig.json. I see some other modules doing the same, but I am not sure what's preferred around here.

If you want, you can pull my changes into this PR, otherwise I will create a new one.

nlochschmidt · 2024-04-29T17:33:20Z

This PR can be closed.

#1985 has been merged, which includes these changes.

blumamir · 2024-05-02T14:34:55Z

This PR can be closed.

#1985 has been merged, which includes these changes.

Thanks for updating here 🙏

toddtarsi requested a review from a team May 9, 2023 14:18

toddtarsi changed the title ~~update hapi bindings to v21~~ [fix] update hapi bindings to v21 May 9, 2023

toddtarsi changed the title ~~[fix] update hapi bindings to v21~~ fix: update hapi bindings to v21 May 9, 2023

update hapi bindings to v21

b85f015

toddtarsi force-pushed the update-hapi-to-v21 branch from 41a45e5 to b85f015 Compare May 9, 2023 14:29

Flarna reviewed May 9, 2023

View reviewed changes

pichlermarc added the pkg:instrumentation-hapi label May 11, 2023

github-actions bot added the stale label Jul 31, 2023

github-actions bot removed the stale label Aug 7, 2023

klaemo mentioned this pull request Oct 11, 2023

@opentelemetry/instrumentation-hapi not compatible with hapi 21 hapijs/hapi#4465

Closed

nlochschmidt mentioned this pull request Feb 25, 2024

chore(instrumentation-hapi): setup tav #1957

Merged

nlochschmidt mentioned this pull request Mar 2, 2024

feat(instrumentation-hapi): support v21 #1985

Merged

blumamir closed this May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update hapi bindings to v21 #1491

fix: update hapi bindings to v21 #1491

toddtarsi commented May 9, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 9, 2023 •

edited

Loading

Flarna May 9, 2023

toddtarsi May 9, 2023

Flarna May 9, 2023

dyladan May 25, 2023

Towerism commented May 9, 2023

toddtarsi commented May 15, 2023

github-actions bot commented Jul 31, 2023

IAmVisco commented Aug 1, 2023

toddtarsi commented Aug 1, 2023

IAmVisco commented Aug 1, 2023

toddtarsi commented Aug 1, 2023

IAmVisco commented Aug 1, 2023 •

edited

Loading

mjsalinger commented Sep 19, 2023

arimus commented Nov 5, 2023 •

edited

Loading

sguimont commented Dec 21, 2023

mjsalinger commented Jan 22, 2024

nlochschmidt commented Feb 25, 2024

nlochschmidt commented Feb 28, 2024

nlochschmidt commented Apr 29, 2024

blumamir commented May 2, 2024

fix: update hapi bindings to v21 #1491

fix: update hapi bindings to v21 #1491

Conversation

toddtarsi commented May 9, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented May 9, 2023 • edited Loading

Flarna May 9, 2023

Choose a reason for hiding this comment

toddtarsi May 9, 2023

Choose a reason for hiding this comment

Flarna May 9, 2023

Choose a reason for hiding this comment

dyladan May 25, 2023

Choose a reason for hiding this comment

Towerism commented May 9, 2023

toddtarsi commented May 15, 2023

github-actions bot commented Jul 31, 2023

IAmVisco commented Aug 1, 2023

toddtarsi commented Aug 1, 2023

IAmVisco commented Aug 1, 2023

toddtarsi commented Aug 1, 2023

IAmVisco commented Aug 1, 2023 • edited Loading

mjsalinger commented Sep 19, 2023

arimus commented Nov 5, 2023 • edited Loading

sguimont commented Dec 21, 2023

mjsalinger commented Jan 22, 2024

nlochschmidt commented Feb 25, 2024

nlochschmidt commented Feb 28, 2024

nlochschmidt commented Apr 29, 2024

blumamir commented May 2, 2024

toddtarsi commented May 9, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 9, 2023 •

edited

Loading

IAmVisco commented Aug 1, 2023 •

edited

Loading

arimus commented Nov 5, 2023 •

edited

Loading