Skip to content

feat(telemetry): shared metrics vocabulary + operator reconcile emitter - #183

Merged
glageju merged 5 commits into
mainfrom
gautam/metrics-std-core
Jul 23, 2026
Merged

feat(telemetry): shared metrics vocabulary + operator reconcile emitter#183
glageju merged 5 commits into
mainfrom
gautam/metrics-std-core

Conversation

@glageju

@glageju glageju commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Why

Every Stacklok component today invents its own metric vocabulary — one calls the upstream server mcp_server, another server; one histogram is in seconds, the next in milliseconds; success-vs-failure is a label here and two separate metric names there. The result: you can't write a single cross-component dashboard or alert without memorizing five schemes, and a rename in one repo silently breaks a join in another.

This is the foundational PR of the Metrics Standardization RFC (D6/D8). It puts the shared vocabulary in toolhive-core — one place that defines the label spellings, the histogram bucket shapes, and the emitter-ownership attributes — so every downstream component spells the same concept the same way. Nothing here emits a metric; it's vocabulary plus construction helpers that everyone imports.

What it adds

One package: telemetry/metrics.

  • 7 canonical label keys (LabelMCPServer, LabelOutcome, LabelMCPMethod, …) and 3 outcome values (success/error/rejected), so outcome rides on a label instead of splitting into _succeed/_failed metric names.
  • 3 histogram bucket presets — BucketsFastHTTP(), BucketsMCPProxy(), BucketsLongRunning() — returned as fresh slices so callers can't alias a shared one.
  • D8 emitter-ownership: the stacklok.component / stacklok.product resource-attribute keys, plus the frozen product value stacklok-platform. The per-component stacklok.component value is supplied by each component — core names the concept, not the roster of apps that consume it.
  • RegisterBuildInfo(...) + the fleet-wide stacklok.build_info gauge for build/release correlation.

How downstream uses it

A proxy or gateway builds its own instrument with a shared preset and label key — no meter comes from this package:

meter := otel.Meter("stacklok.toolhive.proxy")
hist, _ := meter.Float64Histogram(
    "stacklok.toolhive.proxy.request.duration",
    metric.WithExplicitBucketBoundaries(metrics.BucketsFastHTTP()...),
)
hist.Record(ctx, elapsed, metric.WithAttributes(
    attribute.String(metrics.LabelOutcome, metrics.OutcomeSuccess),
))

Ownership attributes go on the OTel resource once at startup; the component value is the caller's own:

resource.NewWithAttributes(semconv.SchemaURL,
    attribute.String(metrics.AttrStacklokComponent, "toolhive"),
    attribute.String(metrics.AttrStacklokProduct, metrics.ProductStacklokPlatform),
)

Design decisions worth a reviewer's eye

  • No Meter(), no metric-name constants. The caller owns its meter and passes a preset's boundaries; this package registers no global state and pins no instrument names.
  • Two-or-more-emitter rule. Only concepts ≥2 components emit live here. Single-emitter keys stay with their owner.
  • Keys, not per-app values. Core exports the ownership attribute keys, not the concrete component strings — a foundational library shouldn't enumerate its consumers. The one exception is ProductStacklokPlatform: every component must emit that identical string or the fleet-wide stacklok_product selector breaks, so it's a single cross-cutting contract rather than a per-app value.
  • stacklok.product frozen at stacklok-platform. Deliberately not stacklok-enterprise — the value lands in customer dashboards and must survive the enterprise→platform rename without churn.
  • Resource attribute vs metric label. Ownership attrs are set once on the provider and promoted to labels by the Prometheus exporter, not stamped per instrument.

Tests

The package is guarded by pin tests that fail loudly on a silent respelling — the whole point of the shared vocabulary is that these strings don't drift. They pin every bucket boundary set, label key, outcome value, ownership key, and the frozen product value; plus build_info name/unit and RegisterBuildInfo behavior (validation + the empty→unknown fallback, scraped from a real in-memory OTel reader).

Also in this PR

google.golang.org/grpc bumped 1.82.0 → 1.82.1 to clear a High-severity advisory (GHSA-hrxh-6v49-42gf) the Grype scan flagged — a pre-existing indirect dep, not introduced here, but it rides into the release tag downstream will pin.

Not in this PR

The operator reconcile emitter (D12) was split out on review. It's operator-shared vocabulary, not universal, so its placement — raw toolhive-core vs a shared-operator library — is an open D12 question tracked separately rather than settled here.

Merge order

This must merge and cut a release tag first. toolhive, toolhive-registry-server, and stacklok-enterprise-platform all import this vocabulary and can't build standalone until they bump their toolhive-core pin to the release carrying it.

Links

  • RFC: Stacklok Platform Metrics Standardization (D6, D8)

glageju and others added 4 commits July 22, 2026 15:03
…rpc to 1.82.1

Resolve goconst lint on the reconcile guard test by asserting against the
package label constants instead of string literals. Bump the pre-existing
indirect google.golang.org/grpc dependency 1.82.0 -> 1.82.1 to clear the
High-severity GHSA-hrxh-6v49-42gf flagged by the Grype scan.
…allback registration

Addresses PR review findings on the metrics vocabulary and reconcile
emitter packages:

- RegisterBuildInfo now validates nil meter / empty component and sets
  an explicit unit, matching reconcile.New's contract; adds real test
  coverage via a ManualReader (was previously untested).
- RegisterManagedResources rejects a second registration on the same
  Emitter instead of silently double-counting gauge observations.
- Fixes the ownership guard test's gateway-ban assertion, which checked
  an unreachable substring and never actually enforced the RFC's D3 ban.
- Documents the RecordReconcileError extra-attributes contract and the
  reconcile triplet's namespace/name cardinality trade-off.
- Updates README.md/CLAUDE.md package tables and doc.go for the
  telemetry/metrics rename and the new telemetry/reconcile package.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…failure

sync.Once permanently latched RegisterManagedResources on its first call
regardless of outcome, so a transient RegisterCallback failure (e.g. a
mismatched meter) locked the Emitter into "already registered" forever.
Guard with a mutex instead and only latch on success.
@glageju
glageju marked this pull request as ready for review July 23, 2026 00:40
Comment thread telemetry/reconcile/reconcile.go Outdated
Comment thread telemetry/metrics/ownership.go Outdated
Address review: toolhive-core should not enumerate the apps that consume
it. Remove the per-component Component* value roster from metrics/ownership;
each component now supplies its own stacklok.component value. Keep the
AttrStacklok* keys and the frozen ProductStacklokPlatform, which is a single
cross-cutting value every emitter must share verbatim.

Remove telemetry/reconcile entirely. It is operator-specific vocabulary
shared by two operators (ai-gateway, toolhive-enterprise); its placement
(shared-operator library vs core) is an open RFC/D12 question to settle
separately, not in this foundational vocabulary PR.
@glageju
glageju requested a review from reyortiz3 July 23, 2026 18:31
Comment thread telemetry/metrics/ownership.go
@glageju
glageju merged commit 2853a50 into main Jul 23, 2026
5 checks passed
@glageju
glageju deleted the gautam/metrics-std-core branch July 23, 2026 19:02
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.

2 participants