Repository navigation
Conversation
JuaniRios
changed the title
|
@coderabbitai full review |
|
@rain-marvin review |
✅ Action performedFull review finished. |
|
🔎 Reviewing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (15)
Included review availability: This review used your included allowance. 7 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. WalkthroughThe generator pins liquidity PromQL selectors to Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was found in the current changes; normal checks and the stated dashboard dependency still apply. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (11 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly summarizes the primary observability changes: pinning liquidity board queries to the exporter job and fixing process-start tracking. |
| Description check | |
The description directly explains the board query pinning, scan-skipped label correction, process-start uptime fix, validation results, and merge conditions. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (11 skipped: 10 unsupported, 1 too large.)
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR pins every liq_ query on the five liquidity Grafana boards to job="t0-liquidity-exporter", so the boards do not count two copies of each series once the bot itself publishes liq_* (#1665). The generator now adds the matcher in promql(), refuses to emit a board with an unpinned liq_ selector, and CI runs it with --check. It also fixes the native "Hedge scans skipped" row to group by reason, the label the bot really emits, and makes /health uptime count from process start instead of from the first /health call.
Overall read: correct and low risk. I ran gen-t0-liquidity.py --check at this head: all five boards match the generator, and the JSON diff only adds the job matcher and the reason fix. The exporter's scrape job_name is t0-liquidity-exporter in both terraform/staging-liquidity/main.tf and terraform/production-liquidity/main.tf in t0.devops, so the pinned queries still return today's data in both environments. PROCESS_START is forced before config load, and /health is only served by the server binary. No blockers. The two minor comments are about the pinning helper accepting broken PromQL, and other liq_* readers outside this repo that will still sum both jobs once #1665 deploys. Also outside the diff: the hedge_scan_skipped_total description in src/metrics.rs still says "by symbol and cause".
claude-opus-5-5 · high · 10 min
b3c8de6 to
3551837
Compare
|
@rain-marvin approve |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR pins every liq_ selector on the liquidity Grafana boards to job="t0-liquidity-exporter" through observability/gen-t0-liquidity.py, adds a --check step to CI that fails on drift or on an unpinned selector, fixes the native-row aggregation to sum by (reason, symbol), and moves the process start time to one shared PROCESS_START used by /health.
The new head is a plain rebase onto master. git range-diff shows the PR's own commit unchanged; only the surrounding context in docs/observability.md and flake.nix moved. The generator check line still sits in the backend CI block ahead of the master ci-audit step, and python3 observability/gen-t0-liquidity.py --check passes at this head (5 boards match). No new defects.
Earlier findings:
src/api.rs:123(nit,GIT_COMMITvisibility and comments that describe the next PR): still in the code, left on purpose. The next PR up the stack, #1665, readsapi::GIT_COMMITand addsliq_bot_start_timestamp_seconds, so the code and comments become accurate when it lands. The reason holds for a nit.
Threads (all resolved after agreement, none by hand by someone else):
observability/gen-t0-liquidity.py:90(minor,pin_liqrewrites inside string literals andby (...)lists): dismissed as latent and tracked as a follow-up. That holds: no current board hits it and the change is not needed for this PR to be correct.docs/observability.md:108(minor, t0.devops readers sumliq_*without a job matcher): addressed outside this repo by T0Trade/t0.devops#787, a merge condition on #1665, which is where double counting would start.observability/t0-liquidity-native-rows.json:295(nit, nestedlegendFormatis ignored): dismissed as a follow-up. The aggregation fix stands alone and the nested field does no harm.src/api.rs:123(nit): as above.
Nothing blocks the merge.
claude-opus-5-5 · high · 43 s
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
… start The bot is about to publish liq_* names that the exporter sidecar already publishes. Every liq_ selector on the liquidity boards now reads job="t0-liquidity-exporter" so the two copies are never summed; the board generator adds the matcher and fails on any unpinned selector, and CI runs it with --check. The scan-skipped native row grouped hedge_scan_skipped_total by cause, but the bot labels it reason, so the breakdown was always empty. /health uptime counted from the first /health request because STARTED_AT was a lazy static. The server now captures PROCESS_START before loading the config.
3551837 to
5d21383
Compare
JuaniRios
changed the title
This branch has not been deployed
2 participants
Part of RAI-3083.
Pins every
liq_query on the liquidity boards to the exporter's scrape job, fixes the scan-skipped native row, and makes/healthuptime count from process start. First PR of the RAI-2998 stack (bot publishes theliq_*metrics itself).Live effect: board queries read only
job="t0-liquidity-exporter"(same data as today), the scan-skipped breakdown starts showing reasons, and uptime is right after a restart · Risk: low (no money path, no config, no stored data; a revert undoes it) · Ships: boards on merge (observability workflow); uptime on the next bot releaseWhy
The bot is about to publish the same
liq_*names the exporter sidecar publishes (#1665). The boardssum()those series with no job filter, so both copies would be counted. The pin has to be in before the bot publishes anything.Decisions
gen-t0-liquidity.pyadds the job matcher inpromql()and fails generation if anyliq_selector is left unpinned, so a new panel can't silently double count. CI runs it with--check.hedge_scan_skipped_totalbycause, but the bot labels itreason(src/position_check.rs), so the breakdown was always empty.STARTED_ATwas a lazy static that started on the first/healthcall.PROCESS_STARTis forced first thing inmain.Proof
python3 observability/gen-t0-liquidity.py --check: 5 boards match, everyliq_selector pinned.cargo nextest run -p st0x-hedge health: 9 passed.Merge conditions
liq_*unpinned).