feat(log-ingestor): Add HTTP server skeleton and config setup.#1675
feat(log-ingestor): Add HTTP server skeleton and config setup.#1675hoophalab wants to merge 3 commits into
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughReplaced the trivial main with an asynchronous Axum HTTP server; added CLI parsing, YAML/environment config and credentials loading, hourly-rotating file logging, graceful shutdown handling, and a /health endpoint. Added dependencies: axum, clap, tracing, tracing-appender, tracing-subscriber, and tokio signal feature. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Main as main()
participant Config as Config Loader
participant Logger as Logger Setup
participant Server as Axum Server
participant Shutdown as Shutdown Signal
CLI->>Main: parse args
Main->>Config: load config & credentials (YAML/env)
Config-->>Main: config ready
Main->>Logger: initialize hourly-rotating file logger
Logger-->>Main: logger ready
Main->>Server: bind address & build routes (/ , /health)
Main->>Main: log startup
Main->>Server: serve with graceful shutdown handler
par Serving
Server->>Server: handle requests
and Waiting
Shutdown->>Main: wait for SIGTERM or Ctrl‑C
end
Shutdown-->>Main: signal received
Main->>Server: initiate graceful shutdown
Server-->>Main: shutdown complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
components/log-ingestor/Cargo.toml(1 hunks)components/log-ingestor/src/main.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
components/log-ingestor/src/main.rs
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
components/log-ingestor/Cargo.toml
🧬 Code graph analysis (1)
components/log-ingestor/src/main.rs (2)
components/clp-rust-utils/src/serde/yaml.rs (1)
from_path(18-21)components/clp-py-utils/clp_py_utils/clp_config.py (1)
Database(164-251)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: rust-checks
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (4)
components/log-ingestor/Cargo.toml (2)
6-21: Dependency additions look appropriate for the HTTP server skeleton.The added dependencies align well with the implemented functionality: axum for HTTP serving, clap for CLI parsing, tokio signal handling for graceful shutdown, and the tracing ecosystem for structured logging.
4-4: Ensure the project's MSRV and CI workflows support Rust 1.85 or later.The Rust 2024 edition requires Rust 1.85 (released February 20, 2025) or later. Verify that:
- The project's Minimum Supported Rust Version (MSRV) is set to 1.85+ (check
rust-toolchain.tomlorCargo.toml's[package] rust-version)- CI workflows that validate the build are configured to use Rust 1.85+
components/log-ingestor/src/main.rs (2)
58-67: Unix-specific signal handling limits portability.
tokio::signal::unix::signalis not available on Windows. If cross-platform support is needed, consider conditional compilation or using onlyctrl_c()which is cross-platform.Is Windows support required for the log-ingestor component? If so, the signal handling needs platform-specific conditionals.
69-90: Main function structure looks good.The async main flow is well-organized: argument parsing → config loading → logging setup → server binding → serving with graceful shutdown. The unused
_configand_credentialsare appropriately prefixed for this skeleton implementation.
LinZhihao-723
left a comment
There was a problem hiding this comment.
A few changes. We should use log-ingestor to match our usual standard.
Btw, do u think we should move read_config_and_credentials, set_up_logging, and shutdown_signal to clp-rust-utils? We will probably need these in every new Rust component. If you agree, let's ask rabbit to create an issue to track it.
| RollingFileAppender::new(Rotation::HOURLY, logs_directory, "log_ingestor.log"); | ||
| let (non_blocking_writer, guard) = tracing_appender::non_blocking(file_appender); | ||
| tracing_subscriber::fmt() | ||
| .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) |
There was a problem hiding this comment.
| .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) | |
| .event_format( | |
| tracing_subscriber::fmt::format() | |
| .with_level(true) | |
| .with_target(false) | |
| .with_file(true) | |
| .with_line_number(true) | |
| .json(), | |
| ) | |
| .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) |
To be consistent with the latest API server setup.
| } | ||
|
|
||
| #[derive(Parser)] | ||
| #[command(version)] |
There was a problem hiding this comment.
| #[command(version)] | |
| #[command(version, about = "log-ingestor for CLP.")] |
| } | ||
|
|
||
| async fn health() -> &'static str { | ||
| "Log ingestor is running" |
There was a problem hiding this comment.
| "Log ingestor is running" | |
| "log-ingestor is running" |
|
|
||
| #[derive(Parser)] | ||
| #[command(version)] | ||
| struct Args { |
There was a problem hiding this comment.
Missed this in the previous batch: We should move before functions.
Can you create an issue on this? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @hoophalab, I've created issue #1714 to track moving the common server utilities ( 🧠 Learnings used |
Description
This PR adds the HTTP server skeleton and boilerplate for loading configs in log ingestor
Checklist
breaking change.
Validation performed
GET http://localhost:8080/returnsLog ingestor is runningSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.