Add native Prometheus /internal/metrics endpoint#187
Conversation
Expose the same operational data as /internal/status in Prometheus text format so scrapers can ingest Olla metrics without an external wrapper.
WalkthroughAdds a Prometheus metrics endpoint, refactors status snapshot assembly, exposes the endpoint in route/version metadata, and adds local Prometheus/Grafana compose examples plus Makefile targets. ChangesPrometheus metrics and local dev stack
Estimated review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Provide example compose and Prometheus configs for local stacks that build Olla from source and scrape /internal/metrics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/app/handlers/handler_metrics.go (2)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid round-tripping the success rate through a formatted string.
parsePercentagere-parsesresponse.System.SuccessRate, whichbuildSystemSummaryproduced viaformat.Percentage. Parsing back from display text is fragile (formatting changes silently break the metric). The raw value is available fromsnapshot.proxyStats; compute it directly for the gauge.Also applies to: 171-177
🤖 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 `@internal/app/handlers/handler_metrics.go` around lines 51 - 52, The success-rate metric is being derived by parsing display-formatted text, which is fragile; update the gauge path in handler_metrics.go to compute the value directly from the raw proxy statistics instead of calling parsePercentage on response.System.SuccessRate. Use the existing raw data from snapshot.proxyStats in buildSystemSummary and the related metric-writing logic (including the later matching section around the other success-rate gauge) so the Prometheus gauge is based on the original numeric value, not a formatted string.
106-138: 🚀 Performance & Scalability | 🔵 TrivialPer-endpoint
statuslabel causes time-series churn.Each health-state transition for an endpoint creates a new series because the string value of the
statuslabel changes (for example, from "healthy" to "unhealthy"). This churn leaves stale series in the TSDB, increases memory pressure and degrades query performance. Theolla_endpoint_upgauge already expresses health numerically, so thestatuslabel is redundant for that metric. It is advisable to drop thestatuslabel from all gauges and key the series solely on the stableendpointlabel.🤖 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 `@internal/app/handlers/handler_metrics.go` around lines 106 - 138, The per-endpoint status label is creating unstable metric series in handler_metrics.go, so update the Prometheus gauge writes in the metrics loop to remove the changing status label and keep only the stable endpoint label. Use the existing writePrometheusLabeledGauge calls in this handler to drop status from olla_endpoint_up and the other endpoint gauges, while preserving the health signal via the numeric up value and the existing endpoint name identity.
🤖 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 `@docker-compose.local.yaml.example`:
- Around line 41-44: The Grafana admin credentials in the compose example are
hardcoded in the environment section, so update the
docker-compose.local.yaml.example entries for GF_SECURITY_ADMIN_USER and
GF_SECURITY_ADMIN_PASSWORD to use environment variable interpolation instead of
literal values. Keep GF_USERS_ALLOW_SIGN_UP as-is, and make sure the service
configuration still references the same environment block so users can supply
secrets externally when running Grafana.
In `@internal/app/handlers/handler_metrics.go`:
- Around line 142-151: The totalTrafficBytes helper is incorrectly basing
aggregation on connectionStats, which skips endpoints with traffic but no active
connections; update totalTrafficBytes to sum directly from endpointStats so the
ola_total_traffic_bytes metric stays accurate. Use the existing
totalTrafficBytes symbol in handler_metrics.go and align its logic with
buildSystemSummary by extracting a shared helper if needed to keep the API
response and metrics consistent.
---
Nitpick comments:
In `@internal/app/handlers/handler_metrics.go`:
- Around line 51-52: The success-rate metric is being derived by parsing
display-formatted text, which is fragile; update the gauge path in
handler_metrics.go to compute the value directly from the raw proxy statistics
instead of calling parsePercentage on response.System.SuccessRate. Use the
existing raw data from snapshot.proxyStats in buildSystemSummary and the related
metric-writing logic (including the later matching section around the other
success-rate gauge) so the Prometheus gauge is based on the original numeric
value, not a formatted string.
- Around line 106-138: The per-endpoint status label is creating unstable metric
series in handler_metrics.go, so update the Prometheus gauge writes in the
metrics loop to remove the changing status label and keep only the stable
endpoint label. Use the existing writePrometheusLabeledGauge calls in this
handler to drop status from olla_endpoint_up and the other endpoint gauges,
while preserving the health signal via the numeric up value and the existing
endpoint name identity.
🪄 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: 233fee9a-9ad4-4e8b-96f9-026b015d6a2c
📒 Files selected for processing (12)
config/prometheus.local.yaml.exampledocker-compose.local.yaml.exampledocker-compose.yamlinternal/app/handlers/handler_metrics.gointernal/app/handlers/handler_metrics_test.gointernal/app/handlers/handler_status.gointernal/app/handlers/handler_status_endpoints_test.gointernal/app/handlers/handler_version.gointernal/app/handlers/server_routes.gointernal/core/constants/content.gointernal/core/constants/endpoint.gomakefile
| environment: | ||
| - GF_SECURITY_ADMIN_USER=admin | ||
| - GF_SECURITY_ADMIN_PASSWORD=admin | ||
| - GF_USERS_ALLOW_SIGN_UP=false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid hardcoded Grafana admin credentials in the compose example.
Line 42 and Line 43 embed sensitive auth config directly in the file. Move these to environment variable interpolation so secrets are not committed or copied around.
As per coding guidelines: "Use environment variables for storing API keys and sensitive configuration."
Suggested patch
environment:
- - GF_SECURITY_ADMIN_USER=admin
- - GF_SECURITY_ADMIN_PASSWORD=admin
+ - GF_SECURITY_ADMIN_USER=${GRAFANA_ADMIN_USER:-admin}
+ - GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD}
- GF_USERS_ALLOW_SIGN_UP=false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| environment: | |
| - GF_SECURITY_ADMIN_USER=admin | |
| - GF_SECURITY_ADMIN_PASSWORD=admin | |
| - GF_USERS_ALLOW_SIGN_UP=false | |
| environment: | |
| - GF_SECURITY_ADMIN_USER=${GRAFANA_ADMIN_USER:-admin} | |
| - GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD} | |
| - GF_USERS_ALLOW_SIGN_UP=false |
🤖 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 `@docker-compose.local.yaml.example` around lines 41 - 44, The Grafana admin
credentials in the compose example are hardcoded in the environment section, so
update the docker-compose.local.yaml.example entries for GF_SECURITY_ADMIN_USER
and GF_SECURITY_ADMIN_PASSWORD to use environment variable interpolation instead
of literal values. Keep GF_USERS_ALLOW_SIGN_UP as-is, and make sure the service
configuration still references the same environment block so users can supply
secrets externally when running Grafana.
Source: Coding guidelines
| func totalTrafficBytes(snapshot *statusSnapshot) int64 { | ||
| var total int64 | ||
| for url, conn := range snapshot.connectionStats { | ||
| _ = conn | ||
| if stats, exists := snapshot.endpointStats[url]; exists { | ||
| total += stats.TotalBytes | ||
| } | ||
| } | ||
| return total | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -C4 'func .*GetConnectionStats\s*\('Repository: thushan/olla
Length of output: 150
totalTrafficBytes omits endpoints with no active connections.
The logic iterates over connectionStats, excluding any endpoint that has recorded traffic but zero active connections. Since endpointStats accumulates the definitive byte counts, the loop should aggregate directly from there to ensure the ola_total_traffic_bytes metric remains accurate even when connections drop.
This logic is identical to the redundant pattern in buildSystemSummary; consider extracting a shared helper to guarantee consistency between the API response and metrics.
♻️ Suggested fix
func totalTrafficBytes(snapshot *statusSnapshot) int64 {
var total int64
- for url, conn := range snapshot.connectionStats {
- _ = conn
- if stats, exists := snapshot.endpointStats[url]; exists {
- total += stats.TotalBytes
- }
- }
+ for _, stats := range snapshot.endpointStats {
+ total += stats.TotalBytes
+ }
return total
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func totalTrafficBytes(snapshot *statusSnapshot) int64 { | |
| var total int64 | |
| for url, conn := range snapshot.connectionStats { | |
| _ = conn | |
| if stats, exists := snapshot.endpointStats[url]; exists { | |
| total += stats.TotalBytes | |
| } | |
| } | |
| return total | |
| } | |
| func totalTrafficBytes(snapshot *statusSnapshot) int64 { | |
| var total int64 | |
| for _, stats := range snapshot.endpointStats { | |
| total += stats.TotalBytes | |
| } | |
| return total | |
| } |
🤖 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 `@internal/app/handlers/handler_metrics.go` around lines 142 - 151, The
totalTrafficBytes helper is incorrectly basing aggregation on connectionStats,
which skips endpoints with traffic but no active connections; update
totalTrafficBytes to sum directly from endpointStats so the
ola_total_traffic_bytes metric stays accurate. Use the existing
totalTrafficBytes symbol in handler_metrics.go and align its logic with
buildSystemSummary by extracting a shared helper if needed to keep the API
response and metrics consistent.
1 participant
Summary
GET /internal/metricsendpoint that exposes the same operational data as/internal/statusin Prometheus text format/versionMetrics exposed
System-level gauges/counters aligned with the monitoring docs:
olla_requests_total,olla_failures_total,olla_endpoints_healthy,olla_avg_latency_ms,olla_success_rate_percent,olla_active_connections,olla_uptime_seconds,olla_total_traffic_bytes,olla_security_violations_totalendpointandstatuslabels (requests, connections, success rate, latency, traffic, priority, model count, up)olla_infowith version, commit, engine, profile, and balancer labelsTest plan
make ready/internal/metricshandler and Prometheus formatting helperscurl -s http://localhost:40114/internal/metrics | head/internal/metricsSummary by CodeRabbit
New Features
Bug Fixes
Tests