mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-09 14:49:25 +00:00
docs(agents): make the structured-logging rule reachable and enforceable (#5828)
The RustFS event shape (`event`/`component`/`subsystem`/`result` + context, message last) is specified only in `.agents/skills/rustfs-logging-governance/SKILL.md`, and nothing routes a change to it: - `AGENTS.md`, which is what an agent actually loads by default, never mentions logging. Its only related line is "log unknown fields at `warn`" under Serde Safety, which is about level, not shape. - The skill's `description` says "use when editing or reviewing RustFS logs", so a bugfix that adds one log line in passing — how most new log sites enter this repo — never matches it. - `scripts/check_logging_guardrails.sh` is a blocklist: 500+ `rg -F` literals that retire log lines which already shipped. It cannot see a newly written one. For `crates/ecstore/src/disk/local.rs` the only check is that `#[tracing::instrument]` is TRACE-only; `warn!`/`info!` shape is unchecked. PR #5822 landed `warn!("heal rename_data: purging ... {:?} failed: {}", ...)` in `disk/local.rs` — sentence-style, no fields, directly beside `info!(event = EVENT_DISK_LOCAL_RENAME_REJECTED, component = ..., subsystem = ...)` — with every check green. That is the gap, not an authoring mistake. Close all three: - `AGENTS.md`: a Logging section stating the field shape, the level policy, the reuse-the-file's-constants rule, and that it applies to any `tracing` macro added in passing, not only to log-focused changes. - Skill `description`: trigger on adding or editing any `tracing` macro, naming the single-line-added-in-passing case explicitly. - Guardrail: assert the event shape positively on the already-governed disk files — `error!`/`warn!`/`info!` must open with fields or a `target:`, never a bare string. Commented-out macros are excluded; `debug!`/`trace!` stay out of scope as targeted diagnostics. Self-test fixtures cover both directions. `crates/ecstore/src/disk/mod.rs` carried the one live violation in that file set (`conv_part_err_to_int`), so it is converted here; the guardrail would otherwise fail on an untouched file. Verification: - `./scripts/check_logging_guardrails.sh` — passes - Negative control: re-inserting PR #5822's exact `warn!` line into `disk/local.rs` makes it exit 1 pointing at that line - `cargo fmt -p rustfs-ecstore -- --check`, `cargo check -p rustfs-ecstore`
This commit is contained in:
@@ -842,6 +842,60 @@ for file in "${disk_logging_files[@]}"; do
|
||||
fi
|
||||
done
|
||||
|
||||
# `forbidden_patterns` above only retires log lines that already shipped, so a
|
||||
# newly written sentence-style log passes every check in this script — which is
|
||||
# how one reaches review in the first place (PR #5822 added
|
||||
# `warn!("heal rename_data: purging ... {:?} failed: {}", ...)` to
|
||||
# disk/local.rs with CI green). Assert the RustFS event shape positively on the
|
||||
# file sets already governed here: error!/warn!/info! must open with fields
|
||||
# (`event = ...`) or a `target:`, never with a bare message string. debug! and
|
||||
# trace! stay out of scope — they are targeted diagnostics, not operator
|
||||
# events. Extend this list as more files are converted; it is deliberately
|
||||
# narrower than `checked_files`, which is only a blocklist surface.
|
||||
structured_event_files=(
|
||||
"crates/ecstore/src/disk/mod.rs"
|
||||
"crates/ecstore/src/disk/local.rs"
|
||||
"crates/ecstore/src/cluster/rpc/remote_disk.rs"
|
||||
)
|
||||
|
||||
# The leading class rejects `my_info!(` while still matching `tracing::warn!(`.
|
||||
sentence_style_log_pattern='(?:^|[^A-Za-z0-9_])(?:error|warn|info)!\(\s*"'
|
||||
|
||||
for file in "${structured_event_files[@]}"; do
|
||||
# Commented-out macros are dead code, not emitted events.
|
||||
sentence_style_logs="$(rg -n -U "$sentence_style_log_pattern" "$file" | rg -v '^[0-9]+:\s*//' || true)"
|
||||
if [[ -n "$sentence_style_logs" ]]; then
|
||||
echo "❌ logging guardrail violation: error!/warn!/info! must lead with structured fields (event/component/subsystem) in $file" >&2
|
||||
echo "$sentence_style_logs" >&2
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
# Keep the matcher honest in both directions.
|
||||
for fixture in \
|
||||
'warn!("heal rename_data: purging stale destination data dir {:?} failed: {}", dst_data_path, err);' \
|
||||
$'warn!(\n "rename_data commit failed: {}",\n err\n);' \
|
||||
'tracing::warn!("conv_part_err_to_int: unknown error: {err:?}");' \
|
||||
'info!("disk scan finished");'; do
|
||||
if ! printf '%s\n' "$fixture" | rg -U "$sentence_style_log_pattern" >/dev/null; then
|
||||
echo "❌ logging guardrail self-test failed: sentence-style log was accepted" >&2
|
||||
echo "$fixture" >&2
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
for fixture in \
|
||||
$'info!(\n event = EVENT_DISK_LOCAL_RENAME_REJECTED,\n component = LOG_COMPONENT_ECSTORE,\n reason = "rename_all_data_path_failed",\n "Disk local rename flow failed"\n);' \
|
||||
'warn!(target: "rustfs::heal::manager", event = EVENT_HEAL_RETRY, "Heal retry admission decided");' \
|
||||
'my_info!("not a tracing macro");' \
|
||||
'debug!("list_dir raw {:?}", entries);'; do
|
||||
if printf '%s\n' "$fixture" | rg -U "$sentence_style_log_pattern" >/dev/null; then
|
||||
echo "❌ logging guardrail self-test failed: structured event was rejected" >&2
|
||||
echo "$fixture" >&2
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
if rg -n -U 'debug!\([\s\S]{0,600}"Remote disk RPC started"' crates/ecstore/src/cluster/rpc/remote_disk.rs >/dev/null; then
|
||||
echo "❌ logging guardrail violation: successful remote disk RPC events must not be emitted at DEBUG" >&2
|
||||
exit 1
|
||||
|
||||
Reference in New Issue
Block a user