Exclude ApiPlatform test HttpClient from instrumentation #235

cedricziel · 2024-02-11T17:30:46Z

The client doesn't support the on_progress option and makes tests fail that use the ApiPlatform test client

Longer term we should probably maintain a blacklist or level up this instrumentation significantly so we know the allowed options

codecov · 2024-02-11T17:31:55Z

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.90%. Comparing base (00bf8ba) to head (ae2de68).

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #235      +/-   ##
============================================
- Coverage     83.59%   82.90%   -0.69%     
+ Complexity     1057      980      -77     
============================================
  Files           104       96       -8     
  Lines          4389     3943     -446     
============================================
- Hits           3669     3269     -400     
+ Misses          720      674      -46

Flag	Coverage Δ
Aws:7.4	`86.02% <ø> (ø)`
Aws:8.0	`?`
Aws:8.1	`85.75% <ø> (ø)`
Aws:8.2	`85.75% <ø> (ø)`
Aws:8.3	`85.75% <ø> (ø)`
Context/Swoole:7.4	`0.00% <ø> (ø)`
Context/Swoole:8.0	`0.00% <ø> (ø)`
Context/Swoole:8.1	`0.00% <ø> (ø)`
Context/Swoole:8.2	`?`
Context/Swoole:8.3	`0.00% <ø> (ø)`
Instrumentation/CakePHP:8.0	`93.47% <ø> (ø)`
Instrumentation/CakePHP:8.1	`93.47% <ø> (ø)`
Instrumentation/CakePHP:8.2	`93.47% <ø> (ø)`
Instrumentation/CakePHP:8.3	`93.47% <ø> (ø)`
Instrumentation/CodeIgniter:8.0	`?`
Instrumentation/CodeIgniter:8.1	`?`
Instrumentation/CodeIgniter:8.2	`75.86% <ø> (ø)`
Instrumentation/CodeIgniter:8.3	`75.86% <ø> (ø)`
Instrumentation/ExtAmqp:8.2	`89.58% <ø> (ø)`
Instrumentation/ExtAmqp:8.3	`89.58% <ø> (ø)`
Instrumentation/Guzzle:8.0	`?`
Instrumentation/Guzzle:8.1	`?`
Instrumentation/Guzzle:8.2	`?`
Instrumentation/Guzzle:8.3	`?`
Instrumentation/HttpAsyncClient:8.0	`81.33% <ø> (ø)`
Instrumentation/HttpAsyncClient:8.1	`?`
Instrumentation/HttpAsyncClient:8.2	`?`
Instrumentation/HttpAsyncClient:8.3	`?`
Instrumentation/IO:8.2	`?`
Instrumentation/IO:8.3	`?`
Instrumentation/Laravel:8.0	`65.20% <ø> (ø)`
Instrumentation/Laravel:8.1	`65.20% <ø> (ø)`
Instrumentation/Laravel:8.2	`65.20% <ø> (ø)`
Instrumentation/Laravel:8.3	`65.20% <ø> (ø)`
Instrumentation/MongoDB:7.4	`80.55% <ø> (ø)`
Instrumentation/MongoDB:8.0	`?`
Instrumentation/MongoDB:8.1	`?`
Instrumentation/MongoDB:8.2	`?`
Instrumentation/MongoDB:8.3	`?`
Instrumentation/OpenAIPHP:8.1	`86.82% <ø> (ø)`
Instrumentation/OpenAIPHP:8.2	`?`
Instrumentation/OpenAIPHP:8.3	`86.82% <ø> (ø)`
Instrumentation/PDO:8.2	`?`
Instrumentation/PDO:8.3	`?`
Instrumentation/Psr14:8.0	`80.64% <ø> (ø)`
Instrumentation/Psr14:8.1	`80.64% <ø> (ø)`
Instrumentation/Psr14:8.2	`?`
Instrumentation/Psr14:8.3	`80.64% <ø> (ø)`
Instrumentation/Psr15:8.0	`93.50% <ø> (ø)`
Instrumentation/Psr15:8.1	`93.50% <ø> (ø)`
Instrumentation/Psr15:8.2	`?`
Instrumentation/Psr15:8.3	`?`
Instrumentation/Psr16:8.0	`97.50% <ø> (ø)`
Instrumentation/Psr16:8.1	`97.50% <ø> (ø)`
Instrumentation/Psr16:8.2	`?`
Instrumentation/Psr16:8.3	`?`
Instrumentation/Psr18:8.0	`82.08% <ø> (ø)`
Instrumentation/Psr18:8.1	`82.08% <ø> (ø)`
Instrumentation/Psr18:8.2	`?`
Instrumentation/Psr18:8.3	`?`
Instrumentation/Psr3:8.0	`63.51% <ø> (ø)`
Instrumentation/Psr3:8.1	`63.51% <ø> (ø)`
Instrumentation/Psr3:8.2	`63.51% <ø> (ø)`
Instrumentation/Psr3:8.3	`63.51% <ø> (ø)`
Instrumentation/Psr6:8.0	`97.61% <ø> (ø)`
Instrumentation/Psr6:8.1	`97.61% <ø> (ø)`
Instrumentation/Psr6:8.2	`?`
Instrumentation/Psr6:8.3	`97.61% <ø> (ø)`
Instrumentation/Slim:8.0	`?`
Instrumentation/Slim:8.1	`?`
Instrumentation/Slim:8.2	`?`
Instrumentation/Slim:8.3	`?`
Instrumentation/Symfony:8.0	`92.55% <41.66%> (-2.29%)`	⬇️
Instrumentation/Symfony:8.1	`92.55% <41.66%> (-2.29%)`	⬇️
Instrumentation/Symfony:8.2	`?`
Instrumentation/Symfony:8.3	`92.55% <41.66%> (-2.29%)`	⬇️
Instrumentation/Yii:8.0	`?`
Instrumentation/Yii:8.1	`79.82% <ø> (ø)`
Instrumentation/Yii:8.2	`?`
Instrumentation/Yii:8.3	`?`
Logs/Monolog:7.4	`100.00% <ø> (ø)`
Logs/Monolog:8.0	`100.00% <ø> (ø)`
Logs/Monolog:8.1	`100.00% <ø> (ø)`
Logs/Monolog:8.2	`100.00% <ø> (ø)`
Logs/Monolog:8.3	`100.00% <ø> (ø)`
Propagation/ServerTiming:8.0	`?`
Propagation/ServerTiming:8.1	`100.00% <ø> (ø)`
Propagation/ServerTiming:8.2	`?`
Propagation/ServerTiming:8.3	`100.00% <ø> (ø)`
Propagation/TraceResponse:7.4	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.0	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.1	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.2	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.3	`?`
ResourceDetectors/Azure:7.4	`?`
ResourceDetectors/Azure:8.0	`91.66% <ø> (ø)`
ResourceDetectors/Azure:8.1	`?`
ResourceDetectors/Azure:8.2	`?`
ResourceDetectors/Azure:8.3	`91.66% <ø> (ø)`
ResourceDetectors/Container:8.0	`93.02% <ø> (ø)`
ResourceDetectors/Container:8.1	`?`
ResourceDetectors/Container:8.2	`93.02% <ø> (ø)`
ResourceDetectors/Container:8.3	`93.02% <ø> (ø)`
Shims/OpenTracing:7.4	`92.99% <ø> (ø)`
Shims/OpenTracing:8.0	`?`
Shims/OpenTracing:8.1	`?`
Shims/OpenTracing:8.2	`92.99% <ø> (ø)`
Shims/OpenTracing:8.3	`92.99% <ø> (ø)`
Symfony:7.4	`88.43% <ø> (ø)`
Symfony:8.0	`88.20% <ø> (ø)`
Symfony:8.1	`?`
Symfony:8.2	`88.20% <ø> (ø)`
Symfony:8.3	`88.20% <ø> (ø)`

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

Files	Coverage Δ
...entation/Symfony/src/HttpClientInstrumentation.php	`87.65% <41.66%> (-8.06%)`	⬇️

... and 8 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 00bf8ba...ae2de68. Read the comment docs.

brettmc · 2024-02-12T13:48:29Z

If the pre hook doesn't start+activate a span, then the post hook could potentially deactivate some other scope, couldn't it? If so, might it be safer to create, activate and then end a span here so that the post hook works as expected? (I don't have access to a pc to actually test this, though)

src/Instrumentation/Symfony/src/HttpClientInstrumentation.php

brettmc

I think the pre hook should be unchanged, and the active span closed in the post callback if the client doesn't support on_progress. This way, a span is still created and activated in pre hook, and post hook cannot detach the wrong scope (which I'm pretty sure it will do, with this change).

bobstrecansky · 2024-03-20T12:05:54Z

@cedricziel - are you still planning on working on this PR?

nesl247 · 2024-03-29T16:44:25Z

We also just ran into this issue, but also ran into when using prophecy to mock the HttpClientInterface. In the case of prophecy, the issue is that because the instrumentation changes the client, it doesn't match the assertions.

$httpClient = $this->prophesize(HttpClientInterface::class);

$httpClient->request('GET', 'https://example.com')
            ->willReturn(new Response());

cedricziel · 2024-06-02T10:51:08Z

The problem is that Symfony will evaluate the $options array and will fail if the client doesn't support one of the provided options.

I'm adapting the instrumentation so that if will create a span, but need to close it manually in post hook as the callback method won't work.

cedricziel · 2024-06-02T11:21:56Z

I think the AMQP error is unrelated...

The client doesn't support the `on_progress` option.

Co-authored-by: Tobias Bachert <git@b-privat.de>

cedricziel · 2024-06-16T11:02:17Z

rebased. Unrelated style issues

cedricziel requested a review from a team February 11, 2024 17:30

agoallikmaa approved these changes Feb 12, 2024

View reviewed changes

Nevay reviewed Feb 12, 2024

View reviewed changes

src/Instrumentation/Symfony/src/HttpClientInstrumentation.php Outdated Show resolved Hide resolved

brettmc requested changes Feb 28, 2024

View reviewed changes

cedricziel requested a review from brettmc June 2, 2024 11:07

cedricziel and others added 9 commits June 16, 2024 12:53

Exclude ApiPlatform test HttpClient from instrumentation

949bad2

The client doesn't support the `on_progress` option.

Ignore missing class

8ef9462

Update src/Instrumentation/Symfony/src/HttpClientInstrumentation.php

26acb1d

Co-authored-by: Tobias Bachert <git@b-privat.de>

Add list of non supported clients

84837bf

Return early if exception

1224bbc

Suppress undefined

e02873f

Use string ref

6f1f669

Negate condition

6694910

Rename const and guard

ae2de68

cedricziel force-pushed the patch-3 branch from 29e7808 to ae2de68 Compare June 16, 2024 10:54

brettmc approved these changes Jun 20, 2024

View reviewed changes

brettmc merged commit 9f96b59 into open-telemetry:main Jun 20, 2024
110 of 116 checks passed

This was referenced Jul 1, 2024

Fix namespace to exclude ApiPlatform test HttpClient from instrumentation #276

Closed

Fix namespace to exclude ApiPlatform test HttpClient from instrumentation #277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude ApiPlatform test HttpClient from instrumentation #235

Exclude ApiPlatform test HttpClient from instrumentation #235

cedricziel commented Feb 11, 2024 •

edited

Loading

codecov bot commented Feb 11, 2024 •

edited

Loading

brettmc commented Feb 12, 2024

brettmc left a comment

bobstrecansky commented Mar 20, 2024

nesl247 commented Mar 29, 2024 •

edited

Loading

cedricziel commented Jun 2, 2024

cedricziel commented Jun 2, 2024

cedricziel commented Jun 16, 2024

Exclude ApiPlatform test HttpClient from instrumentation #235

Exclude ApiPlatform test HttpClient from instrumentation #235

Conversation

cedricziel commented Feb 11, 2024 • edited Loading

codecov bot commented Feb 11, 2024 • edited Loading

Codecov Report

brettmc commented Feb 12, 2024

brettmc left a comment

Choose a reason for hiding this comment

bobstrecansky commented Mar 20, 2024

nesl247 commented Mar 29, 2024 • edited Loading

cedricziel commented Jun 2, 2024

cedricziel commented Jun 2, 2024

cedricziel commented Jun 16, 2024

cedricziel commented Feb 11, 2024 •

edited

Loading

codecov bot commented Feb 11, 2024 •

edited

Loading

nesl247 commented Mar 29, 2024 •

edited

Loading