fix(guzzle): end span when Client::transfer() throws synchronously#629
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #629 +/- ##
============================================
+ Coverage 80.46% 80.51% +0.05%
+ Complexity 1669 1651 -18
============================================
Files 116 114 -2
Lines 6265 6185 -80
============================================
- Hits 5041 4980 -61
+ Misses 1224 1205 -19 Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
andreicodrutonofrei-sketch
force-pushed
the
fix/guzzle-transfer-sync-throw-post-hook
branch
from
6f778d1 to
eee81ca
Compare
| } | ||
|
|
||
| $this->assertCount(1, $this->storage); | ||
| $span = $this->storage->offsetGet(0); |
There was a problem hiding this comment.
The tests are failing, possibly around here. Are you able to run them locally to fix?
There was a problem hiding this comment.
Hey @ChrisLightfootWild, yes, I will work on fixing them as soon as possible.
There was a problem hiding this comment.
can you point me in the direction of what tests should I be focused on? For example the CodeIgniter ones fail for different reasons.
2 participants
Description
GuzzleHttp\Client::transfer()can throw synchronously before returning apromise — e.g. when invalid request options are supplied, since
applyOptions()runs outside
transfer()'s try/catch. In that case the extension invokes thetransferpost hook with anullreturn value.The post hook typed the promise as non-nullable (
PromiseInterface $promise), sothe runtime signature check failed: the hook was skipped, the span leaked (never
ended/detached), and an
E_CORE_WARNINGwas emitted:GuzzleHttp\Client::transfer(): OpenTelemetry: post hook invalid signature,
class=GuzzleHttp\Client function=transfer
This surfaces as a fatal error in error trackers such as Sentry.
Fix
?PromiseInterface $promise).null(synchronous throw), end the span — unless it wasalready ended via the exception branch — and return instead of dereferencing
null.
Test
Adds a regression test that triggers a synchronous throw via a numerically-indexed
headersoption and asserts the span is still ended with an error status.Related
Same class of "post hook invalid signature / span leak on throw" issue previously
fixed for other instrumentations: