Skip to content

Conversation

@hoophalab
Copy link
Contributor

@hoophalab hoophalab commented Nov 26, 2025

Description

This PR adds the HTTP server skeleton and boilerplate for loading configs in log ingestor

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. run log-ingestor with
CLP_LOGS_DIR=. CLP_DB_USER=clp-user CLP_DB_PASS=pass ./build/rust/release/log-ingestor --config clp-config.yaml --host localhost --port 8080
  1. GET http://localhost:8080/ returns Log ingestor is running

Summary by CodeRabbit

  • New Features

    • Log ingestor server now operational with a health-check endpoint.
    • Configuration and credentials loading capability added.
    • Graceful shutdown support for clean server termination.
    • Logging infrastructure with hourly file rotation established.
  • Chores

    • Added dependencies to support the async server runtime and enhanced logging.

✏️ Tip: You can customize this high-level summary in your review settings.

@hoophalab hoophalab requested a review from a team as a code owner November 26, 2025 18:18
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 26, 2025

Walkthrough

Replaced 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

Cohort / File(s) Summary
Log ingestor dependencies
components/log-ingestor/Cargo.toml
Added dependencies: axum (with json), clap (with derive), tracing, tracing-appender, tracing-subscriber (features: json, env-filter, fmt, std), and extended tokio features to include signal.
Log ingestor implementation
components/log-ingestor/src/main.rs
Replaced a trivial Hello World with an async Axum server: CLI arg parsing (clap), config and credentials loading from YAML/env, hourly-rotating file logging setup, TCP bind and route definitions (/, /health), and graceful shutdown handling (SIGTERM/Ctrl-C).

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Check YAML parsing and environment fallback in config/credentials loader.
  • Verify hourly rotation configuration and file permissions for logging.
  • Validate graceful shutdown logic and signal handling ordering.
  • Confirm Axum route handlers and runtime binding error handling.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: adding an HTTP server skeleton and config setup functionality to the log-ingestor component.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca25a25 and 428ed9b.

📒 Files selected for processing (1)
  • components/log-ingestor/src/main.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 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). (4)
  • GitHub Check: package-image
  • GitHub Check: rust-checks
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
components/log-ingestor/src/main.rs (2)

58-67: Verify platform requirements: Unix-specific signal handling.

The shutdown signal handler uses tokio::signal::unix::SignalKind::terminate(), which will not compile on Windows. If cross-platform support is required, consider using conditional compilation or an alternative approach.

Option 1: Conditional compilation for Unix-only signals

 async fn shutdown_signal() {
+    #[cfg(unix)]
     let mut sigterm = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
         .expect("failed to listen for SIGTERM");
     tokio::select! {
+        #[cfg(unix)]
         _ = sigterm.recv() => {
         }
         _ = tokio::signal::ctrl_c()=> {
         }
     }
 }

Option 2: If Windows support is not required, document this in the code or verify that the service only targets Unix/Linux platforms.


92-94: LGTM: Past review feedback addressed.

The function now returns &'static str instead of allocating a String, avoiding unnecessary allocation for the constant health check response.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55f2c9c and ca25a25.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is 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.toml or Cargo.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::signal is not available on Windows. If cross-platform support is needed, consider conditional compilation or using only ctrl_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 _config and _credentials are appropriately prefixed for this skeleton implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant