Skip to content

Commit 63a7116

Browse files
jyejarejyejare
committed
fix: Reviewers comment fixed
Signed-off-by: Jitendra Yejare <11752425+jyejare@users.noreply.github.com>
1 parent d271367 commit 63a7116

4 files changed

Lines changed: 213 additions & 13 deletions

File tree

‎sdk/python/feast/api/registry/rest/system_metrics.py‎

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@
1414
_DEFAULT_THANOS_URL = "https://thanos-querier.openshift-monitoring.svc:9091"
1515
_METRICS_PORT = 8000
1616

17+
# Histogram suffixes that should be folded into the base metric family.
18+
# Counter _total / _created are intentionally NOT stripped so the
19+
# canonical metric name (e.g. feast_offline_store_request_total) is preserved.
20+
_HISTOGRAM_SUFFIXES = ("_bucket", "_sum", "_count")
21+
1722

1823
def _read_sa_token() -> Optional[str]:
1924
try:
@@ -31,17 +36,24 @@ def _get_ca_bundle() -> str:
3136

3237

3338
def _parse_prometheus_text(text: str) -> dict:
34-
"""Parse Prometheus exposition format into structured metric families."""
39+
"""Parse Prometheus exposition format into a structured dict of metric families.
40+
41+
Used by the ``/system-metrics/scrape`` endpoint to convert the raw
42+
text exposition from the local ``/metrics`` endpoint into JSON so the
43+
UI dashboard can consume it without a Prometheus server.
44+
45+
Returns a dict keyed by metric family name, each containing:
46+
- ``type``: histogram, counter, gauge, etc.
47+
- ``samples``: list of {name, labels, value} dicts.
48+
"""
3549
metrics: dict = {}
3650
current_type = ""
3751

3852
for line in text.splitlines():
3953
line = line.strip()
40-
if not line:
54+
if not line or line.startswith("# HELP"):
4155
continue
42-
if line.startswith("# HELP"):
43-
parts = line.split(None, 3)
44-
elif line.startswith("# TYPE"):
56+
if line.startswith("# TYPE"):
4557
parts = line.split(None, 3)
4658
current_type = parts[3] if len(parts) > 3 else "untyped"
4759
elif not line.startswith("#"):
@@ -54,15 +66,15 @@ def _parse_prometheus_text(text: str) -> dict:
5466
labels_str = match.group(2) or ""
5567
value = match.group(3)
5668
base_name = name
57-
for suffix in ("_total", "_bucket", "_sum", "_count", "_created"):
69+
for suffix in _HISTOGRAM_SUFFIXES:
5870
if base_name.endswith(suffix):
5971
base_name = base_name[: -len(suffix)]
6072
break
6173

6274
if base_name not in metrics:
6375
metrics[base_name] = {"type": current_type, "samples": []}
6476
try:
65-
val = float(value)
77+
val: float | str = float(value)
6678
except ValueError:
6779
val = value
6880
metrics[base_name]["samples"].append(
@@ -76,6 +88,10 @@ def _parse_prometheus_text(text: str) -> dict:
7688

7789

7890
def get_system_metrics_router(grpc_handler, store=None):
91+
# Authentication is enforced at the FastAPI application level via
92+
# inject_user_details (see rest_registry_server.py). These endpoints
93+
# expose operational infrastructure metrics, not feature-level data,
94+
# so no additional RBAC (assert_permissions) checks are required.
7995
router = APIRouter()
8096

8197
def _get_prometheus_url() -> str:
@@ -100,7 +116,7 @@ def _query_prometheus(path: str, params: dict) -> dict:
100116
headers["Authorization"] = f"Bearer {token}"
101117

102118
ca_bundle = _get_ca_bundle()
103-
verify = ca_bundle if ca_bundle else False
119+
verify: bool | str = ca_bundle if ca_bundle else False
104120

105121
try:
106122
resp = http_requests.get(
@@ -125,7 +141,7 @@ def _query_prometheus(path: str, params: dict) -> dict:
125141
)
126142

127143
@router.get("/system-metrics/query", tags=["System Metrics"])
128-
async def promql_instant(
144+
def promql_instant(
129145
query: str = Query(..., description="PromQL expression"),
130146
time: Optional[str] = Query(
131147
None, description="Evaluation timestamp (RFC3339 or Unix)"
@@ -138,7 +154,7 @@ async def promql_instant(
138154
return _query_prometheus("/api/v1/query", params)
139155

140156
@router.get("/system-metrics/query_range", tags=["System Metrics"])
141-
async def promql_range(
157+
def promql_range(
142158
query: str = Query(..., description="PromQL expression"),
143159
start: str = Query(..., description="Start timestamp"),
144160
end: str = Query(..., description="End timestamp"),
@@ -156,7 +172,7 @@ async def promql_range(
156172
)
157173

158174
@router.get("/system-metrics/scrape", tags=["System Metrics"])
159-
async def scrape_metrics():
175+
def scrape_metrics():
160176
"""Fallback: scrape the local Prometheus metrics endpoint directly."""
161177
try:
162178
resp = http_requests.get(

‎sdk/python/feast/infra/feature_servers/base_config.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,9 @@ class MetricsConfig(FeastConfigBaseModel):
9595

9696
prometheus_url: Optional[str] = None
9797
"""URL of the Prometheus or Thanos Querier API for the System Health
98-
dashboard. On OpenShift defaults to
98+
dashboard. Defaults to
9999
``https://thanos-querier.openshift-monitoring.svc:9091``.
100+
Override via this field or the ``FEAST_PROMETHEUS_URL`` env var.
100101
The registry REST API proxies PromQL queries through this URL."""
101102

102103

‎sdk/python/feast/infra/offline_stores/offline_store.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,14 @@ def __init__(
7474

7575
def _extract_retrieval_metadata(job: "RetrievalJob") -> tuple:
7676
"""Return ``(feature_view_names, feature_count)`` from a RetrievalJob's metadata."""
77+
from feast.utils import _parse_feature_ref
78+
7779
try:
7880
meta = job.metadata
7981
if meta:
8082
feature_count = len(meta.features)
8183
feature_views = list(
82-
{ref.split(":")[0] for ref in meta.features if ":" in ref}
84+
{_parse_feature_ref(ref)[0] for ref in meta.features if ":" in ref}
8385
)
8486
return feature_views, feature_count
8587
except (NotImplementedError, AttributeError):

‎sdk/python/tests/unit/test_metrics.py‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1732,3 +1732,184 @@ def test_does_not_raise_on_failure(self):
17321732
status="error",
17331733
latency_ms=5.0,
17341734
)
1735+
1736+
1737+
# ---------------------------------------------------------------------------
1738+
# System Metrics endpoint tests
1739+
# ---------------------------------------------------------------------------
1740+
1741+
1742+
class TestParsePrometheusText:
1743+
"""Tests for _parse_prometheus_text in system_metrics.py."""
1744+
1745+
def test_parses_counter(self):
1746+
from feast.api.registry.rest.system_metrics import _parse_prometheus_text
1747+
1748+
text = (
1749+
"# HELP feast_offline_store_request_total Total requests\n"
1750+
"# TYPE feast_offline_store_request_total counter\n"
1751+
'feast_offline_store_request_total{method="to_arrow",status="success"} 42\n'
1752+
)
1753+
result = _parse_prometheus_text(text)
1754+
assert "feast_offline_store_request_total" in result
1755+
entry = result["feast_offline_store_request_total"]
1756+
assert entry["type"] == "counter"
1757+
assert len(entry["samples"]) == 1
1758+
assert entry["samples"][0]["value"] == 42.0
1759+
1760+
def test_preserves_total_suffix(self):
1761+
"""Counter _total suffix must NOT be stripped (was a reported bug)."""
1762+
from feast.api.registry.rest.system_metrics import _parse_prometheus_text
1763+
1764+
text = (
1765+
"# TYPE feast_offline_store_request_total counter\n"
1766+
"feast_offline_store_request_total 10\n"
1767+
)
1768+
result = _parse_prometheus_text(text)
1769+
assert "feast_offline_store_request_total" in result
1770+
assert "feast_offline_store_request" not in result
1771+
1772+
def test_groups_histogram_samples(self):
1773+
from feast.api.registry.rest.system_metrics import _parse_prometheus_text
1774+
1775+
text = (
1776+
"# TYPE feast_offline_store_request_latency_seconds histogram\n"
1777+
'feast_offline_store_request_latency_seconds_bucket{le="0.1"} 5\n'
1778+
'feast_offline_store_request_latency_seconds_bucket{le="1.0"} 15\n'
1779+
"feast_offline_store_request_latency_seconds_sum 12.5\n"
1780+
"feast_offline_store_request_latency_seconds_count 15\n"
1781+
)
1782+
result = _parse_prometheus_text(text)
1783+
assert "feast_offline_store_request_latency_seconds" in result
1784+
entry = result["feast_offline_store_request_latency_seconds"]
1785+
assert entry["type"] == "histogram"
1786+
assert len(entry["samples"]) == 4
1787+
1788+
def test_empty_input(self):
1789+
from feast.api.registry.rest.system_metrics import _parse_prometheus_text
1790+
1791+
assert _parse_prometheus_text("") == {}
1792+
1793+
def test_skips_comment_and_help_lines(self):
1794+
from feast.api.registry.rest.system_metrics import _parse_prometheus_text
1795+
1796+
text = (
1797+
"# HELP some_metric A help string\n"
1798+
"# This is a random comment\n"
1799+
"# TYPE some_metric gauge\n"
1800+
"some_metric 3.14\n"
1801+
)
1802+
result = _parse_prometheus_text(text)
1803+
assert "some_metric" in result
1804+
assert result["some_metric"]["samples"][0]["value"] == 3.14
1805+
1806+
def test_non_numeric_value(self):
1807+
from feast.api.registry.rest.system_metrics import _parse_prometheus_text
1808+
1809+
text = "# TYPE info_metric gauge\ninfo_metric NaN\n"
1810+
result = _parse_prometheus_text(text)
1811+
assert len(result["info_metric"]["samples"]) == 1
1812+
1813+
1814+
class TestSystemMetricsRouter:
1815+
"""Tests for the FastAPI system-metrics router endpoints."""
1816+
1817+
def _make_test_client(self, store=None):
1818+
from fastapi import FastAPI
1819+
from fastapi.testclient import TestClient
1820+
1821+
from feast.api.registry.rest.system_metrics import get_system_metrics_router
1822+
1823+
app = FastAPI()
1824+
app.include_router(get_system_metrics_router(grpc_handler=None, store=store))
1825+
return TestClient(app)
1826+
1827+
@patch("feast.api.registry.rest.system_metrics.http_requests.get")
1828+
def test_promql_instant_success(self, mock_get):
1829+
mock_resp = MagicMock()
1830+
mock_resp.status_code = 200
1831+
mock_resp.json.return_value = {
1832+
"status": "success",
1833+
"data": {"resultType": "vector"},
1834+
}
1835+
mock_resp.raise_for_status = MagicMock()
1836+
mock_get.return_value = mock_resp
1837+
1838+
client = self._make_test_client()
1839+
resp = client.get("/system-metrics/query", params={"query": "up"})
1840+
assert resp.status_code == 200
1841+
assert resp.json()["status"] == "success"
1842+
1843+
@patch("feast.api.registry.rest.system_metrics.http_requests.get")
1844+
def test_promql_instant_connection_error(self, mock_get):
1845+
import requests
1846+
1847+
mock_get.side_effect = requests.exceptions.ConnectionError("refused")
1848+
1849+
client = self._make_test_client()
1850+
resp = client.get("/system-metrics/query", params={"query": "up"})
1851+
assert resp.status_code == 503
1852+
assert "Failed to connect" in resp.json()["detail"]
1853+
1854+
@patch("feast.api.registry.rest.system_metrics.http_requests.get")
1855+
def test_promql_instant_timeout(self, mock_get):
1856+
import requests
1857+
1858+
mock_get.side_effect = requests.exceptions.Timeout("timed out")
1859+
1860+
client = self._make_test_client()
1861+
resp = client.get("/system-metrics/query", params={"query": "up"})
1862+
assert resp.status_code == 504
1863+
assert "timed out" in resp.json()["detail"]
1864+
1865+
@patch("feast.api.registry.rest.system_metrics.http_requests.get")
1866+
def test_promql_range_success(self, mock_get):
1867+
mock_resp = MagicMock()
1868+
mock_resp.json.return_value = {"status": "success"}
1869+
mock_resp.raise_for_status = MagicMock()
1870+
mock_get.return_value = mock_resp
1871+
1872+
client = self._make_test_client()
1873+
resp = client.get(
1874+
"/system-metrics/query_range",
1875+
params={
1876+
"query": "up",
1877+
"start": "2026-01-01T00:00:00Z",
1878+
"end": "2026-01-02T00:00:00Z",
1879+
},
1880+
)
1881+
assert resp.status_code == 200
1882+
1883+
@patch("feast.api.registry.rest.system_metrics.http_requests.get")
1884+
def test_scrape_success(self, mock_get):
1885+
mock_resp = MagicMock()
1886+
mock_resp.status_code = 200
1887+
mock_resp.text = "# TYPE up gauge\nup 1\n"
1888+
mock_resp.raise_for_status = MagicMock()
1889+
mock_get.return_value = mock_resp
1890+
1891+
client = self._make_test_client()
1892+
resp = client.get("/system-metrics/scrape")
1893+
assert resp.status_code == 200
1894+
body = resp.json()
1895+
assert "up" in body
1896+
1897+
@patch("feast.api.registry.rest.system_metrics.http_requests.get")
1898+
def test_scrape_failure(self, mock_get):
1899+
import requests
1900+
1901+
mock_get.side_effect = requests.exceptions.ConnectionError("refused")
1902+
1903+
client = self._make_test_client()
1904+
resp = client.get("/system-metrics/scrape")
1905+
assert resp.status_code == 503
1906+
assert "Failed to scrape" in resp.json()["detail"]
1907+
1908+
def test_prometheus_url_from_store(self):
1909+
from feast.api.registry.rest.system_metrics import get_system_metrics_router
1910+
1911+
store = MagicMock()
1912+
store.config.feature_server.metrics.prometheus_url = "http://custom:9090"
1913+
1914+
router = get_system_metrics_router(grpc_handler=None, store=store)
1915+
assert router is not None

0 commit comments

Comments
 (0)