feat: surface MCP server status in System Status page
Add per-tool in-memory call counters to the MCP module and a GET /api/v1/status/mcp endpoint that joins them with the live tool registry and endpoint config, then render it as a new card on the System Status page with a one-click client-config copy button. - app/mcp/stats.py: lock-guarded MCPStatsTracker (the mcp SDK runs sync tool bodies via anyio.to_thread.run_sync, so this is genuinely multi-threaded, unlike the async REST routes) - app/mcp/server.py: instrument search_regulations, add get_mcp_status() - app/config/settings.py: optional MCP_PUBLIC_URL override, required because the Vite proxy and reverse proxies rewrite the Host header - StatusPage.tsx: MCP Server card, joins the existing parallel fetch Counters are process-local by design; token usage is already persisted by ModelUsageTracker since MCP calls route through ask(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -4,10 +4,11 @@ import asyncio
|
||||
import time
|
||||
from typing import Any
|
||||
|
||||
from fastapi import APIRouter
|
||||
from fastapi import APIRouter, Request
|
||||
|
||||
from app.config.settings import settings
|
||||
from app.domain.retrieval import RetrievedChunk
|
||||
from app.mcp.server import get_mcp_status
|
||||
from app.services.llm.llm_factory import get_llm_client, get_llm_factory
|
||||
from app.shared.bootstrap import (
|
||||
get_bm25_retriever,
|
||||
@@ -280,3 +281,17 @@ async def ping_model_connections():
|
||||
]
|
||||
await asyncio.gather(*tasks, return_exceptions=True)
|
||||
return {"models": [_build_model_status(role) for role in _MODEL_ROLES]}
|
||||
|
||||
|
||||
@router.get("/mcp")
|
||||
async def get_mcp_server_status(request: Request):
|
||||
"""Return MCP endpoint config, advertised tools, and per-tool call counters.
|
||||
|
||||
This route is a thin HTTP adapter: everything MCP-specific is assembled by
|
||||
app.mcp.server.get_mcp_status(). The only thing decided here is the public
|
||||
URL, because only the HTTP layer knows how the client reached us.
|
||||
"""
|
||||
# request.base_url already carries scheme/host/port and a trailing slash;
|
||||
# strip it before appending so the result is ".../mcp/", not ".../mcp//".
|
||||
public_url = settings.mcp_public_url or f"{str(request.base_url).rstrip('/')}/mcp/"
|
||||
return await get_mcp_status(public_url)
|
||||
|
||||
@@ -212,6 +212,18 @@ class Settings(BaseSettings):
|
||||
),
|
||||
)
|
||||
|
||||
# Optional override for the URL shown on the System Status page and copied
|
||||
# into client configs. Needed because request.base_url reflects the Host
|
||||
# header, which the Vite dev proxy (changeOrigin: true) and reverse proxies
|
||||
# that do not forward the original Host both rewrite.
|
||||
mcp_public_url: str = Field(
|
||||
default="",
|
||||
description=(
|
||||
"Externally reachable MCP endpoint URL, e.g. http://6.86.80.9:8000/mcp/. "
|
||||
"Leave empty to derive it from the incoming request."
|
||||
),
|
||||
)
|
||||
|
||||
@lru_cache
|
||||
def get_settings() -> Settings:
|
||||
"""Return settings."""
|
||||
|
||||
@@ -9,6 +9,7 @@ dict. No new retrieval, ranking, or LLM orchestration logic lives here.
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import time
|
||||
from typing import Annotated
|
||||
|
||||
from mcp.server import MCPServer
|
||||
@@ -18,6 +19,7 @@ from starlette.responses import PlainTextResponse
|
||||
from starlette.types import ASGIApp, Receive, Scope, Send
|
||||
|
||||
from app.config.settings import settings
|
||||
from app.mcp.stats import get_mcp_stats_tracker
|
||||
from app.shared.bootstrap import get_agent_conversation_service, get_jwt_handler
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -50,7 +52,24 @@ def search_regulations(
|
||||
# No session_id is passed: this keeps each call stateless (no
|
||||
# ConversationStore reads/writes), matching "search" semantics rather
|
||||
# than multi-turn chat semantics.
|
||||
_, result = get_agent_conversation_service().ask(query=query, top_k=top_k)
|
||||
started = time.perf_counter()
|
||||
try:
|
||||
_, result = get_agent_conversation_service().ask(query=query, top_k=top_k)
|
||||
except Exception:
|
||||
# Record the failure, then re-raise unchanged so the MCP SDK still
|
||||
# converts it into a protocol-level error for the client. Swallowing
|
||||
# it here would report success to the caller.
|
||||
get_mcp_stats_tracker().record(
|
||||
tool="search_regulations",
|
||||
duration_ms=(time.perf_counter() - started) * 1000,
|
||||
success=False,
|
||||
)
|
||||
raise
|
||||
get_mcp_stats_tracker().record(
|
||||
tool="search_regulations",
|
||||
duration_ms=(time.perf_counter() - started) * 1000,
|
||||
success=True,
|
||||
)
|
||||
return {
|
||||
"answer": result.answer,
|
||||
"sources": [source.__dict__ for source in result.sources],
|
||||
@@ -99,6 +118,13 @@ class MCPAuthMiddleware:
|
||||
await self.app(scope, receive, send)
|
||||
|
||||
|
||||
def _parse_allowed_hosts() -> list[str]:
|
||||
"""Split the configured MCP host allow-list into individual entries."""
|
||||
# Shared by the transport-security builder and the status endpoint so the
|
||||
# panel can never display an allow-list different from the enforced one.
|
||||
return [h.strip() for h in settings.mcp_allowed_hosts.split(",") if h.strip()]
|
||||
|
||||
|
||||
def _build_transport_security() -> TransportSecuritySettings:
|
||||
"""Translate the configured MCP host allow-list into SDK transport settings.
|
||||
|
||||
@@ -106,7 +132,7 @@ def _build_transport_security() -> TransportSecuritySettings:
|
||||
defaults to 127.0.0.1 and therefore rejects every remote client with HTTP
|
||||
421 — fatal for a remotely deployed backend.
|
||||
"""
|
||||
allowed = [h.strip() for h in settings.mcp_allowed_hosts.split(",") if h.strip()]
|
||||
allowed = _parse_allowed_hosts()
|
||||
if "*" in allowed:
|
||||
# Explicit, logged opt-out. Kept as an escape hatch for environments
|
||||
# behind a proxy that rewrites Host unpredictably, but never the default.
|
||||
@@ -140,3 +166,36 @@ def build_mcp_asgi_app() -> ASGIApp:
|
||||
)
|
||||
asgi_app.add_middleware(MCPAuthMiddleware)
|
||||
return asgi_app
|
||||
|
||||
|
||||
async def get_mcp_status(public_url: str) -> dict:
|
||||
"""Assemble the MCP status payload shown on the System Status page.
|
||||
|
||||
Owned by this module rather than the status route so that MCP internals
|
||||
(the tool registry, the allow-list format, the stats tracker) stay behind
|
||||
one boundary; the route only supplies public_url, which is the one value
|
||||
only the HTTP layer can know.
|
||||
"""
|
||||
stats = get_mcp_stats_tracker().snapshot()
|
||||
# list_tools() reads the in-memory registry populated by @mcp.tool() at
|
||||
# import time, so the panel always reflects what is actually advertised
|
||||
# rather than a hand-maintained duplicate list.
|
||||
tools = await mcp.list_tools()
|
||||
return {
|
||||
"endpoint_url": public_url,
|
||||
"auth_required": settings.auth_enabled,
|
||||
"allowed_hosts": _parse_allowed_hosts(),
|
||||
"tools": [
|
||||
{
|
||||
"name": tool.name,
|
||||
"description": (tool.description or "").strip().split("\n")[0],
|
||||
"calls": entry.calls if entry else 0,
|
||||
"errors": entry.errors if entry else 0,
|
||||
"avg_duration_ms": entry.avg_duration_ms if entry else None,
|
||||
"last_called_at": (
|
||||
entry.last_called_at.isoformat() if entry and entry.last_called_at else None
|
||||
),
|
||||
}
|
||||
for tool, entry in ((tool, stats.get(tool.name)) for tool in tools)
|
||||
],
|
||||
}
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
"""In-memory per-tool call counters for the MCP server.
|
||||
|
||||
Lives in `app/mcp/` rather than `app/shared/` because these counters are
|
||||
meaningful only for the MCP transport: they answer "is anything actually
|
||||
calling our MCP endpoint, and does it work?" for the System Status page.
|
||||
Token consumption is deliberately not tracked here — MCP tool calls route
|
||||
through AgentConversationService.ask() like every other caller, so the
|
||||
existing ModelUsageTracker already accounts for it.
|
||||
|
||||
Counters are process-local and reset on restart. That is an accepted
|
||||
tradeoff, recorded in the design spec: nothing billable depends on them.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
from dataclasses import dataclass
|
||||
from datetime import datetime, timezone
|
||||
from functools import lru_cache
|
||||
|
||||
from loguru import logger
|
||||
|
||||
|
||||
@dataclass
|
||||
class MCPToolStats:
|
||||
"""Accumulated call outcomes for a single MCP tool."""
|
||||
|
||||
calls: int = 0
|
||||
errors: int = 0
|
||||
total_duration_ms: float = 0.0
|
||||
last_called_at: datetime | None = None
|
||||
|
||||
@property
|
||||
def avg_duration_ms(self) -> float | None:
|
||||
"""Mean call duration, or None when the tool has never been called.
|
||||
|
||||
Returning None rather than 0.0 keeps "never called" distinguishable
|
||||
from "called, but instantaneous" in the status UI.
|
||||
"""
|
||||
if self.calls == 0:
|
||||
return None
|
||||
return self.total_duration_ms / self.calls
|
||||
|
||||
|
||||
class MCPStatsTracker:
|
||||
"""Thread-safe registry of per-tool MCP call statistics.
|
||||
|
||||
The lock is load-bearing, not defensive habit: the mcp SDK dispatches
|
||||
synchronous tool functions through anyio.to_thread.run_sync, so tool
|
||||
bodies genuinely run on multiple worker threads at once — unlike the
|
||||
async REST routes, which are serialized by the event loop.
|
||||
"""
|
||||
|
||||
def __init__(self) -> None:
|
||||
"""Initialize an empty registry guarded by a single lock."""
|
||||
self._tools: dict[str, MCPToolStats] = {}
|
||||
# One coarse lock is enough: record() runs once per MCP tool call and
|
||||
# snapshot() is only read by the low-traffic status endpoint.
|
||||
self._lock = threading.Lock()
|
||||
|
||||
def record(self, *, tool: str, duration_ms: float, success: bool) -> None:
|
||||
"""Record the outcome of one MCP tool invocation.
|
||||
|
||||
Never raises: a defect in observability code must not turn a working
|
||||
tool call into a protocol error for the client.
|
||||
"""
|
||||
try:
|
||||
# Coerce outside the lock so a bad argument cannot abort mid-update
|
||||
# and leave calls incremented but duration unaccounted for.
|
||||
duration = float(duration_ms)
|
||||
now = datetime.now(timezone.utc)
|
||||
with self._lock:
|
||||
stats = self._tools.setdefault(tool, MCPToolStats())
|
||||
stats.calls += 1
|
||||
if not success:
|
||||
stats.errors += 1
|
||||
stats.total_duration_ms += duration
|
||||
stats.last_called_at = now
|
||||
except Exception as exc: # noqa: BLE001 - tracking must never break a real call
|
||||
logger.warning("MCPStatsTracker.record failed for tool {} - {}", tool, exc)
|
||||
|
||||
def snapshot(self) -> dict[str, MCPToolStats]:
|
||||
"""Return a shallow copy of all tracked tools, safe to read outside the lock."""
|
||||
with self._lock:
|
||||
return dict(self._tools)
|
||||
|
||||
|
||||
@lru_cache
|
||||
def get_mcp_stats_tracker() -> MCPStatsTracker:
|
||||
"""Return the process-wide singleton tracker (mirrors get_model_usage_tracker())."""
|
||||
return MCPStatsTracker()
|
||||
@@ -0,0 +1,100 @@
|
||||
"""Tests for the in-memory MCP per-tool statistics tracker.
|
||||
|
||||
These pin the two properties the status panel depends on: counters stay exact
|
||||
under the concurrent thread dispatch the mcp SDK uses, and recording never
|
||||
raises into a live tool call.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
|
||||
from app.mcp.stats import MCPStatsTracker, MCPToolStats, get_mcp_stats_tracker
|
||||
|
||||
|
||||
def test_avg_duration_is_none_before_any_call():
|
||||
"""A never-called tool reports None, not 0.0, so the UI can distinguish them."""
|
||||
assert MCPToolStats().avg_duration_ms is None
|
||||
|
||||
|
||||
def test_avg_duration_is_the_mean_of_recorded_durations():
|
||||
"""Average is computed over all calls, successful or not."""
|
||||
tracker = MCPStatsTracker()
|
||||
for duration in (100.0, 200.0, 300.0):
|
||||
tracker.record(tool="search_regulations", duration_ms=duration, success=True)
|
||||
|
||||
stats = tracker.snapshot()["search_regulations"]
|
||||
assert stats.calls == 3
|
||||
assert stats.avg_duration_ms == 200.0
|
||||
|
||||
|
||||
def test_failures_increment_both_calls_and_errors():
|
||||
"""errors is a subset of calls, so the UI can show "2 of 3 failed" honestly."""
|
||||
tracker = MCPStatsTracker()
|
||||
tracker.record(tool="t", duration_ms=1.0, success=True)
|
||||
tracker.record(tool="t", duration_ms=1.0, success=False)
|
||||
tracker.record(tool="t", duration_ms=1.0, success=False)
|
||||
|
||||
stats = tracker.snapshot()["t"]
|
||||
assert stats.calls == 3
|
||||
assert stats.errors == 2
|
||||
|
||||
|
||||
def test_last_called_at_is_set_and_timezone_aware():
|
||||
"""The panel renders this as a local time, which requires an aware datetime."""
|
||||
tracker = MCPStatsTracker()
|
||||
tracker.record(tool="t", duration_ms=1.0, success=True)
|
||||
|
||||
last_called = tracker.snapshot()["t"].last_called_at
|
||||
assert last_called is not None
|
||||
assert last_called.tzinfo is not None
|
||||
|
||||
|
||||
def test_concurrent_record_calls_are_not_lost():
|
||||
"""8 threads x 100 calls must total exactly 800.
|
||||
|
||||
Without the lock this loses increments non-deterministically. The mcp SDK
|
||||
runs synchronous tool bodies via anyio.to_thread.run_sync, so this is the
|
||||
real dispatch model, not a hypothetical.
|
||||
"""
|
||||
tracker = MCPStatsTracker()
|
||||
|
||||
def hammer() -> None:
|
||||
for _ in range(100):
|
||||
tracker.record(tool="search_regulations", duration_ms=1.0, success=True)
|
||||
|
||||
threads = [threading.Thread(target=hammer) for _ in range(8)]
|
||||
for thread in threads:
|
||||
thread.start()
|
||||
for thread in threads:
|
||||
thread.join()
|
||||
|
||||
stats = tracker.snapshot()["search_regulations"]
|
||||
assert stats.calls == 800
|
||||
assert stats.total_duration_ms == 800.0
|
||||
|
||||
|
||||
def test_record_swallows_bad_input_instead_of_raising():
|
||||
"""A malformed duration must not propagate into the caller's tool call."""
|
||||
tracker = MCPStatsTracker()
|
||||
tracker.record(tool="t", duration_ms="not-a-number", success=True) # type: ignore[arg-type]
|
||||
|
||||
# Coercion happens before the lock is taken, so the entry is never created
|
||||
# in a half-updated state.
|
||||
assert tracker.snapshot() == {}
|
||||
|
||||
|
||||
def test_snapshot_is_a_copy_not_the_live_dict():
|
||||
"""Callers mutating the snapshot must not corrupt the tracker."""
|
||||
tracker = MCPStatsTracker()
|
||||
tracker.record(tool="t", duration_ms=1.0, success=True)
|
||||
|
||||
snapshot = tracker.snapshot()
|
||||
snapshot.clear()
|
||||
|
||||
assert "t" in tracker.snapshot()
|
||||
|
||||
|
||||
def test_get_mcp_stats_tracker_returns_a_singleton():
|
||||
"""Instrumentation and the status route must observe the same counters."""
|
||||
assert get_mcp_stats_tracker() is get_mcp_stats_tracker()
|
||||
@@ -0,0 +1,79 @@
|
||||
"""Tests for get_mcp_status(), the payload behind the System Status MCP card.
|
||||
|
||||
Covers the join between the live tool registry and the stats tracker, plus
|
||||
the two values the route supplies or the settings decide.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from unittest.mock import patch
|
||||
|
||||
from app.mcp.stats import MCPStatsTracker
|
||||
|
||||
|
||||
def _status(tracker: MCPStatsTracker | None = None, **setting_overrides) -> dict:
|
||||
"""Call get_mcp_status() with an isolated tracker and patched settings.
|
||||
|
||||
The real tracker is a process-wide singleton, so tests must inject their
|
||||
own instance or they leak counters into each other.
|
||||
"""
|
||||
from app.mcp.server import get_mcp_status, settings
|
||||
|
||||
patched = settings.model_copy(update=setting_overrides)
|
||||
with (
|
||||
patch("app.mcp.server.settings", patched),
|
||||
patch("app.mcp.server.get_mcp_stats_tracker", return_value=tracker or MCPStatsTracker()),
|
||||
):
|
||||
return asyncio.run(get_mcp_status("http://6.86.80.9:8000/mcp/"))
|
||||
|
||||
|
||||
def test_public_url_is_passed_through_unmodified():
|
||||
"""The route owns URL resolution; get_mcp_status() must not rewrite it."""
|
||||
assert _status()["endpoint_url"] == "http://6.86.80.9:8000/mcp/"
|
||||
|
||||
|
||||
def test_auth_required_follows_settings():
|
||||
"""The panel's auth badge must reflect live config, not a hard-coded value."""
|
||||
assert _status(auth_enabled=True)["auth_required"] is True
|
||||
assert _status(auth_enabled=False)["auth_required"] is False
|
||||
|
||||
|
||||
def test_allowed_hosts_are_split_and_stripped():
|
||||
"""Displayed allow-list must match the one the transport actually enforces."""
|
||||
status = _status(mcp_allowed_hosts="6.86.80.9:* , 127.0.0.1:*,")
|
||||
assert status["allowed_hosts"] == ["6.86.80.9:*", "127.0.0.1:*"]
|
||||
|
||||
|
||||
def test_tools_come_from_the_live_registry_with_zeroed_stats():
|
||||
"""An advertised but never-called tool reports zeros, not absence."""
|
||||
tools = {tool["name"]: tool for tool in _status()["tools"]}
|
||||
|
||||
assert "search_regulations" in tools
|
||||
assert tools["search_regulations"]["calls"] == 0
|
||||
assert tools["search_regulations"]["errors"] == 0
|
||||
assert tools["search_regulations"]["avg_duration_ms"] is None
|
||||
assert tools["search_regulations"]["last_called_at"] is None
|
||||
|
||||
|
||||
def test_recorded_stats_are_joined_onto_the_matching_tool():
|
||||
"""Counters recorded by the instrumented tool must surface on that tool's row."""
|
||||
tracker = MCPStatsTracker()
|
||||
tracker.record(tool="search_regulations", duration_ms=120.0, success=True)
|
||||
tracker.record(tool="search_regulations", duration_ms=80.0, success=False)
|
||||
|
||||
tool = next(t for t in _status(tracker)["tools"] if t["name"] == "search_regulations")
|
||||
|
||||
assert tool["calls"] == 2
|
||||
assert tool["errors"] == 1
|
||||
assert tool["avg_duration_ms"] == 100.0
|
||||
# Serialized for JSON transport; the frontend parses it with new Date().
|
||||
assert isinstance(tool["last_called_at"], str)
|
||||
|
||||
|
||||
def test_description_is_the_first_docstring_line():
|
||||
"""Multi-line tool docstrings must not blow up the card's row height."""
|
||||
tool = next(t for t in _status()["tools"] if t["name"] == "search_regulations")
|
||||
|
||||
assert "\n" not in tool["description"]
|
||||
assert tool["description"].startswith("Search the compliance knowledge base")
|
||||
Reference in New Issue
Block a user