fix: reduce metric cardinality and close observability gaps - #802
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (55)
✨ Finishing Touches🧪 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 reduces Prometheus metric cardinality across tenant-scoped instrumentation while adding explicit lifecycle cleanup (including cross-pod cleanup via the tenant notify outbox) and closing observability gaps with new bounded “critical-path” metrics. It also implements a more explicit TiDB Cloud OpenAPI api/operation/result metric contract directly in the HTTP producer (including digest retry and protocol/transport classification).
Changes:
- Add TiDB Cloud OpenAPI request classification metrics (api/operation/result) and test coverage for retry/error/protocol behaviors.
- Broadcast tenant metrics cleanup on durable delete/deleting transitions and delete healthy/retired tenant gauge series (plus related tests).
- Add bounded operational metrics for mutation dispatch, API key cache, notify coalescing/outbox delivery, shared-pool convergence/cleanup, Vault feature state, and DB pool state aggregation.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/tenant/tidbcloudnative/provisioner_test.go | Adds tests that assert TiDB Cloud OpenAPI metrics classification and digest retry behavior. |
| pkg/tenant/tidbcloudnative/openapi_metrics.go | Defines api/operation/result constants and request/response classification helpers. |
| pkg/tenant/tidbcloudnative/http_clusters_api.go | Records TiDB Cloud OpenAPI metrics per logical request, including protocol failures and transport classification. |
| pkg/server/work_mask.go | Adds WorkMetricsCleanup bit for outbox-driven per-pod tenant metric cleanup. |
| pkg/server/vault_read_test.go | Updates Vault metrics assertions to reflect reduced tenant-labeling on success paths. |
| pkg/server/tenant_worker.go | Deletes healthy semantic-worker gauges; instruments fs_events retention paths; adds ForgetTenant hook. |
| pkg/server/tenant_worker_metrics_test.go | Tests that semantic-worker observation exports only abnormal gauge state. |
| pkg/server/tenant_pool_reconcile.go | Records metric when stuck shared DB pools are marked failed. |
| pkg/server/tenant_outbox_poller.go | Records poll/cursor-flush metrics; applies broadcast metrics cleanup on WorkMetricsCleanup. |
| pkg/server/tenant_outbox_poller_test.go | Validates cleanup broadcast semantics and removal of tenant labels on success-path counters. |
| pkg/server/tenant_metrics.go | Adds shared DB pool status age metric and deletes it when no longer applicable. |
| pkg/server/tenant_metrics_test.go | Extends shared DB pool metrics test coverage to include status age series. |
| pkg/server/tenant_failed_cleanup.go | Broadcasts tenant metrics cleanup after durable “deleted” transitions in failed cleanup flows. |
| pkg/server/tenant_delete.go | Broadcasts tenant metrics cleanup during delete flows and after finalization. |
| pkg/server/tenant_delete_test.go | Asserts metrics cleanup is enqueued via tenant notify outbox on delete. |
| pkg/server/sse.go | Removes duplicate/dead event-bus publish-error counter in favor of unified operation failure attribution. |
| pkg/server/server.go | Sets Vault feature-enabled metric; adds broadcastTenantMetricsCleanup helper. |
| pkg/server/server_test.go | Tests Vault feature state vs module availability metrics across configurations. |
| pkg/server/quota.go | Removes duplicate TiDB Cloud OpenAPI metric recording now handled in HTTP producer. |
| pkg/server/notify_coalescer.go | Adds bounded metrics for pending size, flush outcomes, and per-row fallback results. |
| pkg/server/notify_coalescer_test.go | Asserts notify coalescer metrics are emitted on fallback paths. |
| pkg/server/managed_shared_db_failed_cleanup.go | Adds step/result metrics for failed shared DB pool cleanup stages. |
| pkg/server/instrumentation.go | Aggregates tenant in-flight metrics across actions per surface to reduce cardinality. |
| pkg/server/instrumentation_test.go | Tests in-flight aggregation behavior across actions. |
| pkg/server/fork.go | Broadcasts tenant metrics cleanup when fork cleanup marks tenants deleted. |
| pkg/server/eventbus_test.go | Updates expectation: old SSE in-flight series should be deleted (not zeroed). |
| pkg/server/auth_test.go | Updates tenant request metric assertions to match new label set (no derived result label). |
| pkg/server/admin_tenants.go | Broadcasts tenant metrics cleanup for admin delete paths. |
| pkg/server/admin_tenant_pool.go | Adds shared-pool wave metrics; broadcasts cleanup on pool-tenant deletions/cleanup. |
| pkg/metrics/workers.go | Introduces bounded worker/critical-path metrics (dispatcher/cache/coalescer/outbox/shared pool). |
| pkg/metrics/workers_test.go | Validates worker metrics stay bounded (no tenant_id/db_pool_id leakage). |
| pkg/metrics/registry.go | Adds internal helper to delete gauge series by label within a specific gauge name. |
| pkg/metrics/operations.go | Refactors label sets to reduce cardinality; adds failure-only tenant attribution counter; adds feature_enabled gauge; adds deletion helpers. |
| pkg/metrics/operations_test.go | Updates/extends metric contract tests for new label sets and deletion behaviors. |
| pkg/metrics/db.go | Reduces per-tenant DB pool connection state series; adds role-level aggregates for saturation states. |
| pkg/metrics/db_test.go | Asserts tenant pools omit idle/max_open series while retaining role-level max_open visibility. |
| pkg/meta/quota_setting_test.go | Updates expectations for aggregated service operation counters without tenant labels on success. |
| pkg/meta/db_pool.go | Extends shared DB pool metric snapshot to include updated_at for status age metric. |
| pkg/meta/api_key_cache.go | Adds cache hit/miss + entry-count metrics; emits entry gauge on fill/evict. |
| pkg/meta/api_key_cache_test.go | Adds test coverage for API key cache metrics. |
| pkg/backend/quota.go | Ensures quota snapshot replaces prior tenant gauge series to avoid stale org/limit labels. |
| pkg/backend/quota_metrics_test.go | Tests quota snapshot replacement semantics (org relabel + limit removal). |
| pkg/backend/mutation_replay.go | Deletes healthy backlog gauges and deletes prior-org gauges on org relabel. |
| pkg/backend/mutation_replay_metrics_test.go | Updates tests to require gauge deletion (not zeroing) for healthy state. |
| pkg/backend/mutation_dispatcher.go | Adds dispatcher queue depth/capacity, enqueue-blocked, batch size, and fallback metrics. |
| pkg/backend/mutation_dispatcher_test.go | Validates dispatcher metrics are present (capacity, fallback). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Regression update:
GitHub CI and local-e2e are still running on the latest head 265a59d. |
![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: 8c11d09186
ℹ️ 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".
|
Final regression update for head
|
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
Review
Verified locally on fix/metrics-cardinality-observability (8c11d091) in an isolated worktree: go build ./... clean, go vet clean on the touched trees, and pkg/metrics, pkg/server, pkg/backend, pkg/meta, pkg/tenant/tidbcloudnative all pass with -count=1. I also cross-checked the alert side (runbooks c52b6b7b): every drive9_* metric referenced by the rules now exists in code, and the four previously false-green alerts were replaced rather than silently dropped.
Good change overall. It closes the real gaps — false-green alerts, the Set(0)-instead-of-Delete leak class, and the five uninstrumented critical paths. The cross-pod cleanup via a new outbox work bit is a better mechanism than a per-pod TTL sweep. Findings below, most severe first.
Blocking
1. WorkMetricsCleanup = 8 has no compile-time assertion and no backend counterpart.
pkg/server/work_mask.go adds bit 3, but the var _ = [1]byte{}[...] assertion block still only covers SSE/Semantic/FileGC. The file's own doc comment says those assertions "ensure the values stay in sync" — bit 3 opts out of that guarantee. backend.BackendWork* stops at 4, so a future BackendWorkQuota = 8 in pkg/backend/dat9.go would collide with metrics-cleanup and the poller would run DeleteTenantCounters on every quota kick. The naming drift is already visible: the comment at the top of work_mask.go refers to a "Quota" bit that is not defined anywhere. Please either reserve bit 3 in the backend const block (even as a commented-out placeholder) or add an assertion that it is unclaimed.
2. DeleteTenantQuotaSnapshot on the quota-check hot path is both a contention and a correctness regression.
pkg/backend/quota.go calls it at the top of recordTenantQuotaSnapshot, which runs from three hot paths — file_count_check, storage_check, and mediaLLMQuotaExceededServerTx — i.e. effectively on every write.
- Cost: each call takes the registry write lock three times and scans every label set of all three gauges (
deleteGaugeByLabelinpkg/metrics/registry.go). That is O(total tenants), not O(1). At N tenants × write QPS it is an N² sweep under a process-global mutex on the write path — the opposite of this PR's goal. - Correctness: the delete removes
confirmed/reserved/limit, then re-addsconfirmed/reserved, and re-addslimitonlyif cfg.Max* > 0. A/metricsscrape landing between the delete and the re-add sees the tenant's series missing entirely — a disappearing denominator forDrive9Tenant*QuotaNearLimit. - Call sites fight each other:
storage_checkreaches the snapshot with a cfg whose media/video limits may be 0, so it deletes the media limit series a media check just wrote, and vice versa.
DeleteTenantQuotaLimitsWithOrg (added in this PR, unit-tested, currently dead code — no non-test caller) is the targeted fix: delete only the three state="limit" series, and only when the corresponding limit is absent. O(1), keeps confirmed/reserved continuously present, no registry scan.
Should fix
3. drive9_db_pool_connections role-only aggregate guard is dead.
In pkg/metrics/db.go the synthetic per-role aggregate is guarded by if _, exists := poolTotals[dbMetricKey{role: role}]. That zero-value key has tenantID/dbPoolUUID/tidbCloudOrgID as "", while every real key goes through metricKey(), which normalizes them to "unknown"/"guest". The lookup can never hit. Today nothing duplicates, because OpenInstrumentedForTenant always supplies a tenantID for user roles — but the guard does not protect what its comment claims, and Drive9DBConnectionPoolSaturation (unchanged, matches bare state= with no role filter) would divide across two different series if an unscoped role pool were ever registered. Build the probe key with metricKey(), or drop the guard and state the invariant.
4. Drive9TenantOutboxBacklogAgeHigh will flap.
RecordTenantOutboxPoll only sets backlog_oldest_age_seconds when full is true (batch size exactly tenantOutboxBatchSize) and sets it to 0 otherwise. A backlog draining to batchSize - 1 resets the gauge to 0 while rows are still pending. With for: 5m on > 30 at critical, a steady near-full backlog oscillates and never holds the window. Suggest reporting the age whenever len(rows) > 0 and gating the alert on the age alone.
5. RecordAPIKeyResolveCache calls entryCount() on every auth request.
pkg/meta/api_key_cache.go takes the cache mutex per hit/miss just to read len(c.byHash) for a gauge — on the exact hot path this cache exists to relieve (per its own comment, 76M execs/12h). fill and evictTenant already publish the count via RecordAPIKeyResolveCacheEntries, and the count only changes there, so the per-request read is redundant. Use the counter-only variant on the request path.
Related: the hits/misses atomics are now superseded by drive9_api_key_resolve_cache_requests_total and are unread outside tests — worth deleting.
6. Global semantic_worker gauges still write 0 and remain unalerted.
pkg/server/tenant_worker.go sets queued, processing, dead_lettered, queue_lag_seconds to 0 at start and never updates them (only inflight/workers are maintained). The per-tenant path was correctly switched to Delete and the two dead global alerts were correctly removed — but four permanently-zero global series remain. Either wire them or drop them; a constant-zero series invites someone to alert on it later.
Minor
7. _ = param placeholders leave a misleading API surface. pkg/metrics/operations.go now carries _ = result, _ = surface, _, _ = surface, action, _ = action, and _, _ = tenantID, tidbCloudOrgID; pkg/server/instrumentation.go has _ = action in tenantInFlightKey. Callers still compute and pass values that are discarded. Acceptable as a mechanical first pass, but the signatures should be narrowed in a follow-up or the dropped args will get quietly re-plumbed.
8. Drive9QuotaReservationLockConflictRetryHigh loses its attribution. The alert still groups by tenant_id, tidbcloud_org_id over drive9_service_operations_total, but that counter no longer carries those labels, so the grouping silently collapses. lock_conflict_retry is also absent from isOperationFailureResult, so it does not land in drive9_tenant_operation_failures_total either. This does resolve the guest-org bug I flagged earlier — but by removing attribution rather than fixing it. Worth deciding explicitly which behavior you want.
9. tidbcloudOperationForRequest returns "unknown" for unmatched paths. It is only reached from doDigestAuthRequest, and the per-method call sites pass explicit constants, so unknown should be unreachable. A one-line comment saying so would keep it from reading like an unbounded label source.
10. Drive9VaultUnavailable correctness depends on pod being attached to drive9_feature_enabled. SetFeatureEnabled sets no pod label; the join relies on the scrape adding it. It does for module_up, so this is very likely fine — worth confirming in one environment before relying on the and on (namespace, pod) join.
Not addressed (carried over, not necessarily for this PR)
- k8s-level alerts still hardcode
namespace=~"dat9|drive9-tidbcloud". The 7-environment namespace/container audit hasn't happened, so several cluster-level rules may be inert outside those two namespaces. DeleteTenantCounterson idle pool reap (pkg/tenant/pool.go) is unchanged —pool.goisn't in this diff. The new outbox broadcast fixes cleanup on tenant delete, which is the larger win, but a 2-minute idle reap still deletes counters for live tenants and makesincrease()under-count on per-tenant alerts. Now thatWorkMetricsCleanupexists, scoping counter deletion to lifecycle transitions only would be a small follow-up.- Tenant pool ref-leak detection remains uncovered, as documented in the rules file.
|
Review follow-up on head Addressed:
Reviewed but intentionally unchanged:
Validation on the pushed head:
GitHub CI and local E2E are running on this head. |
|
Review state rechecked against the current head 23a8ec6. GitHub currently reports no unresolved review threads and no additional formal review or inline comment beyond the four threads already fixed and resolved. No code change was made for this refresh because there is no newly retrievable technical finding to evaluate. The items intentionally left unchanged, with their technical reasons, remain documented in #802 (comment). If another review is still propagating or is visible only through a specific discussion link, it will be evaluated against the current head when that comment becomes available; actionable findings will be fixed, and rejected findings will receive an explanation in their own thread. |
Remove discarded metric parameters from tenant request, bytes, inflight, and fs_events APIs, and rename the local cleanup helper to reflect its behavior. Document that deleting/deleted lifecycle transitions intentionally use per-tenant transactions so outbox delivery remains atomic without holding locks across external cleanup work.
|
Follow-up corrections are pushed in c572e58.
This supersedes the earlier decision to leave signature narrowing as follow-up work. Per the requested submit-first order, the commit was pushed before waiting for complete regression. Before interruption, pkg/metrics, pkg/backend, and pkg/meta passed; pkg/server was still running and was deliberately interrupted, so it is not being reported as passed. |
|
Remaining review follow-ups disposition:
Focused verification before push: |
srstack
left a comment
srstack
left a comment
There was a problem hiding this comment.
LGTM.
All ten items from my earlier review are resolved on e13d5e98, including the three that were initially deferred or pushed back on. Two of those pushbacks were correct and I withdraw them: Drive9QuotaReservationLockConflictRetryHigh is already aggregate-only on runbooks c52b6b7b, so the tenant-grouping concern never applied to the current alert branch; and drive9_feature_enabled / drive9_module_up come from the same scrape target, so the and on (namespace, pod) join contract holds.
What convinced me on the two blocking items
Work-mask allocation. Moving the bits to pkg/meta as 1 << iota and reducing backend.BackendWork* / server.Work* to aliases is a better fix than the assertion I asked for — it removes the possibility of divergence rather than detecting it after the fact. BackendWorkMetricsCleanup existing solely to be asserted, with an explicit "backend does not emit it" comment, is the right call. The phantom "Quota" bit references in the surrounding comments are gone too.
Quota snapshot path. ReplaceTenantQuotaSnapshotWithOrg is O(1) with a single registry lock acquisition and exact-label writes; deleteGaugeByLabel is gone. Two things beyond what I raised are also correct: caching last-known limits in Registry.tenantQuotaSnapshots so the three call sites stop clobbering each other's state="limit" series, and deleting the prior org's label set on relabel. Because it is one critical section with no intermediate delete, the scrape-window gap is closed rather than narrowed.
On the last two commits
c572e58e — the signature narrowing landed properly rather than being silenced. All seven _ = param placeholders are gone, action removal propagated correctly through tenantInFlightKey / adjustTenantInFlight / setRequestMetricTenant, and keeping state.action = action on the in-flight move path is right since action still feeds tenant_requests_total. Dropping tenantRequestResult once nothing consumed it is good hygiene. The transaction-boundary rationale on UpdateTenantStatus / UpdateTenantStatusIf is a better answer than batching — per-tenant transactions keeping failure isolation in loops that interleave external cleanup work is the correct tradeoff, and having it written down is what I wanted.
e13d5e98 — this closes the idle-reap issue I had twice marked as out of scope. With DeleteTenantCounters removed from Pool.closeEntry, the only two call sites are the outbox cleanup-bit dispatch and clearLocalTenantMetrics, both lifecycle-driven, so a 2-minute idle reap no longer deletes counters for a live tenant and the increase() under-counting on per-tenant alerts is fixed. TestCloseEntryPreservesLiveTenantCounters pins it.
Also confirmed the metrics-cleanup outbox row is written transactionally at all nine insertTenantNotifyTx sites, so cleanup delivery cannot be lost between the status transition and the signal.
Independent verification on e13d5e98
go build ./...cleango vet ./pkg/...cleanpkg/metrics,pkg/server,pkg/backend,pkg/meta,pkg/tenantall pass with-count=1-raceclean onpkg/metricsand on the coalescer / outbox / semantic-worker tests inpkg/server- Alert cross-check: every
drive9_*metric referenced by runbooksc52b6b7bexists in code, and the four previously false-green alerts were replaced rather than dropped - GitHub
cipass,drive9-server-local e2e smokepass
Remaining follow-ups (correctly out of scope here)
- The namespace/container audit for the k8s-level rules across all seven environments.
- Tenant-pool ref-leak detection, still documented as uncovered in the rules file.
Nothing blocking from me.
mornyx
left a comment
mornyx
left a comment
There was a problem hiding this comment.
Deep review across six slices (cardinality/lifecycle, new bounded metrics, OpenAPI contract, DB pool reduction, event-bus dedup, gauge deletion plumbing) with findings traced end-to-end. Approving: the failure-only per-tenant counter + whitelisted business events give a real cardinality bound; the WorkMetricsCleanup broadcast is transactional with every status transition, idempotent, and its cursor/prune semantics are correct; the OpenAPI api/operation/result contract has exactly-once, mutually-exclusive classification (verified against every return path of all 10 HTTPClustersAPI methods + the digest doer); gauge registry locking and label-key normalization are consistent. Three worth-fixing items and a handful of nits inline; also two deploy notes: the label rename (path→api, module tidbcloud_quota→tidbcloud_openapi) breaks existing dashboards keyed on the old shape, and drive9_event_bus_publish_errors_total had a live caller before this PR — any alert on it goes silent until the runbooks PR lands.
|
Worth-fixing (anchored at
Consider bucketing requests that fail auth/tenant-status without tenant attribution (e.g. |
adversary-2 review — GREEN / APPROVE (N1/N2 tradeoffs to confirm before merge)B1 coupling (the #1 question from #801): RESOLVED. Cross-pod cleanup correct: cleanup bit is broadcast (not sharded); every pod's poller deletes local series idempotently. Delivery is now reliable — coalescer re-queues failed rows via New metrics bounded: Notes (confirm before merge; not code blockers)
Verdict: GREEN. B1 genuinely resolved. #801 + #802 must land in the same merge result (plus #801's B2 quota-cache panic-path fix). |
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Requesting changes on exact head e13d5e985c292d912fde8c26801fd355c0848090.
This PR does fix the #801 request-path contention class: Pool.closeEntry no longer calls metrics.DeleteTenantCounters, and the lifecycle/outbox cleanup path is transactionally tied to deleting/deleted transitions. I am not blocking on the general N1/N2 tradeoffs about live-tenant series retention or per-tenant service-operation denominators. The blockers below are concrete metric correctness failures still present on this head.
-
Deleted/revoked tenant traffic can recreate the exact tenant series cleanup just removed.
tenantOutboxPoller.dispatchdeletes tenant counters once forWorkMetricsCleanup, but auth sets tenant metric scope atpkg/server/auth.go:258beforeAPIKeyActiveand before the tenant-status switch, and/v1/statusdoes the same atpkg/server/server.go:2066.resolveByAPIKeyHashDBreturns rows byjwt_hashwithout filtering key or tenant status, andrecordTenantHTTPRequestrecords tenant-scoped counters/bytes for every final response once scope is present. Result: after cleanup runs, a stale SDK/FUSE heartbeat or any client retrying a revoked/deleted key can recreatedrive9_tenant_requests_totaland bytes indefinitely, with no later sweep now that pool eviction cleanup was removed. Existing tests prove deletion removes current series; they do not prove cleanup remains sticky against rejected post-delete traffic. Required before merge: fail-closed behavior for inactive/deleting/deleted auth attempts, or an explicit recently-cleaned suppression/re-delete mechanism, plus a regression that cleanup followed by revoked/deleted-key traffic does not reintroduce tenant-labeled series. -
The reduced DB pool metric can go false-green when an unscoped user pool is open. Fork/delete flows still open tenant stores through
datastore.Open(pkg/server/fork.go:647,pkg/server/fork.go:1080), which registers an unscopedrole="user"pool viapkg/datastore/store.go:160-180. Inpkg/metrics/db.go:361-438, tenant-scoped pools are accumulated intotenantRoleTotals, but the synthetic role aggregate is skipped whenever an unscoped same-role key exists. That meansdrive9_db_pool_connections{role="user",state="max_open"}can report only the transient unscoped fork/delete pool and exclude all tenant-scoped pool capacity, exactly while this PR removes per-tenantmax_openseries. The current mixed-pool test only asserts one role-only line exists; it does not assert the value includes tenant-scoped totals. Required before merge: either scope those fork/delete stores or make the role aggregate include both scoped and unscoped capacity, with a value-based regression. -
TiDB Cloud delete failures are counted as
okfor unexpected 2xx responses.DeleteClusterandDeleteBranchreturn success only for 200/204/404, but for any other status they callrecordTiDBCloudHTTPResponse(..., responseValid=true)before returning an error.tiDBCloudHTTPResultmaps every 2xx withresponseValid=truetook, so a 202 Accepted or other unexpected 2xx is both a caller-visible error and anokOpenAPI metric. That violates the newapi/operation/resultcontract and can hide upstream protocol drift. Required before merge: classify unexpected success-class responses that still return errors as protocol/error results, with delete-cluster/delete-branch regressions. -
shared_db_pool_status_age_secondsis not actually age in status.tenant_metrics.go:109-114derives non-active status age fromdb_pool.updated_at, but the schema definesupdated_at ... ON UPDATE CURRENT_TIMESTAMP(3)and unrelated writes such as tenant-count/soft-cap/spending-limit repair also update the row. A stuck provisioning/deleting pool can therefore reset to age 0 without a status transition, making any age alert false-green. Required before merge if this metric is part of the runbook contract: store/use status-transition time or rename/redefine the metric and alerts so it is not presented as status age.
Validation I ran locally: git diff --check clean; changed Go files gofmt -l clean; go vet ./pkg/metrics ./pkg/meta ./pkg/server ./pkg/backend ./pkg/tenant ./pkg/tenant/tidbcloudnative clean; go build ./... clean; go test ./pkg/metrics ./pkg/tenant/tidbcloudnative -count=1 passed; test binaries compile for pkg/meta, pkg/server, pkg/backend, and pkg/tenant. Full local tests for the Docker-backed packages are blocked on this machine by panic: rootless Docker not found. Hosted ci and drive9-server-local e2e smoke are green.
adversary-2 — retracting GREEN, REVISE (endorsing @adversary-1's blockers)I verified the two most impactful of @adversary-1's four findings; both are real metric-correctness defects, harder than my earlier N1/N2 tradeoffs. My gap: I focused on the B1 request-path coupling (the headline) and cardinality bounds, not the per-metric result classification and re-introduction paths. B3 confirmed — B1 confirmed — auth sets tenant metric scope at B2 / B4 — analysis paths credible (fork/delete opens an unscoped Verdict: REVISE. B1 coupling is genuinely resolved (my prior finding stands), but these four metric-correctness defects block merge. #801 + #802 still need the same merge result. Aligned with @adversary-1. |
adversary-2 R2 at
|
|
Current-head review disposition for the auth/revocation finding:
The four concrete blockers in the earlier formal review were fixed in |
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
adversary-1 R2 at 5e9af58e42779a11e9213dd1d4f0eb607723637b
Approved. I re-anchored after qiffang's latest request; there is no newer head after 5e9af58e, and both hosted code-ci and drive9-server-local e2e smoke are green on this exact commit.
The four prior metric-correctness blockers are closed:
-
Cleanup-then-rejected traffic no longer recreates deleted tenant series. The auth paths now set tenant request metric scope only after active-tenant authorization (
setRequestMetricTenantForAuthStatus), and revoked-key cleanup is broadcast through durableWorkAPIKeyCacheCleanupoutbox rows so peer pods invalidate cached API-key resolutions without deleting live tenant metrics. -
DB role aggregate no longer false-greens when unscoped fork/delete pools coexist with tenant-scoped pools. Fork/delete now use
datastore.OpenForTenant, andpkg/metrics/db.gofolds unscoped saturation into the role aggregate instead of letting the unscoped row hide tenant-scoped capacity. The regression checks value, not just line count. -
TiDB Cloud delete unexpected 2xx responses no longer record
ok.DeleteClusterandDeleteBranchnow passresponseValid=falseon fall-through, so a 202-style unexpected delete success recordsprotocol_error; the regression covers both operations. -
shared_db_pool_status_age_secondsnow uses true status-transition age.db_pool.status_updated_atis backfilled and updated only on status transitions, and metrics useStatusUpdatedAtinstead ofupdated_at, so non-status writes cannot reset the age.
Notes: The remaining #801/#802 coupling is a merge-order/composition gate, not a blocker inside #802 itself. #801 still must not land its 20x cap unless this PR or equivalent metrics cleanup is already in the tested merge result; the current exact #801 + #802 synthetic merge still conflicts in pkg/tenant/pool_test.go, so whoever lands both must resolve and retest that combined state.
Validation performed:
- Passed locally:
git diff --check origin/main...HEAD, changed-filegofmt -l,go test ./pkg/metrics ./pkg/tenant/tidbcloudnative -count=1, test binary compilation forpkg/meta,pkg/server,pkg/backend,pkg/tenant,go vet ./pkg/metrics ./pkg/meta ./pkg/server ./pkg/backend ./pkg/tenant ./pkg/tenant/tidbcloudnative, andgo build ./.... - Hosted
code-ci,drive9-server-local e2e smoke, and CodeRabbit are green on5e9af58e.
Summary
Alert consumers
The matching alert and rule-test changes were pushed to tidbcloud/runbooks#2688. They replace false-green SSE, extraction, semantic-worker, background-worker, and Vault expressions and add alerts for the new critical-path metrics.
Validation so far
Scope notes