spiral / framework

High-Performance PHP Framework
https://spiral.dev
MIT License
1.82k stars 89 forks source link

[OTEL] Cast request URI to string for http.url trace attribute #1126

Closed devnev closed 2 months ago

devnev commented 2 months ago
Q A
Bugfix? ✔️
Breaks BC?
New feature?
Issues #1125
Docs PR

As described in #1125, opentelemetry-php balks because http.url is set from $request->getUri(), which is not a primitive or array of primitives, as it is a UriInterface. Cast it to a string to fix.

Fixes #1125

roxblnfk commented 2 months ago

Hi,

I've reverted the changes (casting to string) and then run new tests. The tests passed. Are there any specific conditions required for reproducing the bug in the test environment?

Are you using Nyholm's PSR library?

devnev commented 2 months ago

Maybe some other part of the test is implicitly casting the Uri to a string?

The server where we're having the issue has in its composer.lock

            "name": "nyholm/psr7",
            "version": "1.8.1",
devnev commented 2 months ago

I just tried changing the mock to

            ->with(
                "GET http://example.org/path",
                m::any(),
                m::on(function ($attributes) {
                    $this->assertSame($attributes, [
                        "http.method" => "GET",
                        "http.url" => "http://example.org/path",
                        "http.headers" => [
                            "Host" => ["example.org"],
                            "foo" => ["bar"],
                        ],
                    ]);
                    $this->assertSame(
                        $attributes["http.url"],
                        "http://example.org/path"
                    );
                }),
                true,
                TraceKind::SERVER
            )

and got

   ├ Failed asserting that two arrays are identical.
   ┊ ---·Expected
   ┊ +++·Actual
   ┊ @@ @@
   ┊  Array &0 [
   ┊      'http.method' => 'GET',
   ┊ -····'http.url'·=>·Nyholm\Psr7\Uri·Object·#58155·(
   ┊ -········'scheme'·=>·'http',
   ┊ -········'userInfo'·=>·'',
   ┊ -········'host'·=>·'example.org',
   ┊ -········'port'·=>·null,
   ┊ -········'path'·=>·'/path',
   ┊ -········'query'·=>·'',
   ┊ -········'fragment'·=>·'',
   ┊ -····),
   ┊ +····'http.url'·=>·'http://example.org/path',
   ┊      'http.headers' => Array &1 [
   ┊          'Host' => Array &2 [
   ┊              0 => 'example.org',
roxblnfk commented 2 months ago

I just tried changing the mock to

yeah. Looks like mocker casts Stringable to string under the hood

devnev commented 2 months ago

Instead of asserting the type of just the one attribute, maybe the test should assert the entire set of attributes with assertSame?

roxblnfk commented 2 months ago

Instead of asserting the type of just the one attribute, maybe the test should assert the entire set of attributes with assertSame?

makes sense

devnev commented 2 months ago

Looks like mockery has a mustBe method that might be more exact, I'll try that

devnev commented 2 months ago

Ah you switched it to $this->createMock(). I'll leave it as is then, although it feels like it makes the assertion messier compared to using mockery.

devnev commented 2 months ago

Thanks for the review 😄