fix(laravel): queue spans per request signature in ClientRequestWatcher - #711
Conversation
ClientRequestWatcher tracked in-flight spans in a single array slot keyed by sha1(method|url|body). Concurrent requests with an identical signature (e.g. via Http::pool(), or retries) overwrote each other's entry: the overwritten span was never ended (leaked) and the response/failure event got attributed to the wrong span. Spans are now queued per signature and matched back to their RequestSending/ResponseReceived/ConnectionFailed event in FIFO order, so identical concurrent requests no longer collide.
…tcher Sends two identical requests through Http::pool() and asserts each gets its own span correctly matched to its own response, reproducing the span collision fixed in the previous commit.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
============================================
- Coverage 87.11% 85.56% -1.56%
- Complexity 1227 1391 +164
============================================
Files 86 109 +23
Lines 4983 5838 +855
============================================
+ Hits 4341 4995 +654
- Misses 642 843 +201
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
60979e8 to
43c1642
Compare
…uest Codecov flagged shiftSpan()'s null-return branch as uncovered on the fix/laravel-client-request-watcher-span-collision PR: the existing tests only exercised the path where a response/failure always has a matching in-flight span.
43c1642 to
681436e
Compare
ChrisLightfootWild
left a comment
ChrisLightfootWild
left a comment
There was a problem hiding this comment.
There's an assumption that the response return in order, whereas that may not be the case.
I can see there is already a problem currently, but can we think of any other mechanism to help differentiate these requests? 🤔
The FIFO-per-signature queue introduced to fix span collisions still assumed responses resolve in the same order requests were sent. That assumption doesn't hold for concurrent requests (e.g. Http::pool()): an out-of-order response would pop the wrong queued span and misattribute its status/attributes. RequestSending, ResponseReceived, and ConnectionFailed all reference the same underlying PSR-7 request instance for a given attempt, so spans are now keyed by spl_object_id() of that instance instead of a content hash. This removes the ordering assumption entirely: two in-flight requests with identical method/URL/body can never share a key, regardless of resolution order.
Directly exercises ClientRequestWatcher with two identical in-flight requests, resolving the later-sent one first, and inspects the internal span map via reflection to confirm only the resolved request's entry is removed. This fails under the previous FIFO-per-signature implementation, which pops by send order rather than by which request actually responded.
Yes, there's a better mechanism: correlating requests by object identity instead of content. Firstly, the I considered a few ways to fix this:
Drawback: it pins the entire PSR-7 request (headers, body, etc.) in memory for as long as the entry sits unresolved.
Why did i chose 3In this specific code path, the request object is kept alive by Guzzle's handler/promise chain for the entire in-flight duration. And I also added |
2 participants
Summary
Motivation
ClientRequestWatchertracks in-flight HTTP client spans in a single array slot keyed bysha1(method|url|body).When two requests share the same signature and are in flight at the same time — e.g. identical calls sent through
Http::pool(), or a retried request — the secondrecordRequest()overwrites the first entry in$this->spans.The overwritten span is never ended (it leaks), and whichever
ResponseReceived/ConnectionFailedevent fires next gets attributed to the wrong span (or to none, once both slots have been consumed).What I have done
$spansfromarray<string, SpanInterface>toarray<string, list<SpanInterface>>: one queue per request signature instead of one slot.recordRequest()now appends the new span to the queue for its signature.recordResponse()/recordConnectionFailed()now pop the oldest span for that signature via a newshiftSpan()helper (FIFO), instead of reading/deleting a single keyed entry.Test Plans
test_it_matches_spans_to_responses_for_concurrent_identical_requeststoClientTest.php: sends two identicalGETrequests throughHttp::pool(), withHttp::sequence()returning a distinct status (200, then 500) for each, and asserts each recorded span carries the correct status and error state for its own response.ClientRequestWatcher.phpchange while keeping the test causes it to fail (only 1 span recorded instead of 2).