Repository navigation
Conversation
JuaniRios
changed the title
87ac2d9 to
7864332
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
76a7a16 to
adf06ec
Compare
7864332 to
5bfdb50
Compare
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 (7)
Included review availability: This review used your included allowance. 6 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 change adds a typed Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No additional merge-blocking issue is established. Complete the stated deployment checks and merge conditions before release. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 67.12% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 73 functions across 5 files. (4 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. |
| Description check | |
The description clearly explains the family store, health and settings metric publication, typed gauges, compatibility behavior, tests, and deployment conditions. It is directly related to the changes… |
| Title check | |
The title clearly identifies the main change: publishing the liq_* foundation families from the bot. It is concise and specific. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 67.12% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 73 functions across 5 files. (4 skipped: 4 unsupported.)
- Fix all pre-merge checks with AI
✨ 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 adds a liq_* family store to the bot's /metrics. Each family is replaced as one unit, so a removed symbol drops out on the next scrape. It publishes the health and settings families with the exporter sidecar's names, labels and untyped exposition. Golden tests compare the Rust builders with the exporter's own output, and SPEC.md, ADR 0026 and the observability docs describe the contract.
Overall read: the store is small and correct. Replace takes the lock only to swap an Arc, and render formats outside the lock. Samples are checked at construction (label keys, finite values). Wrong-family and duplicate samples are dropped and logged, and a liq_ name the recorder already rendered is skipped. Nothing reads the new series yet, so the live risk is low.
Several lanes raised that the new docs disclose monitoring topology in a public repo, and that /metrics serves the settings without auth. I checked both and dropped them. The job names, t0.devops, the exporter path, the capital probe and Cloud Monitoring were already public in this repo before the PR. The same settings DTO is already served on the same listener under /api, and the exporter already republishes it. Both endpoints listen only on the VM (docs/observability.md).
Two minor test-quality notes remain. Neither blocks.
claude-opus-5-5 · high · 11 min
adf06ec to
9b093f3
Compare
5bfdb50 to
70c7eef
Compare
9b093f3 to
cca682c
Compare
70c7eef to
c766b5f
Compare
cca682c to
8cf45d2
Compare
c766b5f to
85754af
Compare
✅ Action performedReview finished.
|
c6ff16c to
bbf7fcf
Compare
44039e0 to
815310e
Compare
|
@rain-marvin review |
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR adds a liq_* family store to the bot's /metrics endpoint and publishes the first two families, health and settings, with the same names, labels and values as the exporter sidecar. The store replaces each family as a whole on publish, so removed symbols leave /metrics instead of keeping their last value, and every liq_* block is typed as a gauge. Boot publishes both families once, after the config loads. Nothing reads the new series yet, and the change touches no money path or stored data.
Overall read: sound and ready to merge. The store holds its mutex only to swap or clone a family, and formatting runs after the lock is released, so a scrape cannot stall the bot. Rendering follows the Prometheus text format and matches the exporter goldens. No schema, migration or config-format change can break an upgrade. Three small comments:
SettingsInput::from_ctxrepeats the dashboard's settings-selection rules, and the parity test only covers the branches where the two copies cannot differ.- The freshness series for
healthandsettingsstays at the boot time, which a later staleness alert can misread as a stalled collector. - The ADR file name still says "untyped", which contradicts the decision.
One more concern was checked and dismissed: liq_settings_info carries the wallet address and the Turnkey organization id as labels. The public repo only holds placeholder values, the exporter already publishes the same labels to the same private Prometheus, and neither value is a secret.
Earlier threads (the macro scan, the concurrency test name, DROP_LIST) are resolved, and nothing here repeats them.
claude-opus-5-5 · high · 25 min
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
Add a liq_* family store to the bot's /metrics endpoint, rendered after the recorder's output and untyped like the exporter sidecar, so Cloud Monitoring keeps one series history across the cutover. Each refresh replaces a whole family, so a removed symbol, day or chain disappears instead of keeping its last value. This PR publishes the health and settings families with the exporter's names, labels and values. Golden tests compare the builders with the exporter's output for the same fixtures; scripts/liq-parity/golden.py regenerates those files from a pinned exporter revision. Adds ADR 0026 (proposed), the SPEC metrics contract and the metrics catalog in docs/observability.md.
bbf7fcf to
ddc1e42
Compare
815310e to
d1e622c
Compare
JuaniRios
changed the title
This branch has not been deployed
2 participants
Part of RAI-3085.
Adds a
liq_*family store to the bot's/metricsand publishes the first two families, health and settings, with the same names, labels and values as the exporter sidecar. Part of the RAI-2998 stack; items 2 to 4 port inventory, performance and PnL on top.Live effect: the bot's
/metricsgainsliq_bot_*,liq_settings_*andliq_asset_*series (scraped as jobt0-liquidity); nothing reads them yet because every consumer is pinned to the exporter job · Risk: low (no money path or stored data; boot only builds a few gauges) · Ships: on the next bot releaseWhy
The exporter re-derives numbers the bot already has, from outside the app (see the Zulip thread and the plan). Moving the metrics into the bot starts here.
Decisions
metricsrecorder. The recorder never forgets a label set, so a removed symbol would keep its last value forever. The store replaces a whole family on each publish, like the exporter does. ADR 0026 (status Proposed).liq_*block has# HELPplus# TYPE <name> gauge. Why: Managed Prometheus ingests untyped samples twice (as gauge and as counter), so untyped roughly doubles theliq_*sample cost. Typing now also saves a second migration later, and gives one descriptor per name, so tools treat them right. Every name really is a gauge: snapshots,_24hwindows that go down,*_totalnames that mean "count right now", and precomputed quantiles that aren't real summaries.jobselector has a gap at cutover. While both run, the same name exists as untyped (exporter job) and gauge (bot job). Nothing counts twice: the board selects byjob=~"$source", and the capital probe and TVL report are pinned to the exporter job (t0.devops#787).*_total(liq_pending_orders_total,liq_raindex_orders_total,liq_pending_orders_uncapped_total, and the inventory totals) and on the_msnames. Known exceptions, the names keep the exporter's meaning. Usepromtool check metrics --lint=none --extended.liq_name goes through themetricsmacros.trading_mode="", absent vs zero). The rule and its one exception are in SPEC.md under "Prometheus metrics".# TYPE).golden.pyregenerates them and refuses any exporter file that isn't t0.devops226c029(sha256 pinned).Proof
golden.py --checkmatches.cargo nextest run -p st0x-hedge -p st0x-config5354 passed,golden.py --check12 match,test_compare.pyandgen-t0-liquidity.py --checkpass./metricsand the Cloud Monitoring descriptor check. Both need a release.Merge conditions