Skip to content

Commit 3c426a3

Browse files
committed
fix(stats_flusher,http_client): address Copilot review comments
- Enforce target.timeout_ms on each stats POST attempt so the retry loop stays bounded by configuration, instead of blocking on a hung connection past the configured flush timeout. - Bound the error-body preview captured from non-202 responses to 512 bytes, fall back to lossy UTF-8 instead of silently emptying on invalid encoding, and include the HTTP status in the error message. - Route HttpClientTrait::new_client through create_client(None, None, false) so the never-invoked fallback shares the same construction path and failure surface as the rest of the module.
1 parent 8a94026 commit 3c426a3

2 files changed

Lines changed: 33 additions & 20 deletions

File tree

bottlecap/src/traces/http_client.rs

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,15 @@ impl HttpClient {
4343
}
4444

4545
impl HttpClientTrait for HttpClient {
46+
#[allow(clippy::expect_used)]
4647
fn new_client() -> Self {
47-
// Used by libdd APIs that construct a default client when no
48-
// pre-configured one is supplied. Bottlecap's production code paths
49-
// build the client via `create_client` so this fallback only matches
50-
// libdatadog's `new_default_client` shape.
51-
let connector = libdd_common::connector::Connector::default();
52-
let proxy_connector = hyper_http_proxy::ProxyConnector::new(connector)
53-
.expect("failed to build default proxy connector");
54-
let inner = http_common::client_builder()
55-
.pool_max_idle_per_host(0)
56-
.build(proxy_connector);
57-
HttpClient { inner }
48+
// Required by `HttpClientTrait` but never invoked on bottlecap's code
49+
// paths — production builds the client via
50+
// `create_client(proxy, tls_cert, skip_ssl)`. Routing this fallback
51+
// through the same constructor keeps the failure mode and error
52+
// surface consistent with the rest of the module.
53+
create_client(None, None, false)
54+
.expect("building default proxy connector with default TLS should not fail")
5855
}
5956

6057
fn request(

bottlecap/src/traces/stats_flusher.rs

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,15 @@ impl StatsFlusher {
170170
}
171171
}
172172

173+
/// Maximum number of body bytes to surface in error messages.
174+
const ERROR_BODY_PREVIEW_BYTES: usize = 512;
175+
173176
/// Posts a serialized stats payload using the supplied client.
174177
///
175178
/// Equivalent to libdatadog's `stats_utils::send_stats_payload`, but uses the
176-
/// caller-provided client so bottlecap's proxy/TLS configuration is preserved.
179+
/// caller-provided client so bottlecap's proxy/TLS configuration is preserved,
180+
/// and enforces `target.timeout_ms` on each attempt so the surrounding retry
181+
/// loop stays bounded by configuration.
177182
async fn send_stats_payload(
178183
client: &HttpClient,
179184
target: &Endpoint,
@@ -188,14 +193,25 @@ async fn send_stats_payload(
188193
.header("DD-API-KEY", api_key)
189194
.body(Bytes::from(data))?;
190195

191-
let response = client
192-
.request(req)
193-
.await
194-
.map_err(|e| anyhow::anyhow!("Failed to send trace stats: {e}"))?;
195-
196-
if response.status() != http::StatusCode::ACCEPTED {
197-
let response_body = String::from_utf8(response.into_body().to_vec()).unwrap_or_default();
198-
anyhow::bail!("Server did not accept trace stats: {response_body}");
196+
let response = tokio::time::timeout(
197+
std::time::Duration::from_millis(target.timeout_ms),
198+
client.request(req),
199+
)
200+
.await
201+
.map_err(|_| anyhow::anyhow!("Stats request timed out after {} ms", target.timeout_ms))?
202+
.map_err(|e| anyhow::anyhow!("Failed to send trace stats: {e}"))?;
203+
204+
let status = response.status();
205+
if status != http::StatusCode::ACCEPTED {
206+
let body = response.into_body();
207+
let preview_len = body.len().min(ERROR_BODY_PREVIEW_BYTES);
208+
let preview = String::from_utf8_lossy(&body[..preview_len]);
209+
let truncated = if body.len() > preview_len {
210+
" (truncated)"
211+
} else {
212+
""
213+
};
214+
anyhow::bail!("Server did not accept trace stats (status {status}): {preview}{truncated}");
199215
}
200216
Ok(())
201217
}

0 commit comments

Comments
 (0)