Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins - #5956
Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins#5956glageju wants to merge 22 commits into
Conversation
The D8 ownership labels stacklok.component and stacklok.product are meant to be frozen per-series identity, but they were applied to the OTEL resource before CustomAttributes and WithFromEnv. Because resource detectors merge last-wins, a CLI custom attribute or OTEL_RESOURCE_ATTRIBUTES entry for either key silently overrode the frozen values, letting stacklok_component and stacklok_product drift in the Prometheus output. Apply the two reserved attributes as the final resource detector so they always win over user- and env-supplied attributes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5956 +/- ##
==========================================
+ Coverage 72.24% 72.28% +0.03%
==========================================
Files 722 722
Lines 75125 75152 +27
==========================================
+ Hits 54277 54321 +44
+ Misses 16985 16981 -4
+ Partials 3863 3850 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Multiple review passes on the metrics-standardization migration (proxy + vMCP -> stacklok.* semconv vocabulary) surfaced gaps left by the cutover: - Backend health gauge leaked series for backends removed from the registry (e.g. via list_changed) since it tracked membership in an internal map that only grew. MonitorBackends now takes the BackendRegistry directly and re-derives live backends from registry.List on every collection. - The health gauge collapsed BackendHealthStatus's five states into a boolean, losing degraded/unknown/unauthenticated information the registry already carries before any request completes. The gauge now emits one point per real status value. - A construction failure in core.New occurring after MonitorBackends registered its gauge callback (audit config, workflow validation, health monitor setup) never unregistered it, leaking the callback against the shared meter provider for every failed New attempt. - Docs (observability.md, virtualmcpserver-observability.md, telemetry-migration-guide.md) and four Grafana example dashboards still referenced deleted/renamed legacy metric and label names. - Minor cleanup: an unnamed "not_found" outcome string, inconsistent receiver types on telemetryBackendClient, and a discarded metric registration handle. Adds unit test coverage for the health gauge's registry-driven membership, its multi-state reporting, and a regression test proving core.New unregisters the callback on every post-registration failure path (verified by temporarily reverting the fix and confirming the new test fails). Generated with [Claude Code](https://claude.com/claude-code)
The metrics-standardization migration lost information the deleted legacy metrics carried and left downstream artifacts partially migrated: mcp.client.operation.duration had no backend identity label, three Grafana dashboards still queried deleted metric names, and the new stacklok.vmcp.mcp_server.health gauge never consulted the live health monitor, so it could disagree with capability filtering. - Add mcp_server to mcp.client.operation.duration's attributes - Migrate remaining stale panels in 3 Grafana dashboards to the semconv metric/label vocabulary - Thread a live health.StatusProvider into the backend-health gauge via a new HealthProviderSetter, matching filterHealthyBackends' precedence (live provider > recorded outcome > registry snapshot), and normalize an empty HealthStatus to healthy instead of reporting every state as 0 - Cover all 5 of New()'s post-registration error paths for backend health callback unregistration, not just one - Hoist the duplicated componentName constant into providers.ComponentName - Fix Close()'s stale doc comment and rename unregisterHealthOnError to unregisterHealthLogged (it also runs on the normal shutdown path) - Document that NewMeteredTokenCache has no production caller yet - Add the vMCP optimizer and mcp_server.health rows to the migration guide's metric mapping table Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
error_type on the semconv metrics is only set for HTTP >=500, so the dashboard error-rate panels' error_type!="" filter silently excluded 4xx failures (authz denials, classification rejects) the old status!="success" caught. Switch those panels to the HTTP duration metric's status-code label, which sees every request. The migration guide's request-volume guidance summed mcp_server_operation_duration_seconds_count and http_server_request_duration_seconds_count, but every MCP-method-bearing request increments both, double-counting them. Use the HTTP duration metric alone for total volume. Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Grafana total-RPS panels queried the MCP-only histogram, undercounting transport-level traffic; the migration guide pointed at a label that doesn't exist on its replacement metric; several D8/build_info/health-gauge doc sections contradicted the actual implementation; and the proxy middleware could panic on a rare instrument-registration failure. Also drops the token-cache metrics decorator shipped with no production caller.
The guide's phrasing implied one unified dual-emission strategy covered the whole migration. Metrics and span attributes actually follow different policies: spans dual-emit behind a flag, metrics hard-cut with no fallback. State that distinction and its rationale up front instead of only in a later "Important" aside. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rate-limit metrics were missed by the earlier proxy/vMCP rename pass despite already importing toolhive-core's label constants and bucket presets. Complete the migration for this package: toolhive_rate_limit_* becomes stacklok.toolhive.ratelimit.*, and the guide's mapping table gains the corresponding rows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes documentation and test-coverage gaps surfaced during review of the stacklok.* metrics migration: the new mcp_server label on mcp.client.operation.duration was undocumented in two places, the revision-reclassification rename was missing from the migration guide's mapping table, the health-gauge's doc comment described the wrong divergence condition from filterHealthyBackends, the Grafana sidecar's cluster-wide searchNamespace had no RBAC note or setup steps, and the D8 ownership-label wiring in providers_strategy.go had no test through its actual construction path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
glageju
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the metrics-vocabulary migration end to end: the proxy/vMCP rename to stacklok.*, the six-metric deletion, D8 ownership-label hardening, and the vMCP backend health-gauge rework.
Findings addressed in 045ccad
mcp_serverlabel missing frommcp.client.operation.duration's documentation in two places (docs/operator/virtualmcpserver-observability.md,docs/telemetry-migration-guide.md)currentHealthStatus's doc comment (pkg/vmcp/internal/backendtelemetry/backendtelemetry.go) described the wrong divergence condition fromfilterHealthyBackends— corrected to state it occurs whenprovider == nilandrecord()has an entry, not the "provider set but not yet tracking" case it originally claimed- Grafana sidecar
searchNamespace: ALL(examples/otel/prometheus-stack-values.yaml) had no RBAC note or ConfigMap setup steps — scoped to themonitoringnamespace with a comment on the tradeoff, and added thekubectl create configmap/labelsteps toexamples/otel/README.md - D8 ownership-label wiring in
providers_strategy.gohad no test through its actualUnifiedMeterStrategy.CreateMeterProviderconstruction path — addedTestUnifiedMeterStrategy_PrometheusOwnershipLabels toolhive_vmcp_backend_revision_reclassifications's rename was missing from the migration guide's mapping table
Verified, not bugs
- Using OTel dotted attribute-key constants (
stacklok.component) as Prometheus label-map keys inproviders_strategy.godoes not panicMustRegisteras initially suspected —prometheus/common'sUTF8Validationdefault (active in this repo's pinnedv0.67.5) accepts dots and escapes them to underscore form at scrape time. Confirmed by reproducing the exact registration + scrape against this repo's pinned dependencies. - The
toolhive-core v0.0.34pin ingo.modis not visible as a diff hunk because it already landed via an earlier merge frommain— the branch builds and compiles cleanly.
Overall
Callback lifecycle (register → unregister-on-every-construction-failure-path → idempotent Close()), the D8 label-override hardening, and the six metric deletions/renames are all correctly implemented and backed by tests that exercise the actual failure paths, not just the happy path — including a regression test that scrapes a live Prometheus handler to prove no gauge series leak after a construction failure.
Generated with Claude Code
Summary
The proxy and vMCP used
toolhive_mcp_*/toolhive_vmcp_*metric names, emitted six legacy metrics alongside their emerging-semconv replacements, and diverged from the platform label vocabulary. This finishes the semconv migration and adopts the sharedtoolhive-corevocabulary, per the Metrics Standardization RFC.pkg/telemetry/):toolhive_mcp_active_connections→stacklok.toolhive.proxy.active_connections;build_infoviacoremetrics.RegisterBuildInfo; addedhttp.server.request.duration(OTel HTTP semconv) so deleting the legacy MCP twins doesn't drop transport-level coverage; D8 ownership labels (stacklok_component=toolhive,stacklok_product=stacklok-platform) promoted to every series viaWithResourceAsConstantLabelsand hardened as the final OTEL resource detector so a CLI custom attribute orOTEL_RESOURCE_ATTRIBUTESentry for those keys cannot override the frozen values.pkg/vmcp/):toolhive_vmcp_workflow_*→stacklok.vmcp.composite_tool.*; backendmcp_server.healthgauge rename to a live per-(mcp_server, state)gauge, keyed by workload ID for consistency with the backend registry and health-status provider; optimizer renames +token_savings.toolhive_mcp_requests,toolhive_mcp_request_duration,toolhive_mcp_tool_calls,toolhive_vmcp_backend_requests,toolhive_vmcp_backend_errors,toolhive_vmcp_backend_requests_duration). Their semconv replacements (mcp.server.operation.duration,mcp.client.operation.duration,http.server.request.duration) are kept.toolhive-coredependency tov0.0.34, the release carryingtelemetry/metrics(adopts core label keys, outcome values, and bucket presets). The transitive AWS-SDK and grpc bumps ingo.modcome fromtoolhive-core's own requirements.test/integration/vmcp/,test/e2e/) that asserted the deleted/renamed metric names and old label scheme (mcp_method→mcp_method_name,server→mcp_server,status→HTTP status code) to assert the semconv replacements.docs/telemetry-migration-guide.mdfor the renamed/deleted metrics, including a complete old→new metric and label mapping and a correctedMCP Request Ratevs. total HTTP request-rate query split.examples/otel/prometheus-stack-values.yaml) so the updated example dashboards can be auto-provisioned into a local Kind cluster via a labeled ConfigMap, for validating the renamed metrics land correctly end-to-end.pkg/ratelimit/observability.go, missed by the original pass:toolhive_rate_limit_decisions/_redis_errors/_check_latency→stacklok.toolhive.ratelimit.{decisions,redis_errors,check_latency}, with the migration guide anddocs/observability.mdupdated to match.toolhive_vmcp_backend_revision_reclassifications(added independently onmainafter this branch forked) tostacklok.vmcp.backend.revision_reclassificationsso it doesn't reintroduce the legacy naming scheme this PR retires.docs/telemetry-migration-guide.md's Backward Compatibility section: span attributes and metrics follow different policies (dual-emission vs. hard cutover) for different reasons — the guide previously implied one uniform strategy.Type of change
Test plan
task test)task lint-fix)Verified the shipped standalone path (
GOWORK=off, publishedtoolhive-core v0.0.34, no workspace):GOWORK=off go build ./...— clean.GOWORK=off go test ./pkg/telemetry/... ./pkg/vmcp/...— all packages pass, including the D8 label-promotion assertion inpkg/telemetry/buildinfo_test.go(stacklok_component="toolhive"/stacklok_product="stacklok-platform"promoted to per-series labels) and the updatedTestVMCPServer_Telemetry_CompositeToolMetricsintegration test.task test(full suite) is green.test/e2e/) were updated for the new metric/label vocabulary and compile clean (go vet); they require a fulltask test-e2erun (containers) to exercise end-to-end.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Does this introduce a user-facing change?
Yes. Prometheus metric names emitted by the proxy, vMCP, and rate limiting change to the
stacklok.*namespace, and six legacytoolhive_*twins are removed. Their OTel-semconv replacements cover the same signals;docs/telemetry-migration-guide.mdmaps old → new. A customer scraping the old names loses those series (RFC D13 accepts this: the deleted twins are not scraped in a default shipped deployment).Special notes for reviewers
toolhive-corev0.0.34(the shared-vocabulary release); this PR'sgo.modpins it.toolhive-core v0.0.34trims theComponent*value roster a pre-release build referenced; toolhive defines its ownstacklok.component = "toolhive"constant (pkg/telemetry/middleware.go,pkg/telemetry/providers/providers.go), asserted bypkg/telemetry/buildinfo_test.go.pkg/vmcp/internal/backendtelemetry/backendtelemetry.go's health gauge resolves each backend's status with a three-tier precedence — a livehealth.StatusProviderfirst, then request-outcome tracking, then the registry's discovery-time snapshot — matching the same precedencefilterHealthyBackendsuses for capability aggregation.core.Newunregisters the gauge's callback on every construction-failure path after registration succeeds, and onClose(), so a partially-constructedcoreVMCPnever leaks a callback against the shared meter provider.Links
useLegacyAttributesspan-attribute dual-emission flag (out of scope here; metrics/labels in this PR use a hard cutover with no such flag).Ref: https://github.com/stacklok/stacklok-enterprise-platform/issues/1380
Generated with Claude Code