feat(server): serve the OTLP receiver role — flip RFC0003.16 green - #141
Conversation
Wires the receiver as a config-toggled ourios-server role (the RFC 0003 §9 process-model resolution). `receiver::serve` binds gRPC (tonic) + HTTP (axum) over ONE shared IngestPipeline backed by a single Wal (RFC 0008 §3.1), reusing the receiver crate's http::router + grpc::LogsReceiver. Both listeners run on the binary's tokio runtime alongside the compactor; a single watch channel fans graceful shutdown to both. Config (env): OURIOS_RECEIVER_ENABLED, OURIOS_RECEIVER_GRPC_ADDR (default 0.0.0.0:4317), OURIOS_RECEIVER_HTTP_ADDR (default 0.0.0.0:4318), OURIOS_WAL_ROOT. The server now shuts down on SIGINT *or* SIGTERM (k8s / nerdctl stop send SIGTERM); a failed final telemetry flush is logged, not fatal, so a clean shutdown exits 0 even when the metrics collector is unreachable. Flips RFC0003.16: a real-socket integration test spawns the binary on 127.0.0.1:0 (both transports), reads the reported ports, exports a batch over a real tonic gRPC client + a hand-rolled HTTP POST, SIGTERMs the process (via `kill -TERM` — no nix dep, no unsafe), waits for a clean exit, then replays the WAL to assert both OtlpBatch frames are durable (no acked batch lost). No dedup asserted. Stable across repeated runs. Also adds the 405-on-non-POST confirming test (axum's MethodRouter default) per the OTLP-conformance review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
More reviews will be available in 36 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements a complete OTLP receiver for the ourios-server binary, enabling it to ingest OpenTelemetry logs over both gRPC and HTTP transports. The receiver shares a single WAL and ingest pipeline across both protocols, coordinates graceful shutdown via a watch channel, and is exercised by an RFC0003.16 end-to-end integration test that validates durability, transport correctness, and clean shutdown behavior. ChangesServed-binary OTLP receiver end-to-end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Implements RFC0003.16 by adding an OTLP receiver “role” to ourios-server: the binary can now optionally bind OTLP/gRPC + OTLP/HTTP over a shared ingest pipeline backed by a single WAL, and the prior red-gate stub is replaced with a real-socket served-binary integration test that validates WAL-before-ack durability across both transports and graceful shutdown.
Changes:
- Added
receiver::serveto bind and run OTLP/gRPC (tonic) + OTLP/HTTP (axum) listeners over a sharedIngestPipeline. - Extended
ourios-serverconfig/env parsing and runtime to optionally start the receiver role and shutdown on SIGINT/SIGTERM, with best-effort telemetry flush. - Converted RFC0003.16 to a real-socket integration test and added an OTLP/HTTP 405-on-non-POST conformance test.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-server/tests/rfc0003_16_served_binary.rs | Replaces ignored stub with real-socket spawn/export/SIGTERM/shutdown + WAL replay assertions. |
| crates/ourios-server/src/receiver.rs | New receiver role module: binds gRPC+HTTP and serves them with shared pipeline + watch-driven shutdown. |
| crates/ourios-server/src/main.rs | Adds receiver role toggles + address reporting + SIGTERM handling + shutdown ordering and best-effort telemetry flush. |
| crates/ourios-server/Cargo.toml | Adds required deps (axum/tonic/opentelemetry-proto/WAL) and test-only deps for the new integration test. |
| crates/ourios-ingester/tests/http_transport_errors.rs | Adds explicit 405 test for non-POST requests to /v1/logs. |
| Cargo.lock | Updates lockfile for newly added dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut sigterm = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) | ||
| .map_err(|e| format!("install SIGTERM handler: {e}"))?; | ||
| let shutdown = tokio::select! { |
There was a problem hiding this comment.
Fixed — SIGTERM is now gated behind cfg(unix) in a terminate_signal helper; the binary compiles on non-Unix again with SIGINT/ctrl_c as the cross-platform path, and a SIGTERM-handler install failure is logged (non-fatal), leaving SIGINT in charge.
| #[tokio::test] | ||
| async fn rfc0003_16_served_binary_binds_round_trips_and_shuts_down() { |
There was a problem hiding this comment.
Fixed — the test file is now #![cfg(unix)] (it drives shutdown via kill -TERM, and the server's SIGTERM handling is Unix-only).
| let pid = child.id().expect("server pid"); | ||
| std::process::Command::new("kill") | ||
| .arg("-TERM") | ||
| .arg(pid.to_string()) | ||
| .status() | ||
| .expect("send SIGTERM"); |
There was a problem hiding this comment.
Fixed — the kill now uses tokio::process::Command and awaits the status, asserting kill_status.success() (no blocking the runtime; a failed kill is diagnosed).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-server/src/main.rs (1)
85-104: ⚡ Quick winConsider adding unit tests for
build_receiver_config.This function has non-trivial validation logic (enabled-value parsing,
wal_rootrequirement when enabled). Adding tests similar to the existingbuild_configtests would improve confidence and catch regressions.Example test cases:
- Disabled when
enabled_rawisNone,"0", or"false"- Enabled with valid addresses and
wal_root- Rejects missing
wal_rootwhen enabled- Rejects empty
wal_rootwhen enabled- Uses default addresses when unset
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-server/src/main.rs` around lines 85 - 104, Add unit tests for build_receiver_config to cover its enabled-value parsing, address defaults, and wal_root validation: write tests that call build_receiver_config with various enabled_raw values (None, "0", "false" should return Ok(None); "1"/"true"/"yes" should proceed), with grpc_raw/http_raw unset to ensure parse_addr uses DEFAULT_GRPC_ADDR/DEFAULT_HTTP_ADDR, with a valid wal_root PathBuf to return Ok(Some(ReceiverParams{...})), and failing cases where enabled is truthy but wal_root is None or an empty path to assert an Err with the expected message; reference build_receiver_config, ReceiverParams, and parse_addr in the tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ourios-server/src/main.rs`:
- Around line 85-104: Add unit tests for build_receiver_config to cover its
enabled-value parsing, address defaults, and wal_root validation: write tests
that call build_receiver_config with various enabled_raw values (None, "0",
"false" should return Ok(None); "1"/"true"/"yes" should proceed), with
grpc_raw/http_raw unset to ensure parse_addr uses
DEFAULT_GRPC_ADDR/DEFAULT_HTTP_ADDR, with a valid wal_root PathBuf to return
Ok(Some(ReceiverParams{...})), and failing cases where enabled is truthy but
wal_root is None or an empty path to assert an Err with the expected message;
reference build_receiver_config, ReceiverParams, and parse_addr in the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34bab1df-85b2-4ab5-8f5b-365de79f6932
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/ourios-ingester/tests/http_transport_errors.rscrates/ourios-server/Cargo.tomlcrates/ourios-server/src/main.rscrates/ourios-server/src/receiver.rscrates/ourios-server/tests/rfc0003_16_served_binary.rs
Review fixes: - Gate SIGTERM behind cfg(unix) via a `terminate_signal` helper — the binary compiles on non-Unix again (SIGINT/ctrl_c stays the cross-platform path; SIGTERM is the Unix extra), and a handler-install failure is logged, not fatal. - Mark the served-binary integration test `#![cfg(unix)]` (it drives shutdown with `kill -TERM`). - Send the kill via `tokio::process::Command` and await + assert its status (no blocking the runtime; failures are diagnosed). - Add unit tests for `build_receiver_config` (disabled-by-default, default + custom addresses, WAL-root-required-when-enabled, malformed address). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai added the suggested |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| .env("OURIOS_WAL_ROOT", &wal_root) | ||
| .stdout(Stdio::piped()) | ||
| .spawn() | ||
| .expect("spawn ourios-server"); |
There was a problem hiding this comment.
Fixed — set .kill_on_drop(true) on the spawned server Command, so an early return (timeout or assertion panic) reaps the child rather than leaking it.
Review fix: if the integration test returns early (address-read timeout or an assertion panic) the spawned ourios-server could be left running. Set kill_on_drop(true) so the child is reaped when its handle drops. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // A signal-handler setup failure is fatal: cancelling the compactor and | ||
| // exiting 0 would leave the server silently doing no work. |
There was a problem hiding this comment.
Fixed — the comment now says the SIGINT (ctrl_c) handler failure is fatal (the shutdown? propagates the ctrl_c result), and notes a SIGTERM-handler failure is non-fatal per terminate_signal.
Review nit: SIGTERM-handler failures are non-fatal now (terminate_signal), so the comment on the final shutdown? — which propagates the ctrl_c result — now says SIGINT specifically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
Implements RFC0003.16 — wires the OTLP receiver as a config-toggled
ourios-serverrole (the §9 process-model resolution), and flips the red-gate stub to a green real-socket integration test. This is the last piece of the receiver: it now serves.receiver::servebinds gRPC (tonic) + HTTP (axum) over one sharedIngestPipelinebacked by a singleWal(RFC 0008 §3.1's single-writer rule), reusing the receiver crate'shttp::router+grpc::LogsReceiver. Both listeners run on the binary's tokio runtime alongside the compactor; onewatchchannel fans graceful shutdown to both.Config (env)
OURIOS_RECEIVER_ENABLED,OURIOS_RECEIVER_GRPC_ADDR(default0.0.0.0:4317),OURIOS_RECEIVER_HTTP_ADDR(default0.0.0.0:4318),OURIOS_WAL_ROOT.Graceful shutdown
The server now drains on SIGINT or SIGTERM (k8s /
nerdctl stopsend SIGTERM — a real production need,tokio's already-enabledsignalfeature, no new dep). A failed final telemetry flush is logged, not fatal, so a clean shutdown exits 0 even when the metrics collector is unreachable.Test (RFC0003.16)
A real-socket integration test spawns the binary on
127.0.0.1:0(both transports), reads the reported ports, exports a batch over a realtonicgRPC client + a hand-rolled HTTP POST (no HTTP-client dep),SIGTERMs the process viakill -TERM(nonix, nounsafe— keeps#![deny(unsafe_code)]), waits for a clean exit, then replays the WAL to assert bothOtlpBatchframes are durable (no acked batch lost). No dedup asserted (at-least-once). Stable across repeated runs.Also adds the 405-on-non-POST confirming test (axum's
MethodRouterdefault) per the OTLP-conformance review.Verification
cargo test --all-features✓ — RFC0003.16 green (×3 stable); 405 test green; workspace green. ·cargo clippy --all-targets --all-features -- -D warnings✓ ·cargo fmt --all --check✓Next
Once merged: advance RFC 0003
specified → greenagain (RFC0003.16 now passing) via a doc PR, and update the memory note. The receiver is then a complete, runnable server role — the §9 question fully resolved + implemented.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores