feat: add http_client request logging #1018 #1019
Conversation
cody-why
force-pushed
the
feat-add-log-http-request
branch
from
7e392ce to
1ad8e9d
Compare
|
Thanks for submitting this! Will need to potentially have internal discussion on Monday to see if this is within scope for Rig (it should be, but there may be other things that have not been considered yet) - once that's all cleared I'll do the review |
joshua-mo-143
left a comment
joshua-mo-143
left a comment
There was a problem hiding this comment.
Some changes requested, check my comments.
I am not entirely sure if this is what we want given that theoretically there is nothing stopping you from just having this in your custom impl (... and reqwest has its own traces), but I think this could still serve a purpose given that pretty much every request has to pass through the HttpClientExt impl anyway.
| if tracing::enabled!(Level::TRACE) { | ||
| // Redact sensitive headers (e.g., Authorization) for logging | ||
| let mut redacted_headers = parts.headers.clone(); | ||
| redacted_headers.remove("Authorization"); |
There was a problem hiding this comment.
You will also need to remove any headers that contain "-key" (as this is often used to hold API keys, etc).
|
|
||
| fn log_request(parts: &Parts, body: &Bytes) { | ||
| if tracing::enabled!(Level::TRACE) { | ||
| // Redact sensitive headers (e.g., Authorization) for logging |
There was a problem hiding this comment.
Can be removed.
It's pretty obvious what this is doing
| Ok(req.header("Authorization", auth_header)) | ||
| } | ||
|
|
||
| static LOG_HTTP_BODY_MAX: AtomicUsize = AtomicUsize::new(8 * 1024); |
There was a problem hiding this comment.
Not really a fan of this static global. Is there a way we can do this per-type impl?
| @@ -0,0 +1,41 @@ | |||
| use std::fmt; | |||
There was a problem hiding this comment.
Please rename this file to escape.rs rather than util.rs.
Utility/helper files can generally lead to code smells if not well-maintained. I would like to be optimistic about this, but realistically it'll be a matter of when it will be a code smell and not if - so the best solution is to eliminate the problem entirely.
| let (parts, body) = req.into_parts(); | ||
|
|
||
| let body_bytes: Bytes = body.into(); | ||
| log_request(&parts, &body_bytes); |
There was a problem hiding this comment.
Is it possible to make an on/off toggle for this?
| { | ||
| let (parts, body) = req.into_parts(); | ||
|
|
||
| log_headers(&parts); |
There was a problem hiding this comment.
Is it possible to make an on/off toggle for this?
2 participants
Description: This merge introduces HTTP request logging capabilities to the http_client module. It adds support for previewing request bodies at the TRACE level, with configurable maximum preview length via max_log_body_preview. Sensitive headers like Authorization are redacted before logging. The implementation includes:
HttpLogConfigExt trait for client builders
set_max_log_body_preview function
Escape helper for safe byte logging
log_request and log_headers functions for structured trace output
This enhancement improves observability and debugging of outbound HTTP traffic.