logger: ERROR=254 vs CRITICAL=40 inverts standard severity ordering — intentional or accident? #143

New issue

Open

opened 2026-05-21 15:16:19 +00:00 by sameh-farouk · 1 comment

sameh-farouk commented

2026-05-21 15:16:19 +00:00

Member

Question

herolib_core/src/logger/level.rs defines:

pub const LOG_LEVEL_DEBUG: u8    =   0;
pub const LOG_LEVEL_INFO: u8     =  10;
pub const LOG_LEVEL_NOTICE: u8   =  20;
pub const LOG_LEVEL_WARNING: u8  =  30;
pub const LOG_LEVEL_CRITICAL: u8 =  40;
pub const LOG_LEVEL_ERROR: u8    = 254;
pub const LOG_LEVEL_RESERVED: u8 = 255;

The bottom five values follow a clean Python-style "gap of 10" progression (modeled on logging.DEBUG=10 / INFO=20 / WARNING=30 / ERROR=40 / CRITICAL=50, shifted down by 10 with NOTICE inserted at 20 like syslog). That part is recognizable and intentional.

But LOG_LEVEL_ERROR = 254 jumps far above LOG_LEVEL_CRITICAL = 40, which means hero_log is asserting ERROR is more severe than CRITICAL. That inverts every mainstream logging convention I checked:

Source	Ordering
Python `logging`	`CRITICAL=50 > ERROR=40`
syslog (RFC 5424)	`critical=2 < error=3` (lower = more severe)
Apache log4j	`FATAL > ERROR > WARN > INFO > DEBUG`
Java `java.util.logging`	`SEVERE` is top, no separate ERROR/CRITICAL distinction

In all of them, "critical" (or "fatal") sits at or above "error" in severity.

What I think is going on (three possibilities)

Intentional redefinition: hero_log is saying "ERROR means system-down / catastrophic / page-someone-now", and "CRITICAL" is a step below — something that needs attention but isn't an outage. That's a defensible position; it's just idiosyncratic enough that callers will be surprised. If so, the README/docstring should call it out explicitly.
Intentional sentinel placement: ERROR is being pushed to the byte ceiling (254, with 255 reserved) so any reasonable loglevel_max filter includes it. That makes ERROR effectively the catch-all "always-show" tier. Reasonable wire-format thinking, but the side-effect is the inverted severity ordering.
Accidental ordering swap: someone wrote DEBUG/INFO/NOTICE/WARNING/CRITICAL in Python order, jammed ERROR at 254 as a wire-format reserve, and didn't notice the severity ordering had inverted.

I can't tell which of these it is without asking.

Why I'm raising this

In hero_proc#33d4851 I rewrote the local↔hero loglevel mapping to be 1:1 monotonic over 0..=5, mapping local 5 to LOG_LEVEL_ERROR (preserving the existing ordering — local 5 ends up most severe). That fixed the lossy round-trip that was breaking functional::runs::structured_logs_all_levels in hero_proc_test. But if the ordering of ERROR vs CRITICAL gets flipped later, that mapping needs to flip with it.

So before more consumers grow code that depends on the current ordering, I wanted to surface the question.

Concrete asks

Is LOG_LEVEL_ERROR = 254 placed above LOG_LEVEL_CRITICAL = 40 deliberately? If yes:
- Doc it in level.rs and the logger README so callers don't write loglevel_max=CRITICAL thinking they'll see errors too.
- Consider renaming the constants to make the ordering self-documenting, e.g. LOG_LEVEL_FATAL for the 254 slot if "ERROR" is being used in a non-standard sense.
If no, the natural correction is LOG_LEVEL_ERROR = 50 (Python-aligned) or somewhere ≥ 40 but below RESERVED.

Either way I can adjust the hero_proc mapping to match — I just want the convention to be deliberate, not accidental.

cc whoever owns herolib_core/logger.

## Question `herolib_core/src/logger/level.rs` defines: ```rust pub const LOG_LEVEL_DEBUG: u8 = 0; pub const LOG_LEVEL_INFO: u8 = 10; pub const LOG_LEVEL_NOTICE: u8 = 20; pub const LOG_LEVEL_WARNING: u8 = 30; pub const LOG_LEVEL_CRITICAL: u8 = 40; pub const LOG_LEVEL_ERROR: u8 = 254; pub const LOG_LEVEL_RESERVED: u8 = 255; ``` The bottom five values follow a clean Python-style "gap of 10" progression (modeled on `logging.DEBUG=10 / INFO=20 / WARNING=30 / ERROR=40 / CRITICAL=50`, shifted down by 10 with NOTICE inserted at 20 like syslog). That part is recognizable and intentional. **But `LOG_LEVEL_ERROR = 254` jumps far above `LOG_LEVEL_CRITICAL = 40`**, which means hero_log is asserting **ERROR is more severe than CRITICAL**. That inverts every mainstream logging convention I checked: | Source | Ordering | |---|---| | Python `logging` | `CRITICAL=50 > ERROR=40` | | syslog (RFC 5424) | `critical=2 < error=3` (lower = more severe) | | Apache log4j | `FATAL > ERROR > WARN > INFO > DEBUG` | | Java `java.util.logging` | `SEVERE` is top, no separate ERROR/CRITICAL distinction | In all of them, "critical" (or "fatal") sits at or above "error" in severity. ## What I think is going on (three possibilities) 1. **Intentional redefinition**: hero_log is saying "ERROR means system-down / catastrophic / page-someone-now", and "CRITICAL" is a step below — something that needs attention but isn't an outage. That's a defensible position; it's just idiosyncratic enough that callers will be surprised. If so, the README/docstring should call it out explicitly. 2. **Intentional sentinel placement**: ERROR is being pushed to the byte ceiling (254, with 255 reserved) so any reasonable `loglevel_max` filter includes it. That makes ERROR effectively the catch-all "always-show" tier. Reasonable wire-format thinking, but the side-effect is the inverted severity ordering. 3. **Accidental ordering swap**: someone wrote DEBUG/INFO/NOTICE/WARNING/CRITICAL in Python order, jammed ERROR at 254 as a wire-format reserve, and didn't notice the severity ordering had inverted. I can't tell which of these it is without asking. ## Why I'm raising this In `hero_proc#33d4851` I rewrote the local↔hero loglevel mapping to be 1:1 monotonic over `0..=5`, mapping local 5 to `LOG_LEVEL_ERROR` (preserving the existing ordering — local 5 ends up most severe). That fixed the lossy round-trip that was breaking `functional::runs::structured_logs_all_levels` in `hero_proc_test`. But if the ordering of ERROR vs CRITICAL gets flipped later, that mapping needs to flip with it. So before more consumers grow code that depends on the current ordering, I wanted to surface the question. ## Concrete asks - Is `LOG_LEVEL_ERROR = 254` placed above `LOG_LEVEL_CRITICAL = 40` deliberately? If yes: - **Doc it** in `level.rs` and the logger README so callers don't write `loglevel_max=CRITICAL` thinking they'll see errors too. - Consider renaming the constants to make the ordering self-documenting, e.g. `LOG_LEVEL_FATAL` for the 254 slot if "ERROR" is being used in a non-standard sense. - If no, the natural correction is `LOG_LEVEL_ERROR = 50` (Python-aligned) or somewhere ≥ 40 but below RESERVED. Either way I can adjust the hero_proc mapping to match — I just want the convention to be deliberate, not accidental. cc whoever owns `herolib_core/logger`.

sameh-farouk commented

2026-05-21 23:01:43 +00:00

Author

Member

Audit: blast radius of correcting ERROR=254 → 40, CRITICAL=40 → 50

I grepped every hero_* repo for usages of LOG_LEVEL_ERROR / LOG_LEVEL_CRITICAL and raw values 254 / 40 in log contexts. Conclusion: the correction is surgical — almost no consumer needs source edits.

Consumer audit

12 repos check — hero_collab, hero_db, hero_indexer, hero_lib (internal), hero_os, hero_osis, hero_proc, hero_proxy, hero_router, hero_slides, hero_voice, hero_whiteboard. Every one uses the named constant form, e.g.:

tracing::Level::ERROR => LOG_LEVEL_ERROR,
tracing::Level::WARN  => LOG_LEVEL_WARNING,

Zero raw 254 usage anywhere outside the constant definition itself. Zero raw 40 in log contexts (one hit in hero_lib/crates/crypt/symmetric/utils.rs:127 is len >= 40 — unrelated buffer-size check). So all 12 repos recompile transparently against the new constant values.

What actually needs to change

File	Change
`herolib_core/src/logger/level.rs`	`LOG_LEVEL_ERROR: 254 → 40`, `LOG_LEVEL_CRITICAL: 40 → 50`. Update the `from_u8` match arms. Enum discriminants follow the constants. ~6 lines.
`herolib_core/src/logger/README.md`	Update the value table (lines 88-89).
`hero_proc/crates/hero_proc_server/src/logging/adapter.rs`	Swap the last two arms of `local_level_to_hero` (`local 4 → ERROR`, `local 5 → CRITICAL`) and the matching arms in `hero_level_to_local`. 6-line swap; my recent fix (`33d4851`) used named constants, so it'll re-build but I need to fix the LOCAL↔HERO ordering so monotonicity follows the corrected hero values.

Pre-prod migration: not needed

Per Sameh: customer base is still developers-only, so existing log files with byte 254 don't need migration. Anyone bothered by old TSV entries can just rm -rf ~/hero/var/logs/ after the bump.

Suggested PR sequence

hero_lib PR: level.rs + README.md. Single file pair, trivial. Tag a minor bump after.
hero_proc PR: bump the hero_lib pin in Cargo.toml + the 6-line adapter swap. Re-runs hero_proc_test --basic --functional to confirm structured_logs_all_levels still passes (it relies on the round-trip my mapping provides — if I get the swap right, it stays green).

If you (or whoever owns the logger) take the hero_lib side, I'll handle the hero_proc follow-up.

Closes the ordering question regardless of outcome — either you confirm intentional inversion and I document it, or you take the fix.

**Audit: blast radius of correcting `ERROR=254 → 40, CRITICAL=40 → 50`** I grepped every `hero_*` repo for usages of `LOG_LEVEL_ERROR` / `LOG_LEVEL_CRITICAL` and raw values `254` / `40` in log contexts. Conclusion: **the correction is surgical** — almost no consumer needs source edits. ### Consumer audit 12 repos check — `hero_collab`, `hero_db`, `hero_indexer`, `hero_lib` (internal), `hero_os`, `hero_osis`, `hero_proc`, `hero_proxy`, `hero_router`, `hero_slides`, `hero_voice`, `hero_whiteboard`. Every one uses the named constant form, e.g.: ```rust tracing::Level::ERROR => LOG_LEVEL_ERROR, tracing::Level::WARN => LOG_LEVEL_WARNING, ``` **Zero raw `254` usage anywhere** outside the constant definition itself. **Zero raw `40` in log contexts** (one hit in `hero_lib/crates/crypt/symmetric/utils.rs:127` is `len >= 40` — unrelated buffer-size check). So all 12 repos recompile transparently against the new constant values. ### What actually needs to change | File | Change | |---|---| | `herolib_core/src/logger/level.rs` | `LOG_LEVEL_ERROR: 254 → 40`, `LOG_LEVEL_CRITICAL: 40 → 50`. Update the `from_u8` match arms. Enum discriminants follow the constants. ~6 lines. | | `herolib_core/src/logger/README.md` | Update the value table (lines 88-89). | | `hero_proc/crates/hero_proc_server/src/logging/adapter.rs` | Swap the last two arms of `local_level_to_hero` (`local 4 → ERROR`, `local 5 → CRITICAL`) and the matching arms in `hero_level_to_local`. 6-line swap; my recent fix (`33d4851`) used named constants, so it'll re-build but I need to fix the LOCAL↔HERO ordering so monotonicity follows the corrected hero values. | ### Pre-prod migration: not needed Per Sameh: customer base is still developers-only, so existing log files with byte `254` don't need migration. Anyone bothered by old TSV entries can just `rm -rf ~/hero/var/logs/` after the bump. ### Suggested PR sequence 1. **hero_lib PR**: `level.rs` + `README.md`. Single file pair, trivial. Tag a minor bump after. 2. **hero_proc PR**: bump the hero_lib pin in `Cargo.toml` + the 6-line adapter swap. Re-runs `hero_proc_test --basic --functional` to confirm `structured_logs_all_levels` still passes (it relies on the round-trip my mapping provides — if I get the swap right, it stays green). If you (or whoever owns the logger) take the hero_lib side, I'll handle the hero_proc follow-up. Closes the ordering question regardless of outcome — either you confirm intentional inversion and I document it, or you take the fix.