Log original request ID when reusing shared in-flight quote requests#4300
Log original request ID when reusing shared in-flight quote requests#4300squadgazzz merged 2 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the request-sharing library to return a SharedResult structure from the shared_or_else method, which includes a boolean flag indicating if a request was reused. This change is implemented across the driver, liquidity-sources, and price-estimation crates, allowing for improved logging and tracking of shared in-flight requests. I have no feedback to provide.
squadgazzz
force-pushed
the
feat/request-sharing-observability
branch
from
29f4585 to
ef0d432
Compare
squadgazzz
added
the
hotfix
There was a problem hiding this comment.
LGTM but I have notes, ultimately my concern is based on "if someone changes the code structure ever so slightly we might lose this log" — I'm aware my alternative isn't great either but should be slightly harder to misuse
I've numbered some comments so you're able to read them in order if GH messes up
crates/request-sharing/src/lib.rs
Outdated
| /// The (possibly shared) future to await. | ||
| pub future: Shared<Fut>, |
There was a problem hiding this comment.
1 - IMO this could be a private field, and we'd implement Future for SharedResult instead
There was a problem hiding this comment.
Done — made future private and implemented Future for SharedResult, so callers can .await it directly.
| tracing::debug!( | ||
| original_request_id = ?response.request_id, | ||
| "reusing in-flight quote request" | ||
| ); |
There was a problem hiding this comment.
2 - we'd log this inside the Future's await and have the request_id be conditionally attached via span
There was a problem hiding this comment.
The request_id lives inside SharedTradeResponse — it's only available after the future resolves, so it can't be placed in a span before the .await. The .instrument(info_span!("request sharing", original_request_id = ?response.request_id)) pattern from comment 3 would reference response before it exists.
Keeping the post-await check seems like the right fit here since the span data depends on the result. Open to other ideas if you see a way around this.
| let shared = self.sharing.shared_or_else(query.clone(), fut); | ||
| let response = shared.future.await; | ||
|
|
||
| if shared.is_shared { | ||
| tracing::debug!( | ||
| original_request_id = ?response.request_id, | ||
| "reusing in-flight quote request" | ||
| ); | ||
| } |
There was a problem hiding this comment.
3 - which would turn this code into:
let shared_fut = self.sharing.shared_or_else(query.clone(), fut);
let response = if shared.is_shared {
shared.instrument(info_span!("request sharing", original_request_id = ?response.request_id)).await
} else {
shared.await
};
| .send() | ||
| .await | ||
| .map_err(|err| PriceEstimationError::EstimatorInternal(anyhow!(err)))?; | ||
| if response.status() == 429 { |
There was a problem hiding this comment.
Longer but no one needs to check what status code this is
| if response.status() == 429 { | |
| if response.status() == StatusCode::RateLimited { |
There was a problem hiding this comment.
Done — using StatusCode::TOO_MANY_REQUESTS.
| if let Ok(err) = serde_json::from_str::<dto::Error>(&text) { | ||
| PriceEstimationError::from(err) | ||
| } else { | ||
| PriceEstimationError::EstimatorInternal(anyhow!(err)) | ||
| } |
There was a problem hiding this comment.
| if let Ok(err) = serde_json::from_str::<dto::Error>(&text) { | |
| PriceEstimationError::from(err) | |
| } else { | |
| PriceEstimationError::EstimatorInternal(anyhow!(err)) | |
| } | |
| serde_json::from_str::<dto::Error>(&text) | |
| .map(PriceEstimationError::from) | |
| .map_err(|err| PriceEstimationError::EstimatorInternal(anyhow!(err))) |
There was a problem hiding this comment.
The suggestion as-is returns Result<PriceEstimationError, PriceEstimationError> which doesn't collapse into the map_err closure's return type. Used unwrap_or_else to get the same effect:
serde_json::from_str::<dto::Error>(&text)
.map(PriceEstimationError::from)
.unwrap_or_else(|_| PriceEstimationError::EstimatorInternal(anyhow!(err)))
- Implement Future for SharedResult, making the inner future private so callers use .await directly instead of accessing .future - Use StatusCode::TOO_MANY_REQUESTS instead of raw 429 - Refactor error parsing to use functional combinators
3 participants
Background
Fast and optimal quoters share the same
ExternalTradeFinderinstance (viaArc<dyn TradeFinding>), soRequestSharingalready deduplicates identical concurrent requests. However, when a hitchhiker reused an in-flight request, there was no trace linking it back to the original HTTP call, making it hard to debug why the optimal quoter failed to provide solutions, while the fast one succeeded for basically the same quote competition or vice versa.Changes
RequestSharing::shared_or_elseto return aSharedResultthat indicates whether an existing in-flight request was reused (is_sharedflag), improving observability of request deduplication.ExternalTradeFinder, the HTTP request ID (X-REQUEST-ID) is now embedded in the shared future's result. When a caller (e.g. the fast or optimal quoter) reuses an in-flight request instead of sending a new one, a debug log is emitted with theoriginal_request_id— making it possible to trace which HTTP request produced a given quote result.shared_or_elsecallers updated to use the new.futurefield (no behavioral change).How to test
Using staging, use the cowswap UI to get some quotes. Search for the new log.