feat: EarnFM token auth, Prometheus metrics, orchestrator resilience#62
Merged
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 33 minutes and 4 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR migrates EarnFM to token-based authentication, reduces Docker privilege across bandwidth services, introduces a comprehensive Prometheus metrics system, hardens container status operations, and updates documentation and tests accordingly. ChangesEarnFM and Service Configuration Updates
Prometheus Metrics System
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/metrics.py (1)
305-308: 💤 Low valueAvoid relying on prometheus_client private
Gauge._metricsinternals inapp/metrics.py.
_refresh_gauges()clears label sets viam["..."]._metrics.clear()(lines 305-308, plus 356, 362-363, 387-388), which is an undocumented internal that can break on library updates. Prefer the public API (e.g.,Gauge.clear()/remove()/remove_by_labels()) or document why internals are required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/metrics.py` around lines 305 - 308, _in _refresh_gauges(), stop using the private _metrics dict and instead clear labelsets via the public API: for each metric key (e.g., "containers_total", "container_info", "container_cpu_percent", "container_memory_mb") either iterate m["..."].collect() to extract each sample's label dict and call the Gauge.remove(...) for that labelset, or unregister the entire metric from the CollectorRegistry and re-create it (using Registry.unregister and re-instantiating the Gauge) to achieve a full reset; update _refresh_gauges() to use one of these approaches rather than m["..."]._metrics.clear().docs/guides/prometheus-metrics.md (1)
14-14: ⚡ Quick winAdd an explicit “internal-only” exposure warning for
/metrics.The guide says
/metricsis unauthenticated but doesn’t warn against public exposure. Add a short warning to keep it private (LAN/VPN/reverse-proxy auth/IP allowlist) to prevent leaking operational metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/prometheus-metrics.md` at line 14, Add an explicit internal-only warning for the unauthenticated GET /metrics endpoint: update the documentation text that mentions "Metrics are exposed at `GET /metrics`" to append a short caution that `/metrics` must not be publicly accessible and should be restricted to trusted networks (LAN/VPN) or protected via a reverse-proxy with authentication or IP allowlist to avoid leaking operational metadata.tests/test_metrics.py (1)
58-97: ⚡ Quick wintests/test_metrics.py: reduce fragility from
prometheus_clientprivate metric accessThe assertions read values via Prometheus client private internals (
._value.get()), which isn’t part of the public API and can break with upgrades (even though the current test run succeeds). Consider asserting through a stable path (e.g.,generate_latest()output or parsing metriccollect()results) to make the tests upgrade-proof.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metrics.py` around lines 58 - 97, The tests currently assert metric values by touching Prometheus internals (metrics._metrics["..."]._value.get()), which is fragile; update each test that calls metrics.record_collection_end, record_collection_error, record_container_lifecycle, record_login, record_rate_limit, and record_heartbeat to assert via the public Prometheus API instead — e.g., call prometheus_client.generate_latest(...) or use each metric's collect() and inspect Sample.value for the matching label set (replace any usage of metrics._metrics["..."]._value.get() or ._value with parsing of generate_latest output or reading the sample values returned by metric.collect()) so assertions no longer rely on private attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/main.py`:
- Line 939: The lifecycle metrics for stop/restart are recorded before the proxy
call, so failures are counted as successes; move the
metrics.record_container_lifecycle calls for "stop" and "restart" so they
execute after a successful _proxy_worker_command call (i.e., only record when
_proxy_worker_command returns/does not raise), updating the occurrences of
metrics.record_container_lifecycle("stop", slug) and
metrics.record_container_lifecycle("restart", slug) to run after the proxy call
in the same handler functions.
In `@app/metrics.py`:
- Around line 260-262: The except block increments
_metrics["http_requests_total"] for status "500" but fails to record latency;
update the exception path to also observe the request duration on the duration
histogram (e.g. _metrics["http_request_duration_seconds"].labels(method=method,
path=path, status="500").observe(duration)). Compute duration the same way the
success path does (use the existing start_time or timer used elsewhere, or
compute duration = time.time() - start_time before observing) and then re-raise
the exception; ensure you reference the same label keys (method, path, status)
as used for successful requests.
In `@app/static/js/app.js`:
- Around line 763-765: The code injects user-controlled `hint` directly into
`body.innerHTML` which creates an XSS vector; change the build to avoid raw HTML
insertion by creating a DOM element for the hint (e.g., create a <p> node and
set its textContent to `hint`) or sanitize `hint` with a strict allowlist before
adding it, then append that element to the container and combine with
`fieldsHtml` (or insert `fieldsHtml` into a separate container) instead of
interpolating `hint` into `innerHTML`; update the code paths that reference
`body.innerHTML`, `hint`, and `fieldsHtml` accordingly.
---
Nitpick comments:
In `@app/metrics.py`:
- Around line 305-308: _in _refresh_gauges(), stop using the private _metrics
dict and instead clear labelsets via the public API: for each metric key (e.g.,
"containers_total", "container_info", "container_cpu_percent",
"container_memory_mb") either iterate m["..."].collect() to extract each
sample's label dict and call the Gauge.remove(...) for that labelset, or
unregister the entire metric from the CollectorRegistry and re-create it (using
Registry.unregister and re-instantiating the Gauge) to achieve a full reset;
update _refresh_gauges() to use one of these approaches rather than
m["..."]._metrics.clear().
In `@docs/guides/prometheus-metrics.md`:
- Line 14: Add an explicit internal-only warning for the unauthenticated GET
/metrics endpoint: update the documentation text that mentions "Metrics are
exposed at `GET /metrics`" to append a short caution that `/metrics` must not be
publicly accessible and should be restricted to trusted networks (LAN/VPN) or
protected via a reverse-proxy with authentication or IP allowlist to avoid
leaking operational metadata.
In `@tests/test_metrics.py`:
- Around line 58-97: The tests currently assert metric values by touching
Prometheus internals (metrics._metrics["..."]._value.get()), which is fragile;
update each test that calls metrics.record_collection_end,
record_collection_error, record_container_lifecycle, record_login,
record_rate_limit, and record_heartbeat to assert via the public Prometheus API
instead — e.g., call prometheus_client.generate_latest(...) or use each metric's
collect() and inspect Sample.value for the matching label set (replace any usage
of metrics._metrics["..."]._value.get() or ._value with parsing of
generate_latest output or reading the sample values returned by
metric.collect()) so assertions no longer rely on private attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa8df843-4c8d-438f-8dc9-a7d442b4e428
📒 Files selected for processing (27)
README.mdapp/collectors/__init__.pyapp/collectors/earnfm.pyapp/main.pyapp/metrics.pyapp/orchestrator.pyapp/static/css/style.cssapp/static/js/app.jsdocs/guides/earnfm.mddocs/guides/peer2profit.mddocs/guides/prometheus-metrics.mddocs/guides/speedshare.mdrequirements.txtservices/bandwidth/bitping.ymlservices/bandwidth/earnapp.ymlservices/bandwidth/earnfm.ymlservices/bandwidth/honeygain.ymlservices/bandwidth/iproyal.ymlservices/bandwidth/mysterium.ymlservices/bandwidth/packetstream.ymlservices/bandwidth/proxyrack.ymlservices/bandwidth/repocket.ymlservices/bandwidth/traffmonetizer.ymltests/test_collectors_deep.pytests/test_collectors_extended.pytests/test_coverage_gaps.pytests/test_metrics.py
Make orchestrator skip corrupted containers instead of crashing the entire status endpoint. Add clickable URLs in credential hint text with distinct accent color so users can quickly navigate to auth pages.
Rich observability when CASHPILOT_METRICS_ENABLED=true: - HTTP request count/latency/in-flight by method/path/status - Container count, CPU/memory per service, lifecycle events - Earnings balance per platform in native currency and USD - Collection duration histogram, per-platform errors, staleness - Worker heartbeat tracking, Docker availability, container counts - Health scores and uptime percentages per service - Auth login attempts and rate-limit counters - System uptime and build info Gated behind env var so zero overhead when disabled.
Peer2Profit: domain completely unreachable, all services offline. SpeedShare: authentication broken, confirmed dead in Discord Mar 2026.
- earnfm: handle null API response body (or {} fallback)
- metrics: 30s TTL cache on _refresh_gauges to avoid DB load per scrape
- metrics: remove image label from container_info (cardinality risk)
- metrics: precompile path regex, cover sub-routes (/services/{slug}/*)
- metrics: use .set() for containers_total instead of .inc() after clear
- metrics: fix timestamp handling for timezone-aware last_seen
- orchestrator: simplify redundant (APIError, Exception) to Exception
- Move stop/restart lifecycle metrics after proxy call (only count success) - Record HTTP duration on 500 errors in metrics middleware - Add security warning about /metrics endpoint exposure in docs
- Add sanitizeHint() to strip all tags except a/b/code with safe attrs - Replace ._metrics.clear() with public .clear() method
GeiserX
force-pushed
the
fix/earnfm-and-privileged
branch
from
d84c3e8 to
70653a0
Compare
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (82.09%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
==========================================
- Coverage 92.03% 91.18% -0.86%
==========================================
Files 24 25 +1
Lines 2499 2722 +223
==========================================
+ Hits 2300 2482 +182
- Misses 199 240 +41
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
privileged: truefrom all 10 bandwidth service YAMLs (none actually need it; Mysterium usescap_add: NET_ADMINcorrectly)/metricsendpoint gated behindCASHPILOT_METRICS_ENABLED=true— HTTP latency, container CPU/memory, earnings per platform, collection health, worker heartbeats, auth counters, health scoresTest plan
Closes #58, closes #59
Summary by CodeRabbit
Release Notes
New Features
/metricsendpoint for monitoring and observability.Bug Fixes
Documentation
Style
Chores