bug: Set sentry-trace header when using tracing middleware

[DAcodedBEAT]

According to PSR-7, withHeader operates in the following manner:

Return an instance with the provided value replacing the specified header.
[...]
This method MUST be implemented in such a way as to retain the
immutability of the message, and MUST return an instance that has the
new and/or updated header and value.

(see: https://www.php-fig.org/psr/psr-7/)

Due to this, we must set $request to the modified request object (from the return value of
withHeader) in order to actually send the sentry-trace header.

[ste93cry]

Nice catch! Can you please update the CHANGELOG (remember to also reference the PR) and change the tests to ensure that no regression can happen in the future?

[DAcodedBEAT]

@ste93cry I updated the CHANGELOG and fixed up the tests (I had to rework the unit tests to properly test the request/response values from Guzzle.). Let me know what you think and thanks for the quick reply!

[ste93cry]

I'm not sure I understand why the entire testing class had to be changed. The lambda that is returned from GuzzleTracingMiddleware::trace() must be invoked by passing to it an handler, which is then called here:

sentry-php/src/Tracing/GuzzleTracingMiddleware.php

Line 83 in 6d1a6ee

return $handler($request, $options)->then($handlerPromiseCallback, $handlerPromiseCallback);

What we want to assert is that the request contains the sentry-trace header. To do it, it's enough to add the $request parameter to the handler and add the missing assertion...

sentry-php/tests/Tracing/GuzzleTracingMiddlewareTest.php

Line 107 in 6d1a6ee

$function = $middleware(static function () use ($expectedPromiseResult): PromiseInterface {

[DAcodedBEAT]

I wanted to make sure the middleware was fully-working from the Guzzle client's configuration perspective (with the added benefit of removing PHPStan violations in the baseline since Guzzle would've been introduced as a dev dependency by using their MockHandler) but you're right -- this can be just the assertion on the header.

I updated this in 72094da. If this test update is acceptable, I can either squash and force-push to my branch or you can do squash these commits on merge. 👍

[ste93cry]

Thank you!

[DAcodedBEAT]

Thanks for merging this @ste93cry - can't wait to use the distributed tracing capability!

I noticed this was flagged for the 3.6 milestone but 3.6.0 has already been tagged off. Is this going into a 3.6.1 or would this be in a new semver version?

[ste93cry]

Yes, since it's a bugfix it will be released with the next patch version. We usually wait a few days/weeks to see if more issues comes and then we make the release