feat: per-user token usage as a Grafana table (LGTM, no Langfuse) - #112
feat: per-user token usage as a Grafana table (LGTM, no Langfuse)#112tylerpotts wants to merge 20 commits into
Conversation
Live-cluster testing showed the v1.6.2 SecurityPolicy CRD already has apiKeyAuth.forwardClientIDHeader/sanitize; they were added in EG v1.5.1. The earlier v1.7.0 bump was unnecessary — revert examples + dev/Makefile to the pack's existing v1.6.2 pin and fix the code/docs claims.
…rom) Live-cluster testing showed ai-gateway-helm v0.5.0 flattens extProc.extraEnvVars into a CLI arg and only honors literal value:; secretKeyRef rendered as nil and no traces exported. Switch the OTLP endpoint/headers to literals (with a security caveat about the Basic header containing the Langfuse secret key), drop the now-unused interface-Secret example, and update docs.
Companion to the prior commit: ai-gateway-helm v0.5.0 only honors literal value: in extProc.extraEnvVars (secretKeyRef -> nil), so the OTLP endpoint and Basic-auth header are literals (with a security caveat). Docs §14 updated; the chart-limitation is documented.
…ot a Langfuse span
Live testing on tyler-hetzner-dev proved the gen_ai_* metrics are exposed on the extProc sidecar admin port (:1064 /metrics), not on envoy's :19001 /stats/prometheus that the default prometheus.io/scrape annotation targets. A dedicated collector scrape job for :1064 is required. Document the scrape_config and correct the spec; the metricsRequestHeaderAttributes label (user_id) is confirmed working.
Verified on a rebuilt cluster: an unescaped ${1} crashes the OpenTelemetry
Collector (env-var expansion), so the documented scrape_config must use
$${1}. End-to-end scrape :1064 -> Mimir -> per-user token query confirmed.
…e_usage) Verified on a live cluster with a streaming mock: streaming requests are attributed only when the client sends stream_options.include_usage=true (10 with it -> exact usage recorded; 5 without -> zero). Non-streaming always counted.
dcmcand
left a comment
There was a problem hiding this comment.
Architecture review
Reviewed against origin/main, which is 100 commits ahead of this branch's merge base (b31234e). GitHub reports the PR as CONFLICTING, and git merge-tree --write-tree origin/main 4a6516ae confirms exactly three conflicts: dev/Makefile, operator/internal/controller/reconcilers/auth.go, operator/internal/controller/reconcilers/auth_test.go. Most of what follows falls out of that staleness rather than out of the design itself, so please read it as "this needs a rebase and five targeted edits", not "this approach is wrong". The approach is right.
This review focuses on architectural fit rather than line-level correctness. It enforces documented patterns with high confidence and inferred ones more softly (Questions rather than Blockers), so it may miss patterns that are not written down anywhere.
What's good here
- The pivot away from self-hosted Langfuse is the right call. Trading Postgres + ClickHouse + Redis + MinIO plus an OTLP Basic-auth secret in the GitOps repo for one metric label and a Grafana table is a big reduction in operational surface, and it removes a secret from the repo. The "Pivot note" in the description documents the reasoning honestly.
- The commit history is exemplary. Each live-cluster discovery is its own commit carrying its evidence:
6379c05dcorrects the Envoy Gateway floor from v1.7 to v1.5.1 after finding the fields already present in v1.6.2;f6e58742findsgen_ai_*on the extProc admin port:1064rather than envoy's:19001;644ef6f1finds the OTel${1}env-expansion crash;4a6516aequantifies the streaming caveat (10 requests withinclude_usagecounted exactly, 5 without counted zero). That is real engineering, not guesswork. - The two operator gotchas are the most valuable content in the PR. The
$${1}escaping (with the exactenvironment variable "1" has invalid namefailure) and the:1064-vs-:19001scrape distinction will each save someone a day. They must survive the rebase verbatim. - The chart template is idiomatic. It reuses
nebari-llm-serving.fullnameandnebari-llm-serving.labels, gates on a value like every other template, and uses.Files.Getwith a siblingdashboards/directory, which keeps the JSON out of the template and stays valid in the OCI-packaged chart. The{{- ... | nindent 4 }}underper-user-token-usage.json: |-handles the indentation trap correctly. The header comment explains why the namespace is set explicitly rather than just what the line does; more of this, please. - Scope discipline is explicit and appropriate. Per-model split, alerting, cost estimation, and JWT-path attribution are all named as deferred rather than half-built.
- The new
auth_test.gocase is table-driven per the repo convention, and itsresult.ExternalSecurityPolicy == nilguard before the type assertion is better than main's equivalent atmain:operator/internal/controller/reconcilers/auth_test.go:266, which would panic rather than fail cleanly. Worth porting that guard to main independently of this PR.
Blockers
1. The forwarded header name is wrong against main, and it is load-bearing for authorization, not just telemetry.
main:operator/internal/controller/reconcilers/auth.go:20 defines const apiKeyClientIDHeader = "x-llm-client-id". This PR introduces the literal "x-client-id" in five places: auth.go, auth_test.go, examples/envoy-ai-gateway.yaml, the values.yaml comment, and the token-usage-dashboard.yaml comment, plus docs §14.1 and §14.2.
That constant is consumed by more than the forwardClientIDHeader field. On main it is also the authorization principal matcher (main:auth.go:220) for the deny-by-default per-model scoping added in #117 (#116 / #122), and it is asserted by main:auth_test.go:270,302, main:passthrough_test.go:451, and the live anti-spoof e2e main:dev/test-model-scope.sh:206-214. It is documented at main:docs/src/content/docs/architecture.mdx:200,205.
Two separate consequences:
- In the chart/examples:
controller.metricsRequestHeaderAttributes: "x-client-id:user.id"maps a header the gateway never populates. Theuser.idattribute is never set,user_idis absent from every series, and the dashboard'suser_id!=""selector drops everything. The table renders permanently empty with nothing logged anywhere, which is the worst failure mode for an observability feature. Fix:"x-llm-client-id:user.id". - In the operator: if the conflict resolved in this branch's favour, authentication would forward
x-client-idwhile authorization matchedx-llm-client-id, so the allow rule would never match and every external request would 403.
I confirmed the mechanism itself is correct upstream: ai-gateway-helm v0.5.0 values.yaml:68-71 documents metricsRequestHeaderAttributes as "header1:label1,header2:label2" with the example "x-team-id:team.id,x-user-id:user.id". Only the header name is wrong.
2. The auth.go conflict is shaped so that a careless resolution silently reverts #117's security fix.
Please resolve this one by hand rather than with "take theirs". In the conflicted file, main's side of the region spans lines 181-223 and swallows far more than the two fields:
<<<<<<< origin/main
"forwardClientIDHeader": apiKeyClientIDHeader,
"sanitize": true,
},
"authorization": buildAPIKeyAuthorization(clientIDs), // <-- per-model scoping
...
func buildAPIKeyAuthorization(clientIDs []string) ... { // <-- entire function
=======
"forwardClientIDHeader": "x-client-id",
"sanitize": true,
>>>>>>> HEADTaking this branch's side deletes the deny-by-default per-model authorization block and the whole buildAPIKeyAuthorization function, reverting #117 including the R-04 anti-spoofing hardening.
The simplest fix for both 1 and 2: drop the auth.go and auth_test.go hunks entirely. Both forwardClientIDHeader and sanitize already ship on main (landed in dac9bc4f / #117), and main:auth_test.go:260 already covers them, asserting against the constant rather than a literal. Nothing in this PR's Go change is novel. Same for dev/Makefile: main parameterises the version as ENVOY_GATEWAY_VERSION ?= v1.6.7 (main:dev/Makefile:16), so changing a hardcoded v1.3.0 to a hardcoded v1.6.2 is both a conflict and a regression to an older pin.
3. The dashboard ConfigMap targets a foreign namespace and is enabled by default.
charts/nebari-llm-serving/templates/token-usage-dashboard.yaml sets namespace: {{ .Values.observability.dashboard.namespace }} (default monitoring) with enabled: true. This is the first chart template to do so: all 15 namespaced templates on main use include "nebari-llm-serving.operatorNamespace" ., and architecture.mdx documents a single-namespace deployment model while pack-metadata.yaml declares scope.standalone-supported: yes.
Three concrete consequences:
helm installfails on any cluster without a pre-existingmonitoringnamespace, breaking the declared standalone install.- The pack deploys as an ArgoCD
Applicationwithdestination.namespace: nebari-llm-serving-systemunder projectfoundational(examples/argocd-application.yaml). A resource outside that destination is out-of-scope unless the AppProject, owned by nebari-infrastructure-core, whitelistsmonitoring, so this likely hard-fails sync on the primary deployment path. prune: trueon either side creates ownership ambiguity over an object living in another pack's namespace.
Suggested fix: default enabled: false and document the AppProject/RBAC change it requires, or invert the integration per the Question below. If the LGTM pack ships a Grafana Operator, a GrafanaDashboard CR with an instance selector is the namespace-agnostic form.
4. The new Helm values are documented in neither values reference.
README.md:192 and docs/src/content/docs/configuration.mdx:20 both enumerate every value, down to defaults.monitoring.enabled. This PR adds observability.dashboard.enabled and observability.dashboard.namespace and touches neither file. A default-on switch that writes into another namespace is exactly the value an operator needs to find in the reference table.
5. The docs land in a file that no longer exists, with no sidebar entry.
docs/install-production.md was deleted in ded34521 when the docs moved to Astro/Starlight; the runbook is now docs/src/content/docs/installation.md. The good news is that git's rename detection does follow it - merge-tree reports Auto-merging docs/src/content/docs/installation.md - and installation.md is one of the five files excluded from scripts/check-content-parity.mjs, so appending there is permitted. Two things still need doing:
- Main's last section is
## 13. Troubleshooting, so## 14.lands after the troubleshooting appendix. Either move it above §13 or renumber. - No
sidebarentry is added todocs/astro.config.mjs. PerREADME.md§Documentation site, this content would arguably be better as its own page (docs/src/content/docs/observability.md) with the value rows going intoconfiguration.mdx, since it is feature configuration rather than install-runbook material.
6. Version-floor claims disagree with main.
Docs §14.1 and examples/envoy-gateway.yaml say "v1.6.2, the pack's pinned version". Main carries two pins: v1.6.2 in examples/envoy-gateway.yaml and v1.6.7 in dev/Makefile, and architecture.mdx states the validated combination as EG v1.6.7 / AI Gateway v0.5. Pick one authoritative statement and have the others reference it. The "absent in v1.3" aside can go, since main no longer mentions v1.3 anywhere.
I did verify the underlying claim upstream: forwardClientIDHeader ("the name of the header to forward the client identity to the backend service") and sanitize ("indicates whether to remove the API key from the request before forwarding it") both exist on APIKeyAuth in the Envoy Gateway extension types, so the v1.5.1+ floor looks right.
Questions
Should the :1064 scrape be shipped rather than documented? This is the most consequential design decision in the PR, so I want to raise it explicitly rather than bury it. As written, the pipeline is inert until a human hand-edits an OpenTelemetry Collector config owned by a different repo (the nebari-infrastructure-core LGTM pack, whose collector config §14.2 itself notes is single-owner per ADR-0005). The dashboard ships enabled and renders an empty table until that happens, and the pack has no way to declare or detect the dependency: pack-metadata.yaml has no dependency field, and no README or installation.md prerequisite entry or NIC tracking issue is added here. It also couples this pack to three internals of the other pack's config: the extProc admin port :1064, the gateway_envoyproxy_io_owning_gateway_name pod-label regex, and OTel's $${1} escaping.
The pack already solves this exact problem once, in the other direction: it owns a PodMonitor for model pods in its own namespace and lets the platform stack discover it (reconcilers/modelservice.go:376), treating the CRD as optional so a cluster without Prometheus Operator still installs cleanly (llmmodel_controller.go:366). main:charts/nebari-llm-serving/templates/operator-clusterrole.yaml:64 already grants monitoring.coreos.com. If the LGTM collector honours prometheus-operator CRDs, a pack-owned PodMonitor selecting the gateway proxy pods on port 1064 would remove the out-of-repo manual step entirely and keep every resource inside this pack's ownership, which also dissolves Blocker 3.
Was that considered and rejected, or is the manual path a v1 expedient? If it stays manual, please add the prerequisite to the README and installation.md plus a tracking issue in nebari-infrastructure-core, mirroring the GPU operator (nebari-infrastructure-core#232) and AI Gateway (#44) precedents.
Does the installer actually have permission to write into monitoring? I could not determine this from the repo. ArgoCD rejects out-of-destination resources unless the project allows the target, and a namespace-scoped install role would not cover monitoring. If either is restrictive, the fix for Blocker 3 is "ship it into the release namespace and require the sidecar to watch all namespaces" rather than a default flip, which is a different design.
Is observability the right place for this value? Main's top-level keys are either components (keyManager, frontend, operator, modelDownloader) or platform-wide concerns (platform, auth, defaults), and the one comparable knob today is defaults.monitoring.enabled. Two similarly-named monitoring switches at different nesting levels will get confused; defaults.monitoring.dashboard.* may sit better. Genuinely a judgement call, since main's top-level layout is heterogeneous enough that this is inferred rather than stated.
Are these numbers meant to be advisory, or a basis for chargeback? It matters for how hard the streaming caveat needs to be fixed, since the figure can currently be understated: streaming requests without stream_options.include_usage contribute zero, per 4a6516ae. Advisory is fine; chargeback is not, until that is resolved.
Stylistic
- The description no longer matches the diff, which matters because a reviewer working from it would approve a different design. Stage 3 says "The proxy already carries
prometheus.io/scrape, so the LGTM collector scrapesgen_ai_*from/stats/prometheus… no gateway-side OTLP export", whichf6e58742reverses in favour of the dedicated:1064job. The "Removed the self-hosted Langfuse ArgoCD app and all OTLP/Langfuse wiring" line is true within this branch's history, but the net diff against main contains no such removal because none of it ever landed on main. Worth refreshing before merge so the review record matches the shipped design. - The OTel
scrape_configis pasted inline in the docs.docs/test/docs-examples.test.mjscodifies the opposite convention for manifests ("does not keep an inline duplicate"), and this is a snippet meant to be applied verbatim. Consider committingexamples/lgtm-collector-genai-scrape.yamland embedding it with a?rawimport so it is lintable and cannot drift. token-usage-dashboard.yamlbreaks the<component>-<resource>.yamlfilename convention used byfrontend-configmap.yaml,key-manager-role.yaml, and friends. Considerobservability-dashboard-configmap.yaml.examples/envoy-ai-gateway.yamlat this HEAD still carriesrepoURL: oci://docker.io/envoyproxy, which main's docs test now forbids ("drops the oci:// prefix (NIC form)"). Rebase artifact; flagging only so it is not reintroduced. Thehelm.values:block itself matches the established pattern inexamples/envoy-gateway.yamland is fine.
Shortest path to merge
Rebase onto main and:
- Drop the
auth.go,auth_test.go, anddev/Makefilehunks. All three are superseded, and they are the only three conflicts. - Change the header to
x-llm-client-idin all five places, and referenceapiKeyClientIDHeaderin a comment so the coupling is discoverable from the chart side. - Default
observability.dashboard.enabledtofalse, or convert to a pack-ownedPodMonitorper the Question above. - Add the two values to the README table and
configuration.mdx. - Re-home §14 in
docs/src/content/docs/installation.mdabove §13, or as its own page with a sidebar entry.
What remains after that is small, high-value, and entirely absent from main today: metricsRequestHeaderAttributes, gen_ai, observability, and grafana_dashboard all return zero hits across origin/main. The dashboard, the chart plumbing, and the two hard-won operator gotchas are all genuinely new work.
What
Attribute LLM token usage to the API-key owner who made each request, and
surface it as a Grafana table — one row per user, total tokens used in
the last 30 days — built entirely on the LGTM pack (Grafana + Mimir)
already running in the cluster. No Langfuse, no extra datastores, no secrets
in GitOps.
Scope: external (API-key) access path only. The internal JWT path is not
attributed in this version.
How it works
apiKeyAuth.forwardClientIDHeader: x-client-id+sanitize: trueon the externalSecurityPolicy, so Envoy Gatewayforwards the matched key's clientID (e.g.
user-chuck-1) downstream andstrips the raw key. (Requires Envoy Gateway v1.5.1+; present in the pack's
pinned v1.6.2.)
controller.metricsRequestHeaderAttributes: "x-client-id:user.id"labels the
gen_ai_client_token_usagemetric withuser_id=<clientID>.prometheus.io/scrape, so the LGTM collectorscrapes
gen_ai_*from/stats/prometheusinto Mimir — no gateway-sideOTLP export.
grafana_dashboard: "1", auto-loaded by the sidecar) runssum by (user_id) (increase(gen_ai_client_token_usage_sum{user_id!=""}[30d]))in one Table panel.
Changes
operator/.../auth.go— forward clientID header + sanitize (kept; comment updated)examples/envoy-ai-gateway.yaml— metric-label value (replaces OTLP→Langfuse env)charts/.../dashboards/per-user-token-usage.json— dashboard JSONcharts/.../templates/token-usage-dashboard.yaml+values.yaml—ConfigMap gated on
observability.dashboard.enabled(namespacemonitoring)docs/install-production.md§14 — rewritten for the metric + dashboard pathPivot note
This PR originally routed per-user usage to a self-hosted Langfuse via OTLP
traces. Live-cluster testing showed that path is heavy (Postgres + ClickHouse
token-usage metric + Grafana table delivers the same per-user view on
infrastructure already present, with nothing secret in the repo.
Out of scope (v1)
Per-model / input-output split, time-series & rate panels, alerting, cost
estimation, and internal/JWT-user attribution.
Testing
go build ./... && go test ./internal/controller/reconcilers/— passhelm template/helm lint— render clean; ConfigMap embeds validdashboard JSON; toggle off removes it
gen_ai_client_token_usage_*{user_id="..."}reaches Mimir after authedrequests, then that the Grafana table renders it. Blocked in the dev cluster
by an extProc sidecar-injection install issue (env, not a code defect).