fix: bound tenant metrics on successful control paths - #806
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTenant request and business-event metrics now conditionally emit tenant labels. Tenant HTTP byte metrics are removed from recording, instrumentation, tests, and dashboard documentation. Metric contract assertions and smoke checks are updated for aggregate-only control-plane successes. ChangesTenant metrics cardinality
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR tightens the Prometheus metric label cardinality for tenant-related metrics by aggregating successful control-plane request series (and HTTP transport bytes) while preserving tenant/org attribution for data-plane traffic and error paths.
Changes:
- Make
drive9_tenant_requests_totalconditionally includetenant_id/tidbcloud_org_idbased on(surface, status_class)to bound high-cardinality series. - Aggregate
drive9_tenant_http_bytes_totalbydirectiononly (no tenant/org labels). - Update metric contract tests and add an e2e shared-smoke guard to ensure metric shape stays bounded.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/server/instrumentation.go | Switch tenant HTTP byte recording to the new aggregate-only bytes API. |
| pkg/server/auth_test.go | Adjust status-route metric assertions to expect aggregated (non-tenant-labeled) success series. |
| pkg/metrics/operations.go | Implement conditional tenant attribution for request counters; aggregate HTTP byte metric by direction only; narrow business-event tenant labeling to failures. |
| pkg/metrics/operations_test.go | Update metric contract tests to reflect new label/attribution rules for requests, events, and bytes. |
| e2e/shared-smoke-test/cases/metrics-consistency.sh | Add metric-shape guards (bounded series count for HTTP bytes; no tenant labels on control-plane 2xx request series). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025988f0c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
LGTM.
Verified locally on 2bd0a80d in an isolated worktree: go build ./... clean, go vet ./pkg/... clean, and pkg/metrics + pkg/server pass with -count=1. Net −12 lines (+96/−108), changes are tightly scoped with no unrelated refactoring.
This implements the three P0 items from the cardinality analysis, and the alert side needs no changes at all — which I verified rather than assumed.
Measured baseline this addresses
From dev-tke-bj (cls-9rse7rff, context verified, env=dev, 3 pods), scraping :9009/metrics directly since that cluster has no vmagent: 363,033 lines per pod, under a 10k-tenant load test.
The key finding was that surface="fs" had exactly 10,000 distinct tenants — matching the load test precisely — while tenant_requests_total had 31,655. The extra 21,655 were all surface="provision", and cross-checking confirmed the leak boundary was clean: tenant_storage_bytes and fs_events_rows were strict subsets of the fs set (empty difference), while tenant_http_bytes_total had 21,655 tenants outside it.
The reason this matters beyond the current numbers: tenant_count{status="active"} is 574,071, and only 21,655 have gone through the HTTP provision path so far. Provisioning is a one-shot lifecycle event, but the series it left behind persisted forever on whichever pod served the request — the WorkMetricsCleanup path added in #802 only fires on deleting/deleted transitions, so active tenants never get cleaned. This was the only genuinely unbounded metric in the system, on track for ~26× current cardinality.
The three changes
1. tenant_http_bytes_total removed entirely (−63,310)
Right call over just dropping the tenant label. It had zero alert consumers, and its only dashboard panel was already broken: HTTP Transport Bytes grouped and filtered by surface, which #802 had removed, so it had been rendering empty since then. With tenant_file_bytes_total confirmed as the billing metric — its 7 call sites all record logical file bytes on fs/upload read/write paths, independent of whether data lands in DB or S3 — the transport-layer counter had no remaining purpose. Declaration, both Record* wrappers, the private helper, both call sites, the dead panel, and both README lines all go together. No orphaned code.
2. tenant_provision attribution split by result (−~42,500)
Successful provisioning (ok/accepted/shared_pool) drops the tenant labels; failures keep them. This reuses isOperationFailureResult from #802, which already matches error and cluster_error, with quota_error matching via the _error suffix rule.
This is the design point I'd flag as the most important one in the PR. My first suggestion was to drop tenant_provision from the whitelist outright and rely on tenant_operation_failures_total for attribution — that was wrong, and I checked before asserting it here: provisioning failures are reported only through metricEvent(ctx, "tenant_provision", "result", ...), and tenant_operation_failures_total is populated exclusively by RecordTenantOperation*, which the provision path never calls (its only instrumentation is a single setRequestMetricTenant at server.go:5589). Dropping the whitelist entry would have left Drive9TenantProvisionFailures with no backing metric. Splitting by result keeps per-tenant failure attribution while bounding the unbounded success path — consistent with the "failures only carry tenant" philosophy #802 established.
3. Control-plane surfaces split by status class (−~23,500)
Data-plane surfaces (fs, upload, object_store, sql, events, journal, git_workspace) always carry tenant labels; control-plane surfaces only on 4xx/5xx. Measured impact is negligible on the savings — 21,569 of provision's 21,655 series were 2xx, and all 1,969 status series were — while preserving per-tenant attribution for control-plane errors.
Alert compatibility: verified, zero runbooks changes
I checked each affected rule against the new label shapes:
Drive9TenantProvisionFailuresselectsresult=~"error|cluster_error"withtenant_id!=""— lands exactly on the branchisOperationFailureResultmatches, so per-tenant attribution survives.Drive9TenantSchemaInitFailuresselectsevent="tenant_schema_init"withtenant_id!=""— unconditionally whitelisted.Drive9TenantServerErrorRateHighreads through thedrive9_tenant_requests:rate5m:*recording rule, whosesurface=~"fs|upload|object_store|sql|events|journal|git_workspace"filter is identical to the newtenantAttributedSurfacedata-plane list.
That last one is worth noting: the runbooks side had already narrowed to these seven surfaces, so the whitelist here is aligned with an existing contract rather than newly invented.
The self-caught regression in 575c4a8c
Worth calling out. The first version wrote:
return event == "tenant_provision" && isOperationFailureResult(result)which silently dropped auth, tenant_schema_init, and tenant_status from the whitelist — meaning Drive9TenantSchemaInitFailures would have gone false-green. Exactly the failure class this whole effort has been fighting. Catching and reverting to the switch form before review is the right instinct.
Cardinality guard
Adding the assertion to e2e/shared-smoke-test/cases/metrics-consistency.sh — control-plane 2xx series must not carry tenant_id, fail if they do — is more valuable than a unit test here, because it validates real /metrics output. This is the gate whose absence let the original problem through.
Minor, non-blocking
docs/grafana/README.md still documents drive9_tenant_requests_total as carrying result unconditionally, and drive9_tenant_file_bytes_total as carrying surface/action. Both were removed in #802, so the doc drift predates this PR — but since it already edits this file, correcting those two lines would be a cheap win. The $surface template variable in the usage dashboard is also worth a look now that the panel using it is gone.
Remaining work from the analysis (out of scope here)
- P1 —
shared_db_pool_*: 5,698 physical pools produce ~39,886 series across four metrics.soft_max/hard_maxare configuration constants andspending_limitis per-org (only 2 distinct orgs measured), so exporting them per-org instead of per-pool saves ~17,000;shared_db_pool_totalis always 1 and its presence semantics are already covered byshared_db_pool_tenants(another ~5,700). Also worth dropping the redundantdb_pool_idlabel sincedb_pool_uuidis globally unique — it doesn't reduce series count but stops identity churn on every pool create/delete from accumulating within the retention window.status_ageat 95 series (non-active only) is correctly designed; leave it. - Billing metrics stay as-is:
tenant_file_bytes_total,tenant_storage_bytes,tenant_media_files,tenant_video_files— 90,000 series, all strict subsets of the fs tenant set with no leakage. Legitimate cost. - Worth a separate look:
tenant_operation_failures_totalcovers 10,000 tenants, of whichaction="write", status_class="4xx"accounts for 9,552 — near-total. Could be expected quota rejection or something the load test surfaced. That's an observability question, not a cardinality one.
Also confirmed clean and needing no action: service_gauge is down to 51 series per pod (46 from mutation_replay), so #802's Set(0) → Delete conversion fully worked — earlier high readings were pre-#802 residue. db_pool_connections has zero distinct tenant_id, as intended.
Nothing blocking.
Summary
Validation
Summary by CodeRabbit