Skip to content

feat: add structured JSON logging and file output support#16

Merged
mickvandijke merged 1 commit intoWithAutonomi:mainfrom
jacderida:feat-support_logging_for_auto_upgrade_tests
Mar 3, 2026
Merged

feat: add structured JSON logging and file output support#16
mickvandijke merged 1 commit intoWithAutonomi:mainfrom
jacderida:feat-support_logging_for_auto_upgrade_tests

Conversation

@jacderida
Copy link
Copy Markdown
Collaborator

@jacderida jacderida commented Mar 2, 2026

Summary

  • Add --log-format (text/json), --log-dir, and --log-max-files CLI flags for structured JSON log output and daily-rotated file logging via tracing-appender
  • Embed git commit hash at build time (SAORSA_GIT_COMMIT env var in build.rs) for log traceability
  • Convert key info! statements in node.rs and upgrade/monitor.rs to use structured tracing fields (e.g. version, commit, current_version, new_version, listen_addrs) for Elasticsearch queryability without regex extraction

Test plan

  • cargo build --release succeeds
  • cargo test — all 151 unit + 43 e2e tests pass
  • cargo clippy --all-features -- -D clippy::panic -D clippy::unwrap_used -D clippy::expect_used passes clean
  • Manual: --log-format json outputs valid JSON to stdout
  • Manual: --log-dir /tmp/saorsa-logs writes daily-rotated files
  • Manual: --log-max-files 3 retains only 3 rotated files

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 2, 2026 23:27
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds structured, query-friendly tracing output for saorsa-node, including JSON formatting and optional daily-rotated file logging, and embeds build-time git commit metadata for traceability.

Changes:

  • Introduces --log-format, --log-dir, and --log-max-files CLI flags and wires them into tracing_subscriber + tracing-appender setup.
  • Embeds a SAORSA_GIT_COMMIT value at build time and logs it alongside the crate version.
  • Converts key info! logs in node startup and upgrade monitoring to structured tracing fields.

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/upgrade/monitor.rs Switches upgrade-monitor logs to structured fields for better querying.
src/node.rs Adds version/commit fields and structured fields for startup/upgrade logs.
src/bin/saorsa-node/main.rs Implements log format + file output configuration via tracing layers and appender guards.
src/bin/saorsa-node/cli.rs Adds CLI flags / env vars for log format and file output configuration.
build.rs Computes and injects SAORSA_GIT_COMMIT into the build.
Cargo.toml Adds tracing-appender dependency to support rolling file logs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 2, 2026

Greptile Summary

This PR adds structured JSON logging, daily-rotated file output, and build-time git commit embedding to saorsa-node. The logging infrastructure is well-structured and the conversion from format-string info! calls to typed tracing fields is clean and consistent across node.rs and upgrade/monitor.rs.

Key changes:

  • New CLI flags --log-format (text/json), --log-dir, and --log-max-files with matching env-var overrides and sensible defaults.
  • A four-arm match in main.rs builds the correct tracing_subscriber layer for each format×destination combination using tracing_appender::non_blocking; the WorkerGuard is correctly retained for the duration of main().
  • build.rs embeds SAORSA_GIT_COMMIT via git rev-parse --short HEAD, falling back gracefully to "unknown" when git is unavailable.
  • build.rs only watches .git/HEAD, not the actual branch ref (e.g. .git/refs/heads/main). Because .git/HEAD typically only stores ref: refs/heads/main (a static string), Cargo won't re-run the build script after new commits on the same branch, causing the embedded hash to go stale during incremental development builds.
  • Both main.rs (before node.run()) and node.rs (run() itself) log version and commit at startup, producing a duplicate log entry on every launch.

Confidence Score: 4/5

  • Safe to merge with minor fixes; one logic issue in build.rs may cause stale commit hashes during development builds.
  • The logging infrastructure is correct and the guard lifetime is handled properly. The only substantive issue is in build.rs where rerun-if-changed=.git/HEAD is insufficient to detect new commits on the current branch, leaving the embedded hash stale in incremental builds. The duplicate startup log is a minor annoyance. Neither issue affects runtime correctness or security.
  • build.rs — missing rerun-if-changed directives for .git/refs/heads and .git/packed-refs.

Important Files Changed

Filename Overview
build.rs Adds git commit hash embedding via git rev-parse --short HEAD at build time; correctly falls back to "unknown" when git is unavailable, but only watches .git/HEAD — not the branch ref file — so the hash can become stale on incremental builds after new commits on the same branch.
src/bin/saorsa-node/main.rs Adds a four-arm match to configure a boxed tracing_subscriber layer supporting text/JSON × stdout/file combinations; uses tracing_appender::non_blocking with a WorkerGuard correctly held for the duration of main(); the #[allow(clippy::collection_is_never_read)] suppression is intentional and well-commented.
src/bin/saorsa-node/cli.rs Adds --log-format, --log-dir, and --log-max-files CLI flags with sensible defaults (text, no dir, 7 files) and corresponding env-var overrides; CliLogFormat enum is cleanly derived with ValueEnum and Default.
src/node.rs Converts info! format-string calls to structured tracing fields; introduces a duplicate version/commit log at the start of run() that is already emitted by main.rs before calling this function.
src/upgrade/monitor.rs Straightforward conversion of info! format-string messages to structured fields (new_version, delay_hours, delay_minutes, current_version, version); no functional changes.
Cargo.toml Adds tracing-appender = "0.2" dependency; all other entries unchanged.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CLI parse] --> B{log_format × log_dir}
    B -->|Text, None| C[fmt::layer stdout]
    B -->|Json, None| D[fmt::layer JSON stdout]
    B -->|Text, Some dir| E[rolling::Builder daily]
    B -->|Json, Some dir| F[rolling::Builder daily]
    E --> G[non_blocking writer\n+ WorkerGuard held]
    F --> H[non_blocking writer\n+ WorkerGuard held]
    G --> I[fmt::layer file text]
    H --> J[fmt::layer file JSON]
    C & D & I & J --> K[tracing_subscriber::registry\n.with layer\n.with EnvFilter\n.init]
    K --> L[node.run]
    L --> M[Goodbye! log\nGuard drops → flush]
Loading

Last reviewed commit: b584f97

Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

fn main() {
// Rerun if feature configuration changes
// Rerun if the git HEAD changes (new commit, branch switch, etc.)
println!("cargo:rerun-if-changed=.git/HEAD");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale commit hash on incremental builds

cargo:rerun-if-changed=.git/HEAD only tracks the HEAD pointer file (which typically contains ref: refs/heads/main — a static string that does not change when new commits are made on the current branch). Cargo will therefore skip re-running the build script after a git commit on the same branch, meaning the embedded SAORSA_GIT_COMMIT value becomes stale until something else triggers a full rebuild.

To keep the embedded hash up-to-date you need to also watch the ref that HEAD points to, as well as packed-refs (which is updated when refs are packed):

// Rerun if the git HEAD pointer changes (branch switch, detached HEAD, etc.)
println!("cargo:rerun-if-changed=.git/HEAD");
// Rerun if the current branch tip moves (new commits)
println!("cargo:rerun-if-changed=.git/refs/heads");
// Rerun if refs are packed (e.g. after `git gc`)
println!("cargo:rerun-if-changed=.git/packed-refs");
println!("cargo:rerun-if-changed=build.rs");

Alternatively, consider the vergen crate which handles all of these edge cases automatically.

Prompt To Fix With AI
This is a comment left during a code review.
Path: build.rs
Line: 7

Comment:
**Stale commit hash on incremental builds**

`cargo:rerun-if-changed=.git/HEAD` only tracks the HEAD *pointer* file (which typically contains `ref: refs/heads/main` — a static string that does not change when new commits are made on the current branch). Cargo will therefore skip re-running the build script after a `git commit` on the same branch, meaning the embedded `SAORSA_GIT_COMMIT` value becomes stale until something else triggers a full rebuild.

To keep the embedded hash up-to-date you need to also watch the ref that HEAD points to, as well as `packed-refs` (which is updated when refs are packed):

```rust
// Rerun if the git HEAD pointer changes (branch switch, detached HEAD, etc.)
println!("cargo:rerun-if-changed=.git/HEAD");
// Rerun if the current branch tip moves (new commits)
println!("cargo:rerun-if-changed=.git/refs/heads");
// Rerun if refs are packed (e.g. after `git gc`)
println!("cargo:rerun-if-changed=.git/packed-refs");
println!("cargo:rerun-if-changed=build.rs");
```

Alternatively, consider the [`vergen`](https://crates.io/crates/vergen) crate which handles all of these edge cases automatically.

How can I resolve this? If you propose a fix, please make it concise.

src/node.rs Outdated
Comment on lines +428 to +432
info!(
version = env!("CARGO_PKG_VERSION"),
commit = env!("SAORSA_GIT_COMMIT"),
"Starting saorsa-node"
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Duplicate version/commit log at startup

main.rs already logs version and commit with the message "saorsa-node starting" (line 78–82) immediately before calling node.run(). This info! in run() will produce a second, nearly identical log line at every startup, which is redundant and may confuse operators querying logs in Elasticsearch.

Consider either removing the duplicate here and keeping only the one in main.rs, or removing the version/commit fields from this call and using a distinct message (e.g., "Node runtime loop started") that adds context rather than repeating what was already logged.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/node.rs
Line: 428-432

Comment:
**Duplicate version/commit log at startup**

`main.rs` already logs `version` and `commit` with the message `"saorsa-node starting"` (line 78–82) immediately before calling `node.run()`. This `info!` in `run()` will produce a second, nearly identical log line at every startup, which is redundant and may confuse operators querying logs in Elasticsearch.

Consider either removing the duplicate here and keeping only the one in `main.rs`, or removing the `version`/`commit` fields from this call and using a distinct message (e.g., `"Node runtime loop started"`) that adds context rather than repeating what was already logged.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Collaborator

@grumbach grumbach left a comment

Choose a reason for hiding this comment

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

LGTM

Add CLI flags (--log-format, --log-dir, --log-max-files) for JSON log
output and daily-rotated file logging via tracing-appender. Embed git
commit hash at build time. Convert key info! statements in node.rs and
upgrade/monitor.rs to use structured tracing fields for Elasticsearch
queryability.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jacderida jacderida force-pushed the feat-support_logging_for_auto_upgrade_tests branch from b584f97 to f0387cb Compare March 3, 2026 16:25
@mickvandijke mickvandijke merged commit d77d544 into WithAutonomi:main Mar 3, 2026
1 check failed
mickvandijke added a commit that referenced this pull request Apr 1, 2026
Add unit and e2e tests covering the remaining Section 18 scenarios:

Unit tests (32 new):
- Quorum: #4 fail→abandoned, #16 timeout→inconclusive, #27 single-round
  dual-evidence, #28 dynamic threshold undersized, #33 batched per-key,
  #34 partial response unresolved, #42 quorum-derived paid-list auth
- Admission: #5 unauthorized peer, #7 out-of-range rejected
- Config: #18 invalid config rejected, #26 dynamic paid threshold
- Scheduling: #8 dedup safety, #8 replica/paid collapse
- Neighbor sync: #35 round-robin cooldown skip, #36 cycle completion,
  #38 snapshot stability mid-join, #39 unreachable removal + slot fill,
  #40 cooldown peer removed, #41 cycle termination guarantee,
  consecutive rounds, cycle preserves sync times
- Pruning: #50 hysteresis prevents premature delete, #51 timestamp reset
  on heal, #52 paid/record timestamps independent, #23 entry removal
- Audit: #19/#53 partial failure mixed responsibility, #54 all pass,
  #55 empty failure discard, #56 repair opportunity filter,
  response count validation, digest uses full record bytes
- Types: #13 bootstrap drain, repair opportunity edge cases,
  terminal state variants
- Bootstrap claims: #46 first-seen recorded, #49 cleared on normal

E2e tests (4 new):
- #2 fresh offer with empty PoP rejected
- #5/#37 neighbor sync request returns response
- #11 audit challenge multi-key (present + absent)
- Fetch not-found for non-existent key

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

4 participants