Add prometheus metrics, option to cap per-search results, optimizations#397
Add prometheus metrics, option to cap per-search results, optimizations#397funkypenguin wants to merge 7 commits intog0ldyy:mainfrom
Conversation
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
Signed-off-by: David Young <davidy@funkypenguin.co.nz>
WalkthroughThis PR introduces server-side stream result limiting via new configuration caps, integrates Prometheus metrics to track stream requests by debrid service, adds credential validation for debrid services, and includes supporting infrastructure changes for database replica routing, startup optimizations, and metadata handling improvements. Changes
Sequence DiagramsequenceDiagram
participant Client
participant StreamEndpoint as Stream Endpoint
participant DebridService as Debrid Service
participant MetricsCollector as Metrics Collector
Client->>StreamEndpoint: GET /stream (request)
alt Non-Torrent Debrid Service
StreamEndpoint->>DebridService: validate_credentials()
DebridService->>DebridService: check_premium()
alt Credentials Invalid
DebridService-->>StreamEndpoint: return False
StreamEndpoint->>MetricsCollector: record_stream_request(service)
StreamEndpoint-->>Client: error placeholder stream
else Credentials Valid
DebridService-->>StreamEndpoint: return True
end
end
opt Torrent Service
DebridService-->>StreamEndpoint: return True (immediate)
end
StreamEndpoint->>StreamEndpoint: clamp maxResultsPerResolution by MAX_RESULTS_PER_RESOLUTION_CAP
StreamEndpoint->>StreamEndpoint: process torrents & track streams_emitted
StreamEndpoint->>StreamEndpoint: enforce MAX_STREAM_RESULTS_TOTAL cap
StreamEndpoint->>MetricsCollector: record_stream_request(debrid_service)
MetricsCollector->>MetricsCollector: increment comet_stream_requests_total counter
StreamEndpoint-->>Client: limited streams response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
funkypenguin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comet/metadata/manager.py (1)
85-103: Replace broad exception catching with specific exception types.The try/except block catches all exceptions, which can mask unexpected errors and make debugging difficult. Based on the unpacking operation, you should catch specific exceptions.
Apply this diff to catch only the expected exceptions:
def normalize_metadata(self, metadata: dict, season: int, episode: int): if not metadata: return None try: title, year, year_end = metadata - except Exception: + except (ValueError, TypeError): return None if title is None: return NoneThis change:
ValueErrorhandles incorrect number of values to unpackTypeErrorhandles non-iterable metadata- Makes the error handling explicit and intentional
🧹 Nitpick comments (1)
comet/core/metrics.py (1)
22-26: Hardenrecord_stream_request()normalization (empty/whitespace/non-str) + verify Python version forstr | Nonedef record_stream_request(debrid_service: str | None): """Increment the stream request counter for the provided service.""" - label = (debrid_service or "unknown").lower() + # Defensive normalization (and avoids empty-string labels) + raw = (debrid_service or "").strip() + label = (raw if raw else "unknown").lower() _stream_requests_total.labels(debrid_service=label).inc()If the repo supports Python < 3.10,
str | Nonewill fail at import time—switch toOptional[str](or bump the minimum Python version).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.env-sample(1 hunks)CHANGELOG.md(1 hunks)PR_SUMMARY.md(1 hunks)comet/api/app.py(2 hunks)comet/api/endpoints/config.py(1 hunks)comet/api/endpoints/metrics.py(1 hunks)comet/api/endpoints/stream.py(5 hunks)comet/core/database.py(0 hunks)comet/core/metrics.py(1 hunks)comet/core/models.py(2 hunks)comet/metadata/manager.py(1 hunks)comet/services/debrid.py(2 hunks)comet/services/torrent_manager.py(2 hunks)comet/templates/index.html(1 hunks)comet/utils/parsing.py(1 hunks)deployment/grafana/comet-stream-requests.json(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- comet/core/database.py
🧰 Additional context used
🧬 Code graph analysis (3)
comet/api/endpoints/metrics.py (1)
comet/core/metrics.py (1)
prom_response(33-36)
comet/api/endpoints/stream.py (2)
comet/core/metrics.py (2)
record_non_debrid_stream_request(28-30)record_stream_request(22-25)comet/services/debrid.py (1)
validate_credentials(18-40)
comet/services/debrid.py (2)
comet/debrid/manager.py (2)
get_debrid(28-43)retrieve_debrid_availability(46-63)comet/debrid/stremthru.py (1)
check_premium(44-56)
🪛 Ruff (0.14.8)
comet/metadata/manager.py
91-91: Do not catch blind exception: Exception
(BLE001)
comet/services/debrid.py
36-36: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (17)
.env-sample (1)
211-216: LGTM!The new stream result limit configuration is well-documented with clear explanations of behavior. The default values (0 = disabled) are appropriate for backward compatibility.
comet/utils/parsing.py (1)
52-79: LGTM!The
_safe_inthelper function provides robust integer parsing with proper error handling:
- Handles
None, empty strings, and conversion errors gracefully- Uses sensible fallback values
- The updates to
parse_media_idmake the parsing more resilient to malformed inputcomet/templates/index.html (1)
413-416: LGTM!The conditional rendering logic correctly:
- Sets a dynamic default based on the
disableTorrentStreamssetting- Hides the Torrent option when torrents are disabled
- Aligns with the backend context provided by
comet/api/endpoints/config.pycomet/api/endpoints/config.py (1)
22-22: LGTM!The addition of
disableTorrentStreamsto the template context is straightforward and correctly sources the value fromsettings.DISABLE_TORRENT_STREAMS.CHANGELOG.md (1)
32-33: LGTM!The changelog entries accurately document the new features introduced in this PR.
comet/api/app.py (2)
13-13: LGTM!The metrics router import follows the established pattern in the codebase.
126-126: LGTM!The metrics router is correctly registered with the FastAPI application, following the same pattern as other routers.
comet/services/debrid.py (1)
18-40: Credential validation logic looks good.The defensive approach with early return for "torrent" service and None check for the client is appropriate. The broad
Exceptioncatch (flagged by Ruff BLE001) is acceptable here since credential validation should fail gracefully regardless of the error type, and you're already logging the exception details.comet/core/models.py (1)
121-122: LGTM! Robust input validation for stream cap settings.The validator correctly normalizes various input edge cases (None, non-integers, negatives) to safe defaults. The
mode="before"ensures this runs prior to Pydantic's type coercion, which is the right approach for environment variable parsing.Also applies to: 187-199
comet/services/torrent_manager.py (2)
505-511: LGTM! Query construction is correct.The ON CONFLICT clause is properly formed with target columns and predicate. The cache key lookup and SQL assembly are correct.
468-485: Conflict map correctly implements PostgreSQL partial unique indexes.The POSTGRES_CONFLICT_MAP structure precisely matches the partial unique indexes defined in the database schema. Each target and predicate pair aligns exactly with the corresponding index:
"series"maps totorrents_series_both_idxwith both season and episode"season_only"maps totorrents_season_only_idxwith season only"episode_only"maps totorrents_episode_only_idxwith episode only"none"maps totorrents_no_season_episode_idxwith neitherThe SQL construction at line 509 correctly uses both target and predicate, and the batching logic groups records by conflict key before executing upserts. This implementation properly resolves conflicts for different content types.
deployment/grafana/comet-stream-requests.json (1)
1-169: LGTM! Well-structured Grafana dashboard.The dashboard correctly queries the
comet_stream_requests_totalmetric exposed by the new metrics module. The Prometheus queries are appropriate for visualizing request rates and totals per debrid service.comet/api/endpoints/stream.py (4)
10-11: LGTM!Imports for the new metrics functions are correctly referenced.
145-158: Per-resolution cap and metrics logic looks good.The clamping logic correctly handles both unlimited (0) and over-cap client values. One minor observation:
record_non_debrid_stream_request()is called when service is "torrent" - the naming may seem counterintuitive, but based on the metrics module this tracks non-API (direct torrent) requests, which is the intended behavior.
380-397: Good fail-fast credential validation.This correctly implements the PR objective of failing early for invalid debrid accounts. The validation happens after potential scraping completes but before serving streams, preventing users from receiving cached results they can't actually use.
471-479: Total stream cap implementation is correct.The counter is incremented after each stream is processed, and the break occurs at the start of the next iteration when the cap is reached. This ensures exactly
max_stream_resultsstreams are emitted (or fewer if less are available).Also applies to: 527-528
comet/core/metrics.py (1)
33-36:prom_response()looks correct; verifyCONTENT_TYPE_LATESTbehavior with your prometheus-client version
| from fastapi import APIRouter | ||
|
|
||
| from comet.core.metrics import prom_response | ||
|
|
||
| router = APIRouter() | ||
|
|
||
|
|
||
| @router.get("/metrics") | ||
| async def metrics_endpoint(): | ||
| return prom_response() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "PUBLIC_METRICS_API" --type=py -C3Repository: g0ldyy/comet
Length of output: 1522
LGTM! Metrics endpoint should respect PUBLIC_METRICS_API setting.
The implementation is clean and minimal. However, the metrics endpoint doesn't check settings.PUBLIC_METRICS_API like the admin metrics endpoint does in /admin/api/metrics. For consistency and security, add a check to require admin authentication when PUBLIC_METRICS_API=False:
from fastapi import APIRouter, Cookie
from comet.core.metrics import prom_response
from comet.api.endpoints.admin import require_admin_auth
router = APIRouter()
@router.get("/metrics")
async def metrics_endpoint(admin_session: str = Cookie(None)):
if not settings.PUBLIC_METRICS_API:
await require_admin_auth(admin_session)
return prom_response()🤖 Prompt for AI Agents
In comet/api/endpoints/metrics.py lines 1-10, the metrics endpoint currently
always returns prom_response() and does not respect settings.PUBLIC_METRICS_API;
update the function to import settings, Cookie, and require_admin_auth, add an
admin_session: str = Cookie(None) parameter, and if settings.PUBLIC_METRICS_API
is False call await require_admin_auth(admin_session) before returning
prom_response() so the endpoint requires admin authentication when public
metrics are disabled.
| from fastapi import Response | ||
| from prometheus_client import (CONTENT_TYPE_LATEST, CollectorRegistry, Counter, | ||
| generate_latest) | ||
|
|
||
| # Dedicated registry so we only expose Comet-specific metrics | ||
| _registry = CollectorRegistry() | ||
|
|
||
| _stream_requests_total = Counter( | ||
| "comet_stream_requests_total", | ||
| "Total number of stream requests grouped by debrid service", | ||
| ["debrid_service"], | ||
| registry=_registry, | ||
| ) | ||
|
|
||
| _non_debrid_stream_requests_total = Counter( | ||
| "comet_stream_requests_non_debrid_total", | ||
| "Total number of stream requests that do not require a user API key", | ||
| registry=_registry, | ||
| ) |
There was a problem hiding this comment.
Prometheus label cardinality: ensure debrid_service is bounded/allowlisted
If debrid_service can vary widely (or be user-influenced), comet_stream_requests_total{debrid_service=...} can create high-cardinality time series and stress Prometheus/Grafana. Consider mapping to a small allowlist (and collapsing everything else to "unknown"/"other").
🤖 Prompt for AI Agents
In comet/core/metrics.py lines 1-19, the debrid_service label on
comet_stream_requests_total can create high cardinality; validate and normalize
the label to a bounded allowlist before using it. Add a small constant
ALLOWED_DEBRID_SERVICES set and a helper (e.g., normalize_debrid_service(name))
that returns the original name if in the set or "other"/"unknown" otherwise, and
use that normalized value whenever incrementing
_stream_requests_total.labels(...). Ensure the allowlist is documented and
updated centrally when supported services change.
| # Branch Summary | ||
|
|
||
| - Added replica-aware database routing (`comet/core/db_router.py`, `comet/core/database.py`, `comet/core/models.py`), letting PostgreSQL deployments list `DATABASE_READ_REPLICA_URLS` so reads can go to replicas while writes remain on the primary, plus a force-primary escape hatch inside maintenance queries. | ||
| - Reduced startup thrash by gating the heavy cleanup sweep behind `DATABASE_STARTUP_CLEANUP_INTERVAL` and persisting anime mapping data in `anime_mapping_cache` tables so most boots load instantly from the DB instead of redownloading (`comet/core/database.py`, `comet/services/anime.py`, env knobs `ANIME_MAPPING_SOURCE`/`ANIME_MAPPING_REFRESH_INTERVAL`). | ||
| - Hardened torrent ingestion and ranking: `TorrentManager` now funnels inserts through an `INSERT ... ON CONFLICT DO UPDATE` helper to quiet duplicate-key races, and ranking/logging paths gained small cleanups (`comet/services/torrent_manager.py`, `comet/services/lock.py`, logging tweaks in `comet/core/log_levels.py`). | ||
| - Added an env-toggle to skip Gunicorn preload (`GUNICORN_PRELOAD_APP`) so large deployments can trade startup cost vs. schema-read requirements (`comet/main.py`, `.env-sample`). | ||
| - Introduced `DISABLE_TORRENT_STREAMS` plus customizable placeholder metadata so operators can block magnet-only usage and show a friendly Stremio message instead (`comet/api/endpoints/stream.py`, `comet/core/models.py`, `.env-sample`). | ||
| - Updated documentation/config samples and changelog to capture the new settings and behavior (`.env-sample`, `CHANGELOG.md`). |
There was a problem hiding this comment.
PR summary misses the PR’s headline features (metrics + server-side result caps + fail-early invalid debrid)
Given the PR title/objectives, add explicit bullets for:
- Prometheus
/metricsendpoint + the new counters (and label dimensions) - The new “cap results” settings (per-resolution vs total)
- The “fail early for invalid debrid accounts” behavior
🤖 Prompt for AI Agents
In PR_SUMMARY.md around lines 1 to 8, the summary omits key headline features:
add explicit bullets describing the Prometheus /metrics endpoint and new
counters with their label dimensions, the new result-cap settings
(per-resolution and total caps) and their effect, and the fail-early behavior
for invalid debrid accounts; update the bulleted list to include three concise
bullets covering (1) Prometheus /metrics endpoint plus which counters and labels
were added, (2) the configurable result caps with distinction between
per-resolution vs total caps, and (3) the fail-early invalid debrid account
behavior and where it is enforced.
| "python-multipart", | ||
| "rank-torrent-name", | ||
| "uvicorn", | ||
| "prometheus-client", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
prometheus-client python latest stable version security vulnerabilities
💡 Result:
-
Latest stable prometheus-client (Python) on PyPI: 0.23.1 (released Sep 18, 2025). [1]
-
Known security vulnerabilities: as of now there are no direct/known CVEs reported for the prometheus-client Python package; major scanners (Snyk / Safety DB) list no direct vulnerabilities for the latest release. [2][3]
-
Note: there are security advisories and CVEs related to other Prometheus projects/components (server/exporters) — those are separate from the prometheus-client Python library. Check OSV/GitHub security pages for component-specific advisories. [4]
Sources:
[1] PyPI: prometheus-client package page (release 0.23.1).
[2] Snyk: prometheus-client (pip) security page — no known issues.
[3] Safety DB / ReleaseAlert summaries for prometheus-client.
[4] OSV / GitHub security advisories for Prometheus projects (for exporter/server CVEs).
Consider pinning prometheus-client to a specific version for reproducibility.
Adding a version constraint (e.g., "prometheus-client==0.23.1" or "prometheus-client>=0.20,<1.0") ensures consistent builds across environments. The latest stable version is 0.23.1 with no known security vulnerabilities reported.
🤖 Prompt for AI Agents
In pyproject.toml around line 28, the dependency "prometheus-client" is unpinned
which can lead to non-reproducible builds; update the dependency to include an
explicit version constraint such as "prometheus-client==0.23.1" or a compatible
range like "prometheus-client>=0.20,<1.0" to ensure consistent installs across
environments and add that updated spec in place of the current entry.
1 participant
Another copilot-driven PR, feel free to disregard :)
This PR includes:
Summary by CodeRabbit
New Features
Bug Fixes
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.