Repository navigation
Conversation
b7c336f to
f15ac9b
Compare
f9f5bdd to
b58db1d
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
There was a problem hiding this comment.
This PR makes the 24-hour log counts safe against a clock that steps back. At boot, before the tracing subscriber exists, the bot records the length of each log file, and the one-time seed reads each file only up to that length, so this process's own lines can never be counted twice. The Prometheus recorder now installs before tracing (the call is idempotent, so the bot session reuses it), which lets log_events_total count boot warnings. The two log-count series move to a new logs family that reads no database and runs first in the refresh cycle.
Overall read: the change is small, well reasoned and well tested. The seed boundary by file length is correct for the daily append-only appender, the cut-line case is handled and tested, the earlier review threads are all fixed at this commit, and nothing here can stop the bot from starting on state an earlier version wrote. One wording nit: the log_events_total HELP text and the docs metrics table still describe the counter as lines the file log wrote.
claude-opus-5-5 · high · 3 h 50 min
|
🔎 Reviewing |
1 similar comment
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR prevents duplicate 24-hour error and warning counts after a clock rollback by seeding only the log bytes present before tracing starts. It also installs the metrics recorder early enough to count boot warnings and moves log counts into their own metrics family.
No new production defect, upgrade incompatibility, or shared stall was found beyond the existing resolved threads and accepted follow-ups. One wording nit remains in the counter description. Reviewed the requested commit 5d302d7; tests were inspected, not run.
gpt-6.1-sol · high · 3 h 24 min · Claude Opus 5.5 failed (You've hit your session limit · resets 10:30am (UTC))
There was a problem hiding this comment.
This PR fixes double counting in the 24-hour log counts after the clock steps back. At boot, before the tracing subscriber exists, the bot records the length of each log file, and the one-time seed reads each file only up to that length, so this process's own lines can never reach the seed. The seed also drops entries dated after activation, and the window does not count minutes after the current one. The Prometheus recorder is now installed before tracing, so log_events_total also counts boot warnings (the session's later metrics::setup() call reuses the same recorder). The two log count series move to their own logs family, which reads no database, is published first in each refresh cycle, and stays unpublished until the seed finishes.
Overall read: the change does what it says. The length snapshot is taken before any byte of this process is written, files that roll after activation are not listed and so are not read, a line cut by the recorded length does not parse and is skipped, and the seed retry states all have a way out. Metric names and the on-disk log format are unchanged, so there is no upgrade or rollback hazard. The tests cover the clock step back, the cut line, future-dated entries, the failed listing, and the logs family refreshing while a database loader fails. The remaining weak points (symlinked log files, a truncate between activation and the seed, the recorder order held only by a comment, HELP and docs table wording, and where log_count_samples lives) are already raised in resolved threads and tracked in RAI-3195. No new findings.
claude-opus-5-5 · high · 53 min
b58db1d to
b1283d1
Compare
f15ac9b to
5fe66d9
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR fixes how the bot rebuilds its 24-hour error and warning log counts at boot. It records each log file's length when it starts and seeds the counts only from those bytes, so a clock step back can no longer make this process's own lines count twice. It installs the metrics recorder before tracing, so boot warnings are counted too. It also moves the two log-count series into their own logs family, which reads no database and is published first, so a slow database read no longer holds them back.
Overall read: the change does what the description says. The byte bound keeps earlier processes' entries apart from this one's, entries dated after activation are skipped, live minutes ahead of the clock count only once the clock reaches them, and the logs family stays unpublished until the seed finishes. SPEC.md and docs/observability.md match the code. All earlier threads are resolved, and the items deferred to RAI-3195 were not raised again. The panel found no blocker. One minor test gap is noted inline: the test that closes the database to show the logs family still refreshes runs without file logging, so it never goes through the seed.
claude-opus-5-5 · high · 2 h 23 min
There was a problem hiding this comment.
This PR changes how the bot seeds its 24-hour log counts at boot. It records the length of each log file before tracing starts, and the seed reads only those bytes. This replaces the split by wall-clock time, which double counted after the clock stepped back. The metrics recorder now installs before tracing, so warnings logged during boot reach log_events_total. The log counts also move to their own logs family, which reads no database and is published first in each refresh.
The change does what the description says. Each claim holds in the code: the seed is bounded by the lengths recorded at activation and by until: activated_at, the recorder install comes before both tracing setups and metrics::setup is idempotent, and logs publishes even when a reliability loader fails. The panel found nothing new beyond the gaps already deferred to RAI-3195. One minor test gap is below.
Design note for RAI-3195: the truncate, symlink and final-listing gaps all exist because the seed runs in the background after this process starts writing to the same files. Reading the previous 24 hours inside activate_log_counts, before the subscriber is installed, would remove that overlap and most of the seed state machine. The trade-off is a short synchronous file scan at boot.
claude-opus-5-5 · high · 28 min
5fe66d9 to
a33c98c
Compare
b1283d1 to
2f3b1c1
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
a33c98c to
f05dd5d
Compare
2f3b1c1 to
7cef748
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
7cef748 to
ca03d20
Compare
f05dd5d to
b7b0546
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
Part of RAI-3145.
The 24-hour log counts no longer double count after the clock steps back. At boot the bot records how long each log file is and seeds the counts only from that part, instead of splitting old and new entries by wall-clock time. The metrics recorder now installs before tracing, so
log_events_totalalso counts the boot's warnings. The log counts get their ownlogsfamily, so a slow database read no longer holds them back.Live effect: on the next bot release,
liq_collector_last_success_ts_seconds{collector="logs"}appears, and the two log-count series move from thereliabilityfamily to it · Risk: low (metrics only; a seed read error still goes through the existing retry and warning path) · Ships: next bot release · Blocks: needs #1721 below itDecisions
LiqFamily::Logsowns the two log-count names, is published first in each refresh, reads no database, and stays unpublished until the seed finishes.Proof