*: add keyspace observability labels#68404
Conversation
ti-chi-bot
Bot
added
release-note
|
Hi @zeminzhou. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds keyspace observability: new config schema and resolution, startup PD metadata fetch for NextGen, updates global config with resolved metric labels and optional log fields, integrates resolved values into metrics registration and slow/statement log serialization, and adds tests and BUILD updates. ChangesKeyspace Observability
🎯 4 (Complex) | ⏱️ ~60 minutes Sequence DiagramsequenceDiagram
participant TiDBMain
participant PDClient
participant PD
participant Config
participant MetricsCommon
TiDBMain->>PDClient: create PD client (TLS, timeout)
TiDBMain->>PD: fetch KeyspaceMeta via PDClient
PD->>TiDBMain: KeyspaceMeta
TiDBMain->>Config: prepareKeyspaceObservabilityWithKeyspaceMeta -> UpdateGlobal
Config->>MetricsCommon: GetKeyspaceObservabilityMetricLabels()
MetricsCommon->>MetricsCommon: SetConstLabelsFromMap(merged) and initMetrics()
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/tidb-server/main_test.go`:
- Around line 202-213: The test
TestSetupKeyspaceObservabilityForStartSkipsClassic should be gated to run only
in classic mode to avoid environment-dependent failures: at the start of
TestSetupKeyspaceObservabilityForStartSkipsClassic, call kerneltype.IsClassic()
and if it returns false call t.Skip with a brief reason so the test only
exercises the classic short-circuit path; this change is minimal and keeps the
rest of the test (the config.Update, prepareKeyspaceObservability call, and
assertions) unchanged.
In `@cmd/tidb-server/main.go`:
- Around line 1227-1229: The current copy
(maps.Copy(resolvedValues.MetricLabels, configuredValues.MetricLabels)) allows
user-configured labels to overwrite reserved labels like "keyspace_id" and
"keyspace_name"; change the logic so reserved keys are preserved by filtering
out those keys from configuredValues.MetricLabels before copying (or by copying
then restoring original reserved values from resolvedValues.MetricLabels).
Locate the block around copiedConfig.KeyspaceObservabilityValues.Clone(),
configuredValues.MetricLabels and maps.Copy and implement a filter that excludes
"keyspace_id" and "keyspace_name" (or ensures resolvedValues' reserved entries
are not overwritten) when populating resolvedValues.MetricLabels.
In `@pkg/sessionctx/variable/tests/session_test.go`:
- Around line 388-401: The test currently calls restore := config.RestoreFunc()
but only invokes restore() at the end, which can leave global config mutated if
an earlier assertion fails; after obtaining restore (restore :=
config.RestoreFunc()) call defer restore() or t.Cleanup(restore) immediately so
the global config is always restored even on test failures—ensure this is done
before calling config.UpdateGlobal or require.NoError(t,
conf.ResolveKeyspaceObservability(...)) so the cleanup guarantees restoring the
original config.
In `@pkg/util/metricsutil/common.go`:
- Around line 92-94: GetConstLabels() may return a nil map so cloning and
writing into it can panic; before modifying or copying, ensure you initialize a
non-nil map. In the block that builds labels for defaultKeyspaceLabel (around
metricsutil/common.go where you call maps.Clone(metricscommon.GetConstLabels())
and then set labels[defaultKeyspaceLabel] = fmt.Sprint(keyspaceMeta.GetId())),
replace the direct clone with creating a new map when Clone returns nil (or
always allocate a new map and then maps.Copy into it) and then call
metricscommon.SetConstLabelsFromMap(labels). Do the same for the other
occurrence that uses maps.Copy(): ensure the destination map is non-nil
(allocate make(map[string]string)) before calling maps.Copy or writing keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 40faa2a3-a280-4b6e-a6c1-73fb04b01cb5
📒 Files selected for processing (17)
cmd/tidb-server/BUILD.bazelcmd/tidb-server/main.gocmd/tidb-server/main_test.gopkg/config/BUILD.bazelpkg/config/config.gopkg/config/config.toml.examplepkg/config/config.toml.nextgen.examplepkg/config/config_test.gopkg/metrics/common/wrapper.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/tests/session_test.gopkg/util/metricsutil/BUILD.bazelpkg/util/metricsutil/common.gopkg/util/metricsutil/common_test.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/logger.gopkg/util/stmtsummary/v2/record_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #68404 +/- ##
===========================================================
Coverage ? 77.5998%
===========================================================
Files ? 1963
Lines ? 544613
Branches ? 0
===========================================================
Hits ? 422619
Misses ? 121142
Partials ? 852
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@ChangRui-Ryan: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ChangRui-Ryan, yibin87 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What problem does this PR solve?
Issue Number: close #68405
Problem Summary:
NextGen deployments need keyspace identity labels for metrics. Starter mode also needs configured keyspace metadata values to be resolved at TiDB startup and attached to metrics, slow logs, and statement summary logs.
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores