Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HookManagerInterface annotation #1395

Conversation

ChrisLightfootWild
Copy link
Contributor

@ChrisLightfootWild ChrisLightfootWild commented Oct 2, 2024

Align OpenTelemetry\API\Instrumentation\AutoInstrumentation\HookManagerInterface::hook postHook Closure signature with OpenTelemetry\Instrumentation\hook.

Phan complains in open-telemetry/opentelemetry-php-contrib#269. Upon checking, I think the annotated signature of the post-hook is possibly incorrect?

This is an example of what I am seeing:.

In pre-hook:

src/Hooks/Illuminate/Contracts/Http/Kernel.php:48 PhanTypeMismatchArgumentSuperType Argument 3 ($preHook) is (function) of type Closure(\Illuminate\Contracts\Http\Kernel,array,string,string,?string,?int):array{0:?\Illuminate\Http\Request} but \OpenTelemetry\API\Instrumentation\AutoInstrumentation\HookManagerInterface::hook() takes Closure(null|object|string,array,string,string,null|string,int|null):void|null defined at vendor/open-telemetry/api/Instrumentation/AutoInstrumentation/HookManagerInterface.php:17 (expected type to be the same or a subtype, but saw a supertype instead)

In post-hook:

\OpenTelemetry\API\Instrumentation\AutoInstrumentation\HookManagerInterface::hook() takes Closure(null|object|string,array,mixed,\Throwable|null):void|null defined at vendor/open-telemetry/api/Instrumentation/AutoInstrumentation/HookManagerInterface.php:17 (expected type to be the same or a subtype, but saw a supertype instead

Edit: it was late and I pasted an example of pre-hook instead of post-hook. Both were throwing phan errors.

@ChrisLightfootWild ChrisLightfootWild requested a review from a team as a code owner October 2, 2024 23:21
Copy link

netlify bot commented Oct 2, 2024

Deploy Preview for opentelemetry-php canceled.

Name Link
🔨 Latest commit b7eff27
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-php/deploys/670025110434c6000847d9a4

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.90%. Comparing base (0ae2723) to head (b7eff27).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1395      +/-   ##
============================================
+ Coverage     73.88%   73.90%   +0.02%     
  Complexity     2672     2672              
============================================
  Files           386      386              
  Lines          7650     7650              
============================================
+ Hits           5652     5654       +2     
+ Misses         1998     1996       -2     
Flag Coverage Δ
8.1 73.55% <ø> (+0.10%) ⬆️
8.2 73.72% <ø> (-0.08%) ⬇️
8.3 73.66% <ø> (-0.01%) ⬇️
8.4 73.70% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ae2723...b7eff27. Read the comment docs.

@ChrisLightfootWild ChrisLightfootWild marked this pull request as draft October 2, 2024 23:25
@ChrisLightfootWild ChrisLightfootWild force-pushed the instrumentation-post-hook-signature branch from 469b476 to 99c2166 Compare October 3, 2024 08:57
@ChrisLightfootWild ChrisLightfootWild marked this pull request as ready for review October 3, 2024 08:59
@ChrisLightfootWild ChrisLightfootWild force-pushed the instrumentation-post-hook-signature branch from 99c2166 to 8a856db Compare October 3, 2024 09:01
…erInterface::hook signatures with OpenTelemetry\Instrumentation\hook.
@ChrisLightfootWild ChrisLightfootWild force-pushed the instrumentation-post-hook-signature branch from 8a856db to 2061923 Compare October 3, 2024 09:08
@brettmc brettmc merged commit cd8c0e3 into open-telemetry:main Oct 5, 2024
14 checks passed
@ChrisLightfootWild ChrisLightfootWild deleted the instrumentation-post-hook-signature branch October 5, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants