Migrate OTLP/OTAP/Arrow receivers to axum#1591
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review WalkthroughReplaces synchronous tiny_http accept loops with async axum/Tokio servers for arrow_ipc, otap, and otlp receivers. Each receiver now binds a non-blocking TcpListener, runs axum route handlers (e.g., Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
Comment |
ApprovabilityVerdict: Needs human review Major infrastructure refactor replacing the HTTP server framework (tiny_http → axum) across three receiver components. While the request handling logic appears preserved, changing the underlying HTTP serving implementation is a significant runtime behavior change that warrants human review. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the OTLP, OTAP, and Arrow IPC HTTP receivers in logfwd-io from tiny_http request loops to axum servers, while preserving the existing background runtime-thread model and graceful shutdown behavior.
Changes:
- Replaced per-receiver
tiny_httpaccept/read loops withaxumrouters +axum::serve(...)on a single-thread Tokio runtime. - Extended
BackgroundHttpTaskto support graceful shutdown for bothtiny_httpandaxumvia a unified shutdown handle. - Updated OTLP receiver tests/helpers to construct the new axum-backed background task.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/logfwd-io/src/otlp_receiver.rs | Migrates OTLP HTTP receiver to axum, adds request handler helpers, updates a test helper to use new_axum. |
| crates/logfwd-io/src/otap_receiver.rs | Migrates OTAP receiver to axum with an async handler and bounded-body reading. |
| crates/logfwd-io/src/arrow_ipc_receiver.rs | Migrates Arrow IPC receiver to axum, keeping decompression + backpressure semantics in an async handler. |
| crates/logfwd-io/src/background_http_task.rs | Generalizes shutdown/join to support both tiny_http and axum shutdown signaling. |
Change record_error/record_parse_error signatures from &Option<Arc<T>> to Option<&Arc<T>> and update all call sites. Update BackgroundHttpTask doc comment to mention axum instead of tiny_http. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d156838 to
205f954
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-io/src/otap_receiver.rs`:
- Around line 317-338: Extract the duplicated helpers declared_content_length
and read_limited_body into a new shared module (e.g., receiver_http) and update
otap_receiver, otlp_receiver, and arrow_ipc_receiver to import them;
specifically, move the functions currently in otap_receiver.rs into
receiver_http::{declared_content_length, read_limited_body}, make them pub (keep
signatures the same and preserve error types like StatusCode and MAX_BODY_SIZE),
and replace local definitions in each receiver file with use
receiver_http::{declared_content_length, read_limited_body} so all three callers
reference the shared implementations.
In `@crates/logfwd-io/src/otlp_receiver.rs`:
- Around line 414-428: read_limited_body currently always starts with an empty
Vec causing repeated reallocations; change its signature (or add a wrapper) to
accept an optional content_length (u64/usize) and, if content_length is present,
validate it is <= MAX_BODY_SIZE (return StatusCode::PAYLOAD_TOO_LARGE if not)
and pre-allocate the output buffer with Vec::with_capacity(content_length as
usize); otherwise keep the current incremental behavior. Update callers to pass
the request's Content-Length when available and trusted; keep existing frame
iteration (Body.frame(), frame.into_data(), saturation checks) unchanged to
preserve safety.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8972db59-7834-47aa-90a4-2ab9b78e6f5e
📒 Files selected for processing (4)
crates/logfwd-io/src/arrow_ipc_receiver.rscrates/logfwd-io/src/background_http_task.rscrates/logfwd-io/src/otap_receiver.rscrates/logfwd-io/src/otlp_receiver.rs
Summary
otlp_receiver,otap_receiver, andarrow_ipc_receiverfromtiny_httprequest loops toaxumservers running on the existing runtime-thread patternBackgroundHttpTaskwith an axum shutdown handle so drop still performs graceful shutdown and thread joinValidation
cargo fmt --checkcargo check -p logfwd-iocargo test -p logfwd-io --libcargo test -p logfwd-io arrow_ipc_receiver::tests:: -- --nocapturecargo test -p logfwd-io otap_receiver::tests:: -- --nocapturecargo test -p logfwd-io otlp_receiver::tests:: -- --nocaptureNote
Migrate OTLP, OTAP, and Arrow IPC receivers from
tiny_httpto axumReplaces the synchronous
tiny_httpserver in three receivers with async axum routers running on atokiocurrent-thread runtime inside a dedicated worker thread.otlp_receiver,otap_receiver,arrow_ipc_receiver) now binds astd::net::TcpListener, converts it to atokio::net::TcpListener, and serves an axumRouterwith graceful shutdown via aoneshotchannel.BackgroundHttpTaskgains anew_axumconstructor and aShutdownHandleenum to support bothtiny_http(unblock) and axum (oneshot send) shutdown paths onDrop.handle_otlp_request,handle_otap_request,handle_arrow_ipc_request), each enforcingContent-Lengthlimits, body size caps, and backpressure via the existing bounded channel.tiny_httplogic; the 429 response body in the Arrow IPC receiver is shorter; OTAP success responses now explicitly setapplication/x-protobufas theContent-Type.Macroscope summarized 205f954.