Add debug headers to RPC requests#4582
Conversation
|
Claude finished @fleupold's task in 3m 41s —— View job PR Review: Add debug headers to RPC requests
Clean, well-documented, and well-tested change. The layer is correctly placed innermost (after A couple of non-blocking notes for context:
I couldn't run |
There was a problem hiding this comment.
Code Review
This pull request introduces a new RpcHeadersLayer transport layer that annotates outgoing HTTP RPC requests with headers containing the JSON-RPC methods, request IDs, and distributed-tracing request IDs. The feedback suggests optimizing the annotate function to avoid intermediate Vec and String allocations on every outgoing RPC request by using a single loop and writing directly into pre-allocated String buffers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn annotate(req: &mut RequestPacket, request_id: Option<&str>) { | ||
| let requests = req.requests_mut(); | ||
| let methods = requests | ||
| .iter() | ||
| .map(SerializedRequest::method) | ||
| .collect::<Vec<_>>() | ||
| .join(","); | ||
| let ids = requests | ||
| .iter() | ||
| .map(|r| r.id().to_string()) | ||
| .collect::<Vec<_>>() | ||
| .join(","); | ||
| let Some(last) = requests.last_mut() else { | ||
| return; | ||
| }; | ||
| let headers = last.headers_mut(); | ||
| if let Some(v) = header_value(&methods) { | ||
| headers.insert(HeaderName::from_static(METHOD), v); | ||
| } | ||
| if let Some(v) = header_value(&ids) { | ||
| headers.insert(HeaderName::from_static(REQUEST_ID), v); | ||
| } | ||
| if let Some(v) = request_id.and_then(header_value) { | ||
| headers.insert(HeaderName::from_static(TRACING_REQUEST_ID), v); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of annotate performs multiple allocations per RPC request (especially for batch requests) by mapping and collecting methods and IDs into intermediate Vecs before joining them. Since this function is executed on every outgoing RPC request, we can optimize it to use a single loop and write directly into pre-allocated String buffers, avoiding all intermediate Vec and String allocations.
fn annotate(req: &mut RequestPacket, request_id: Option<&str>) {
let requests = req.requests_mut();
let Some(last) = requests.last_mut() else {
return;
};
let mut methods = String::new();
let mut ids = String::new();
for (i, r) in requests.iter().enumerate() {
if i > 0 {
methods.push(',');
ids.push(',');
}
methods.push_str(r.method());
use std::fmt::Write as _;
let _ = write!(ids, "{}", r.id());
}
let headers = last.headers_mut();
if let Some(v) = header_value(&methods) {
headers.insert(HeaderName::from_static(METHOD), v);
}
if let Some(v) = header_value(&ids) {
headers.insert(HeaderName::from_static(REQUEST_ID), v);
}
if let Some(v) = request_id.and_then(header_value) {
headers.insert(HeaderName::from_static(TRACING_REQUEST_ID), v);
}
}References
- Prioritize findings related to resource leaks, unnecessary complexity, or potential security vulnerabilities. (link)
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
MartinquaXD
left a comment
There was a problem hiding this comment.
Logic makes sense to me. Instead of merging and then checking if it actually works, why not temporarily log the headers and run some e2e test?
| assert_eq!(headers[METHOD], "eth_call,eth_getBalance,eth_chainId"); | ||
| assert_eq!(headers[REQUEST_ID], "1,2,3"); |
There was a problem hiding this comment.
Seems a bit awkward to use, no? Wouldn't it be nicer to attach tuples of (method, id) so it's easier to figure out which method belongs to which ID?
| } | ||
|
|
||
| fn call(&mut self, mut req: RequestPacket) -> Self::Future { | ||
| let request_id = observe::tracing::distributed::request_id::from_current_span(); |
There was a problem hiding this comment.
IIUC this will never produce a useful ID because the annotation layer would live in the background task spawned by the batching layer, right?
I think getting usable request_ids could be achieved by writing the request_id header before the batching layer forwards the request into the background task. This layer here would then just have to aggregate all request id headers and add them to the last sub-request in the batch.
Alternatively this could also be done in a dedicated layer that comes before the batching layer - probably cleaner than doing that in the batching layer.
Description
It looks like we dropped our rpc debug headers when moving from ethcontracts to alloy. This makes it harder to trace requests through our whole stack and understand for instance which node responded to a specific RPC request. This PR re-creates the annotation logic and adds
X-Rpc-Method/X-Rpc-Request-Id/X-Request-Idrequest headers that our node proxies log.e68c266 (in particular crates/ethrpc/src/http.rs) contains the old logic.
Unlike the old logic, we now fold batch requests into comma separated lists (before we would have only very limited visibility). Given our current maximum batch size (30-40) I don't foresee the size increase to be an issue.
Changes
How to test
Unit tests. Once merged, check on the node-proxy that the logs are populated correctly again.