Skip to content

Stop emitting metrics the MCP dashboard no longer queries#76

Merged
ChiragAgg5k merged 2 commits into
mainfrom
cleanup/unused-metrics
Jul 9, 2026
Merged

Stop emitting metrics the MCP dashboard no longer queries#76
ChiragAgg5k merged 2 commits into
mainfrom
cleanup/unused-metrics

Conversation

@ChiragAgg5k

Copy link
Copy Markdown
Member

The MCP Grafana dashboard (appwrite-labs/dashboards mcp.json) was curated down to six rows — Overview, Protocol & Messages, Tool Execution, Agentic & Token Metrics, Sessions & Connections, System Resources. This drops every instrument whose panels are gone, so we stop exporting series nothing reads:

Removed (with their record helpers, call sites, and tests):

  • mcp_connection_total — Transport row removed; mcp_handshake_total{status="success"} remains the connection signal, so the initialize path now records handshakes only
  • mcp_rate_limit_total / mcp_rate_limit_active — Rate Limiting row removed
  • mcp_resource_accessed/errors/size/access_anomaly — Resource Access row removed
  • http_requests_total / http_request_duration_seconds + HTTPMetricsMiddleware — HTTP Layer row removed
  • Legacy Appwrite-specific metrics that were never on the new dashboard: mcp_appwrite_calls/call_duration/errors, mcp_write_confirmations, mcp_search_tools_*, mcp_search_docs_*, mcp_context_requests, mcp_auth_validations/duration, mcp_uploads/upload_bytes/upload_errors

Kept (everything mcp.json queries): tool calls/duration/errors/in-flight/result size, token usage (+per-call), handshakes, active sessions (+by client), session duration/disconnects, messages/latency/size, JSON-RPC errors, protocol versions, CPU/memory, mcp_server_info (dashboard variables).

The telemetry module docstring now states the contract: only metrics queried by the hosted dashboard are emitted — remove a panel there, drop its instrument here.

Behavior otherwise unchanged: error monitoring (Sentry) paths, write-confirmation gating, and the identity middleware are untouched.

Testing: ruff/black/pyright clean, 160 unit tests pass, Docker build passes, and the ASGI app smoke-boots (healthz 200, unauthenticated /mcp 401).

The hosted MCP Grafana dashboard was curated down to six rows; drop every
instrument whose panels were removed so we stop paying for unused series:

- mcp_connection_total (Transport row removed; mcp_handshake_total remains
  the connection signal, so record_connection now emits handshakes only)
- mcp_rate_limit_total / mcp_rate_limit_active (Rate Limiting row removed)
- mcp_resource_* accessed/errors/size/anomaly (Resource Access row removed)
- http_requests_total / http_request_duration_seconds and the
  HTTPMetricsMiddleware (HTTP Layer row removed)
- legacy Appwrite-specific metrics never on the dashboard: appwrite
  calls/duration/errors, write confirmations, search_tools/search_docs,
  context requests, auth validations/duration, uploads

Kept: tool execution, token usage, handshakes, activity sessions,
messages/JSON-RPC errors, protocol versions, CPU/memory, server info.
The telemetry docstring now states the contract: only metrics queried by
appwrite-labs/dashboards mcp.json are emitted.
@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR prunes the telemetry layer to match the curated Grafana dashboard, removing every metric instrument whose panel was dropped and establishing a client-name allowlist to bound client_id label cardinality. All removed call sites, record helpers, and tests are cleaned up consistently across the module.

  • Metrics removed: mcp_connection_total, rate-limit gauges, resource-access counters, the full HTTP-layer middleware, and ~15 Appwrite-specific instruments that were never on the hosted dashboard — telemetry.py shrinks by ~150 lines with no retained references.
  • Cardinality hardening added: _normalize_client_name now checks against a fixed KNOWN_MCP_CLIENTS allowlist (sorted longest-first for greedy prefix matching) and collapses anything unrecognized to "other", preventing client-controlled input from minting unbounded label values.
  • Behavior preserved: SSRF validation, Sentry error capture, write-confirmation gating, and the record_handshake_failure path in RequireBearer are all untouched.

Confidence Score: 5/5

Safe to merge — removes unused instrumentation with no changes to request handling, auth, or business logic.

Every deleted metric has its call sites, record helpers, and tests removed consistently. The one new behavior — collapsing unknown client names to "other" — is additive cardinality protection and is well-covered by the new allowlist tests. No correctness-affecting paths were changed.

No files require special attention.

Important Files Changed

Filename Overview
src/mcp_server_appwrite/telemetry.py Removes ~150 lines of unused metric instruments and record helpers; adds allowlist-based client name normalization to bound client_id label cardinality.
src/mcp_server_appwrite/constants.py Adds KNOWN_MCP_CLIENTS allowlist tuple; purely additive, no logic changes.
src/mcp_server_appwrite/auth.py Removes all telemetry.record_auth(...) call sites and the import telemetry / timing boilerplate; auth logic itself is unchanged.
src/mcp_server_appwrite/http_app.py Removes HTTPMetricsMiddleware class and its registration; simplifies the unauthenticated-request branch; drops unused import time; record_handshake_failure call is correctly retained.
src/mcp_server_appwrite/operator.py Drops telemetry for context requests, search-tools queries, and write confirmations; simplifies the write-guard conditional — logic is equivalent.
src/mcp_server_appwrite/server.py Removes record_appwrite_call, record_upload*, and record_resource_access call sites; Sentry error-capture paths and SSRF validation logic are untouched.
src/mcp_server_appwrite/docs_search.py Drops record_search_docs calls; renames unused embedding_duration_s return value to _embedding_duration_s; search logic is unchanged.
tests/unit/test_telemetry.py Removes tests for deleted helpers; adds new tests for allowlist enforcement (test_unknown_client_names_collapse_to_other, test_unknown_client_sessions_are_labeled_other); updated connection dedupe test aligns with handshake-only counting.
tests/unit/test_http_app.py Updates MCPIdentityMiddleware test to assert on mcp.handshake (not mcp.connection); assertions are correct and complete.

Reviews (2): Last reviewed commit: "(fix): bound client_id label values to a..." | Re-trigger Greptile

client_id comes straight from client-controlled input (clientInfo.name /
User-Agent), so any caller could mint arbitrary label values and blow up
series cardinality on the Active Sessions by Client panel. Normalize names
against KNOWN_MCP_CLIENTS (constants.py) — exact or '<entry>-…' prefixed,
longest entry first — and collapse everything else to 'other'.
@ChiragAgg5k

Copy link
Copy Markdown
Member Author

Added a second commit: client_id label values are now bounded by a KNOWN_MCP_CLIENTS allowlist in constants.py (exact or prefixed match, longest first) — unrecognized client names collapse to other, so a caller can't mint arbitrary series on the Active Sessions by Client panel.

@ChiragAgg5k ChiragAgg5k merged commit 501727c into main Jul 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant