Skip to content

feat(spur-logging): add structured JSON logging - #737

Open
maybeharshit wants to merge 2 commits into
ROCm:mainfrom
maybeharshit:feat/structured-json-logging
Open

feat(spur-logging): add structured JSON logging#737
maybeharshit wants to merge 2 commits into
ROCm:mainfrom
maybeharshit:feat/structured-json-logging

Conversation

@maybeharshit

Copy link
Copy Markdown
Collaborator

Summary

Adds a shared spur-logging crate so every binary (spurctld, spurd, spur-k8s-operator, spur) installs one subscriber with a single schema, replacing four independent tracing_subscriber::fmt() setups. In json mode each line is one flat JSON object; text mode keeps the existing human-readable output.

Every JSON line carries a fixed schema: timestamp (RFC 3339, nanosecond), level (lowercase), component, target (module path), message, plus any structured fields the event already passed (job_id, node, error, ...), each keeping its native JSON type — a numeric job_id stays a number.

  • Format: --log-format > [logging].format > (stderr is a TTY ? text : json).
  • Level: RUST_LOG > --log-level > [logging].level > info.

Approach / design

  • Uses a custom FormatEvent rather than stock fmt().json(), which uppercases level, has no top-level component, and nests span fields under span. A Visit impl records each field with its native JSON type.
  • Implements the "fields already in scope" reading: job_id/node are flattened from what call sites already pass — no span-propagation layer and no call-site rewrites.
  • Config is now loaded before logging init so [logging] can choose the format; nothing logs before init.
  • LoggingConfig gains #[serde(default)] on all fields and its default format changes "text" to unset. It is TOML-config only (not Raft/WAL state, not proto) and field types are unchanged, so this is backward compatible.

Behavior change (intended, documented)

A daemon under a service manager (no TTY) now emits JSON by default where it previously emitted text; interactive terminals still get text. examples/spur.conf ships format = "json", and docs/admin-guide/configuration.rst documents the formats, the TTY default, and precedence.

Testing

  • 7 unit tests in spur-logging (schema/required fields, lowercase level, native-typed job_id, message fallback, quote/newline robustness, level & format precedence).
  • cargo clippy --workspace --exclude spur-ffi --all-targets --locked — no warnings.
  • cargo test — green; the only failures are 10 spurd cgroup/PTY tests that need kernel features absent in the build sandbox and pass on a real host.
  • End-to-end on an LXD container via the native-host harness: new tests/native_host/e2e/test_json_logging.py (valid JSON schema from real daemons; numeric job_id in the controller's job submitted line after a real submit; text mode stays readable), plus the existing test_single_node.py (18 tests) all pass with JSON as the piped default.

Follow-ups (out of scope)

Span-based propagation (fields on events that don't pass them), a file sink (logging.file), and OpenTelemetry are intentionally deferred.

Made with Cursor

@maybeharshit
maybeharshit force-pushed the feat/structured-json-logging branch from df710e8 to 316e668 Compare September 1, 2026 07:04
@codecov-commenter

codecov-commenter commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 54.72441% with 115 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #737      +/-   ##
==========================================
- Coverage   80.15%   80.11%   -0.04%     
==========================================
  Files         184      185       +1     
  Lines       87772    87989     +217     
==========================================
+ Hits        70348    70487     +139     
- Misses      17424    17502      +78     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

harssriv and others added 2 commits September 3, 2026 05:16
Every binary now initializes one shared subscriber (the new spur-logging
crate) instead of four independent tracing_subscriber::fmt() setups. In json
mode each line is one flat JSON object with a fixed schema (timestamp, level,
component, target, message, plus native-typed fields such as job_id/node/
error); text mode keeps the existing human-readable output.

Format precedence: --log-format > [logging].format > (TTY ? text : json), so
a service with no TTY gets JSON and an interactive terminal gets text. Level
precedence: RUST_LOG > --log-level > [logging].level > info. Config is loaded
before logging init so [logging] can choose the format.

LoggingConfig gains #[serde(default)] on all fields and its default format
changes from "text" to unset; it is TOML-only (not Raft/WAL or proto) and
field types are unchanged, so this is backward compatible.

Co-authored-by: Cursor <[email protected]>
Structured JSON is now the default when stderr is not a TTY, so the e2e
daemons emit JSON, which broke tests that scrape spurctld/spurd logs for
key=value fields (multi-node suspend fan-out, device GPU count). Pin the
default e2e cluster config to text; JSON stays covered by test_json_logging.

Also reword the [logging] prose so pyspelling does not flag stderr/systemd's
(inline-literal stderr, and drop the possessive so bare systemd matches the
wordlist).

Co-authored-by: Cursor <[email protected]>
@maybeharshit
maybeharshit force-pushed the feat/structured-json-logging branch from 872e5ce to 534a0da Compare September 3, 2026 05:23
@maybeharshit
maybeharshit marked this pull request as ready for review September 3, 2026 06:36
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.

2 participants