Dual-emit legacy metric names behind --otel-use-legacy-metrics - #6168
Open
glageju wants to merge 9 commits into
Open
Dual-emit legacy metric names behind --otel-use-legacy-metrics#6168glageju wants to merge 9 commits into
glageju wants to merge 9 commits into
Conversation
Renaming a metric empties a dashboard panel exactly as deleting it does, and the rename half of this migration shipped with no overlap window: the old name stopped and the new name started in the same release, so there was no moment where an operator could run both queries and confirm they agree before cutting over. The six deleted twins had that overlap by accident — their semconv replacements were already emitting alongside them before this branch. This restores the overlap deliberately, behind a flag defaulting to on: - toolhive_mcp_requests, toolhive_mcp_request_duration and toolhive_mcp_tool_calls are re-emitted under their original names and label vocabulary, including the pre-rename "any status >= 400 is an error" classification — an alias that reports different numbers than the metric it replaces is worse than no alias. - toolhive_mcp_active_connections is re-emitted under the old server/transport label keys alongside the renamed gauge. The flag mirrors --otel-use-legacy-attributes at every layer (CLI flag, file config as *bool, runner builder, telemetry.Config, operator CRD, spectoconfig mapping and its drift-test entry) and is passed explicitly rather than through package state, matching how every other --otel* setting reaches its consumers. Legacy instruments are no-ops when the flag is off, so record sites need no branch, and an instrument-creation failure also yields a no-op — a legacy alias must never be why a process fails to start. Tests pin both states: with the flag on, legacy and current names appear in the same scrape; with it off, the legacy names are absent while the current ones are unaffected. Addresses the cutover-strategy request in #5956 from @ChrisJBurns and @aponcedeleonch. Rate-limit, vMCP and optimizer renames are not yet dual-emitted.
The rate-limit renames break dashboards twice over: both the metric name and
its server label changed (toolhive_rate_limit_decisions{server=…} became
stacklok_toolhive_ratelimit_decisions_total{mcp_server=…}), so a panel grouping
by the old label returns nothing even if it finds the series.
All three rate-limit metrics are now re-emitted under their pre-rename names
with the pre-rename "server" label key, gated by the same
--otel-use-legacy-metrics flag as the proxy metrics. The flag reaches
pkg/ratelimit explicitly: through MiddlewareParams (which is JSON-serialized to
the proxyrunner) and the vMCP factory Config, sourced from TelemetryConfig at
both construction sites, rather than through package state.
TestRateLimitMetricNamesUseStacklokPrefix now constructs with the flag off, so
it still proves the rename is complete and additionally covers suppression; a
new test covers the enabled case and asserts the legacy series carries the old
label key.
Part of the cutover-strategy request in #5956.
Completes dual emission across the remaining renamed metrics — composite-tool executions/duration, the deleted backend request/error/duration twins, and the nine optimizer metrics — so every renamed or retired name has an overlap window, not just the proxy ones. Where counters were merged into an outcome label, the legacy aliases are re-emitted as the separate counters they were (workflow errors, optimizer find_tool/call_tool errors and not_found), and each alias keeps its original label vocabulary (workflow.name, tool_name, the target.* set) so an existing dashboard or alert reads its own series rather than a renamed one. Aliases reproduce the originals' semantics exactly, including counting attempts rather than completions where the originals did. The flag reaches these packages explicitly via Provider.UseLegacyMetrics() and a MonitorBackends/monitorOptimizer parameter, sourced from the Config the provider was built with so it cannot drift. telemetryComposer's legacy record calls are nil-checked: workflowInstruments is also built by struct literal in-package, where those fields are unset. The migration guide is reframed from a one-shot cutover to a deprecation schedule: the Backward Compatibility table no longer claims metrics are a hard cutover, Step 1 no longer promises dashboards keep working indefinitely, Step 4's side-by-side verification advice is now actually true for metrics, and Step 5 retires both flags. Two things dual emission cannot cover are called out — bucket boundary changes and the merged-counter semantics above. Addresses #5956: - HIGH docs/telemetry-migration-guide.md:213 (3685784090): Step 1 no longer states existing dashboards continue to work without changes; the window is framed as temporary with a removal release, and Step 4 is rescoped. Completes the cutover-strategy request from @ChrisJBurns and @aponcedeleonch.
Self-review of the preceding commits found one real defect and several comments that the changes had invalidated. thv serve built its telemetry.Config by struct literal without setting either legacy-emission field, so both fell to the Go zero value of false. The documented default is true, and the config fields are *bool precisely so "unset" is distinguishable from an explicit false — so dual emission was silently off on that path, with no error and no log line, just missing legacy series. UseLegacyAttributes had the same pre-existing gap; both are now resolved through boolOrDefaultTrue, with a test pinning the rule. Comments corrected where the code had moved out from under them: - backendtelemetry.go: two comments still said the recorded-health map is "never pruned" and that set() can only ever receive BackendHealthy/BackendUnhealthy. retain() prunes it and healthStatusForError can now yield BackendUnauthenticated. - middleware.go: recordHTTPServerDuration's attribute list omitted network.protocol.version, which it emits. - middleware.go: buildInfoOnce's comment covered the duplicate-callback case but not the inverse — with two distinct meter providers in one process only the first gets the gauge. Not reachable today (one telemetry middleware per proxy process, one provider in vMCP), now stated rather than left implicit. - observability.md: the gen_ai cardinality warning called the toolhive_mcp_* twins "deleted", contradicting the same document's useLegacyMetrics entry; reworded to describe when it becomes true. The http.server.request.duration attribute table gained url.scheme and network.protocol.version, and notes the _OTHER normalization. - limiter.go: NewLimiter hardcodes legacy emission on and takes no telemetry config; its doc now says so and points at NewRedisLimiter.
glageju
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
August 1, 2026 15:54
Six legacy aliases re-emitted their original metric name on
BucketsMCPProxy, but those names shipped on BucketsMCPSemconv -- the
pre-rename MCPHistogramBuckets was the semconv preset. Boundaries are not
dual-emitted: there is one series per name, so an alias that moves them
breaks the exact query the overlap window exists to protect.
An operator upgrading with --otel-use-legacy-metrics=true expects existing
dashboards to keep working unchanged. Instead le="0.02"/"0.2"/"2" stop
being written and le="0.25"/"2.5" start, so histogram_quantile() over a
range spanning the upgrade mixes two layouts and returns a plausible,
wrong number for the whole retention window -- no error, no gap in the
graph.
Affects toolhive_mcp_request_duration, _vmcp_backend_requests_duration,
_vmcp_workflow_duration, _vmcp_optimizer_{find,call}_tool_duration, and
_rate_limit_check_latency. The new names keep the presets they were
assigned; only the aliases move back.
# Conflicts: # pkg/telemetry/middleware.go
The PR-split planning notes under docs/plans/ were working material, not part of the change; they were picked up by an over-broad `git add` while resolving a merge conflict. Removed from version control and kept locally.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## gautam/metrics-std-toolhive #6168 +/- ##
===============================================================
+ Coverage 72.68% 72.69% +0.01%
===============================================================
Files 736 737 +1
Lines 76447 76650 +203
===============================================================
+ Hits 55563 55724 +161
- Misses 16951 16985 +34
- Partials 3933 3941 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#5956 renames ToolHive's metrics to the
stacklok.*vocabulary. @aponcedeleonch and @ChrisJBurns both blocked it on the same point: a rename empties a dashboard panel exactly the way a deletion does, and the renamed metrics have no overlap window — the old name stops and the new one starts in the same release, so there is no moment where a user can run both queries side by side and confirm they agree.Span attributes already have machinery for exactly this: dual emission behind
--otel-use-legacy-attributes, with #6072 tracking its retirement. Metrics went with a hard cutover instead, and the asymmetry wasn't justified. This PR closes it — metrics get the same treatment: a flag, a window, and a documented path.After this PR, upgrading #5956 breaks nothing by default. All 25 pre-rename metric names are still emitted, under their original label keys.
pkg/telemetry/legacymetrics.go): helpers that return a working instrument when enabled and a no-op otherwise, so the ~25 record sites stay branch-free instead of growing anifeach. An instrument-creation failure also yields a no-op — a legacy alias must never be the reason a process fails to start.UseLegacyMetricsend to end — CLI flag, config file, operator CRD,thv serve, and vMCP — defaulting totrueat every layer.toolhive_rate_limit_decisions{server=…}→stacklok_toolhive_ratelimit_decisions_total{mcp_server=…}), so those dashboards break twice over. An alias emitting the old name with the new label key would be a second breaking change, not a compatibility path — so the legacy attribute sets are built separately rather than shared.outcome-labelled counters. Standardize proxy + vMCP metrics to stacklok.*, delete legacy twins #5956 folds four_errors/_not_foundcounters into anoutcomelabel (RFC D4). The aliases fan back out to the original separate counters, andfind_tooldeliberately keeps its legacy counter incrementing before the call — the original counted attempts, not completions, so it's reproduced as-was rather than silently changing meaning during the overlap window.Type of change
Test plan
task test) — 199 packages, race detector ontask lint-fix) — 0 issuesVerified against a running proxy with
--otel-enable-prometheus-metrics-path:/metrics.--otel-use-legacy-metrics=false— zero^toolhive_series; new names still present.toolhive_rate_limit_decisions_totalstill carriesserver="…", notmcp_server="…". This is the check that would catch the "breaks twice over" regression, and an unanchored assertion can't catch it:server="x"is a substring ofmcp_server="x".call_toolincrements both the newoutcome="not_found"series and the legacy_not_foundcounter.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Adds one optional field,
MCPTelemetryOTelConfig.UseLegacyMetrics, with+kubebuilder:default=true. Purely additive — no field is removed or retyped, and existing CRs are unaffected.Does this introduce a user-facing change?
Yes, and it makes #5956's change non-breaking by default.
New flag
--otel-use-legacy-metrics(alsouse-legacy-metricsin config,spec.otel.useLegacyMetricson the CRD), defaulting totrue. While on, ToolHive emits both the newstacklok.*metric names and the pre-renametoolhive_*names with their original label keys, so existing dashboards and alerts keep working across the upgrade.Migrate at your own pace: run the old and new queries side by side, confirm they agree, then set the flag to
falseto drop the legacy series. The flag and the legacy names will be removed in a later minor — seedocs/telemetry-migration-guide.mdfor the old → new mapping and the schedule.Dual emission roughly doubles series count for the metrics involved while enabled; histograms are the expensive case.
Changes
pkg/telemetry/legacymetrics.gopkg/telemetry/config.goConfig.UseLegacyMetrics, default,(*Provider).UseLegacyMetrics()accessorpkg/telemetry/middleware.gopkg/telemetry/serve.gothv servecmd/thv/app/run_flags.go--otel-use-legacy-metrics;resolveOptionalBoolhelperpkg/config/config.goOpenTelemetryConfig.UseLegacyMetricspkg/runner/{telemetry_config,config_builder,middleware}.gotruepkg/ratelimit/{observability,limiter,middleware}.goserverlabelpkg/vmcp/core/core_telemetry.gopkg/vmcp/internal/backendtelemetry/backendtelemetry.gopkg/vmcp/server/sessionmanager/factory.gooutcomeun-mergepkg/vmcp/{cli/serve,ratelimit/factory/limiter}.gocmd/thv-operator/**,deploy/charts/**,docs/{cli,operator,server}/**docs/telemetry-migration-guide.md,docs/observability.mdSpecial notes for reviewers
On RFC D13 ("no dual-emit window"). I don't think this PR contradicts it, for two reasons worth checking rather than taking on faith:
stacklok.*and the deletion of the six legacy twins", and its supporting argument is entirely about those twins not being scraped in a default shipped deployment. That argument covers the 6 deletions. It doesn't reach the 15 renames, which are the larger share.examples/otel/grafana-dashboards/, which is at minimum evidence of intended scraped consumption.@reyortiz3 — as RFC author, could you confirm whether D13's scope was meant to cover the renames or only the twins? One sentence settles whether this needs an RFC amendment or just a note. I've deliberately not edited the RFC.
Deliberate design choices, in case they look wrong at a glance:
find_tool's legacy counter increments before the call. Also looks inconsistent next to the new outcome counter, which increments after. The original counted attempts, so reproducing it faithfully means keeping that placement.legacymetrics.gotakes the metric name as a parameter and contains no name literals, so the seam has no opinion about what's being aliased.token_savingsis the one row where the rename is nearly invisible. Both the new metric and its alias carry unit%, so they export asstacklok_vmcp_optimizer_token_savings_percentandtoolhive_vmcp_optimizer_token_savings_percent— the new Prometheus name differs from the old only in prefix, and the exporter does not double the_percentsuffix on the alias (verified against a real scrape). Worth a sanity check by a second pair of eyes.Extra scrutiny welcome on two things:
useLegacyAttributesis already documented. Two things are therefore still open, and I'd rather settle the version before shipping either: pinning actual release numbers in the timeline, and the startup deprecation warning @aponcedeleonch and @ChrisJBurns both asked for. A warning that says "a future release" gives users nothing to plan against, so I left it out pending a number. Happy to add both here.Known gap, not fixed here (pre-existing, and orthogonal to dual emission):
stacklok.build_infoexports asstacklok_build_info_ratio, becausecoremetrics.RegisterBuildInfosetsmetric.WithUnit("1")and the Prometheus translator appends a unit suffix._ratiois wrong for an identity gauge, and it means a dashboard joining onstacklok_build_inforeturns empty. The durable fix is upstream intoolhive-core. Happy to file it, or fold it in here if you'd rather it not reach customer dashboards at all.🤖 Generated with Claude Code