keyspace: add optional keyspace info metric #11176
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,7 @@ | |
| // Config is the interface for keyspace config. | ||
| type Config interface { | ||
| GetPreAlloc() []string | ||
| IsKeyspaceLevelMetricsEnabled() bool | ||
| ToWaitRegionSplit() bool | ||
| GetWaitRegionSplitTimeout() time.Duration | ||
| GetCheckRegionSplitInterval() time.Duration | ||
|
|
@@ -242,7 +243,13 @@ | |
|
|
||
| // UpdateConfig update keyspace manager's config. | ||
| func (manager *Manager) UpdateConfig(cfg Config) { | ||
| keyspaceInfoMetricsMu.Lock() | ||
| wasEnabled := manager.config.IsKeyspaceLevelMetricsEnabled() | ||
| manager.config = cfg | ||
| if wasEnabled && !cfg.IsKeyspaceLevelMetricsEnabled() { | ||
| resetKeyspaceInfoMetricsLocked() | ||
| } | ||
| keyspaceInfoMetricsMu.Unlock() | ||
| if manager.mgm != nil { | ||
| manager.mgm.updateGroups(cfg.GetMetaServiceGroups()) | ||
| } | ||
|
|
@@ -392,6 +399,7 @@ | |
| zap.Uint32("keyspace-id", keyspace.GetId()), | ||
| zap.String("keyspace-name", keyspace.GetName()), | ||
| ) | ||
| manager.UpdateKeyspaceInfoMetrics(keyspace) | ||
| return keyspace, nil | ||
| } | ||
|
|
||
|
|
@@ -529,6 +537,7 @@ | |
| zap.String("keyspace-name", keyspace.GetName()), | ||
| zap.Any("keyspace", keyspace), | ||
| ) | ||
| manager.UpdateKeyspaceInfoMetrics(keyspace) | ||
| return keyspace, nil | ||
| } | ||
|
|
||
|
|
@@ -770,6 +779,9 @@ | |
| if manager.mgm != nil && meta != nil { | ||
| manager.mgm.AttachEndpoints(meta.GetConfig()) | ||
| } | ||
| if err == nil { | ||
| manager.UpdateKeyspaceInfoMetrics(meta) | ||
| } | ||
| return meta, err | ||
| } | ||
|
|
||
|
|
@@ -793,6 +805,9 @@ | |
| if manager.mgm != nil && meta != nil { | ||
| manager.mgm.AttachEndpoints(meta.GetConfig()) | ||
| } | ||
| if err == nil { | ||
| manager.UpdateKeyspaceInfoMetrics(meta) | ||
|
nolouch marked this conversation as resolved.
Outdated
Member
There was a problem hiding this comment. Blocking: a load that read before deletion can permanently recreate the deleted series. The read transaction finishes before this metric update. A concurrent The metric publication needs ordering or version validation against removal. A second unsynchronized existence check would leave the same check-then-write race. Unit-test reproducerAdd type blockingKeyspaceStorage struct {
endpoint.KeyspaceStorage
blockNext atomic.Bool
reached chan struct{}
release chan struct{}
}
func (s *blockingKeyspaceStorage) RunInTxn(ctx context.Context, f func(kv.Txn) error) error {
err := s.KeyspaceStorage.RunInTxn(ctx, f)
if s.blockNext.CompareAndSwap(true, false) {
close(s.reached)
<-s.release
}
return err
}
func (suite *keyspaceTestSuite) TestStaleLoadCannotRestoreRemovedInfoMetric() {
re := suite.Require()
suite.manager.UpdateConfig(&mockConfig{EnableKeyspaceLevelMetrics: true})
resetKeyspaceInfoMetrics()
created, err := suite.manager.CreateKeyspace(&CreateKeyspaceRequest{
Name: "review_stale_load", CreateTime: time.Now().Unix(),
})
re.NoError(err)
for _, state := range []keyspacepb.KeyspaceState{
keyspacepb.KeyspaceState_DISABLED,
keyspacepb.KeyspaceState_ARCHIVED,
keyspacepb.KeyspaceState_TOMBSTONE,
} {
_, err = suite.manager.UpdateKeyspaceState(created.GetName(), state, time.Now().Unix())
re.NoError(err)
}
store := &blockingKeyspaceStorage{
KeyspaceStorage: suite.manager.store,
reached: make(chan struct{}),
release: make(chan struct{}),
}
suite.manager.store = store
store.blockNext.Store(true)
loadDone := make(chan error, 1)
go func() {
_, loadErr := suite.manager.LoadKeyspaceByID(created.GetId())
loadDone <- loadErr
}()
<-store.reached
re.NoError(suite.manager.RemoveKeyspace(created.GetId()))
re.Equal(0, promtestutil.CollectAndCount(keyspaceInfo))
close(store.release)
re.NoError(<-loadDone)
re.Equal(0, promtestutil.CollectAndCount(keyspaceInfo))
}Run: On
Contributor
Author
There was a problem hiding this comment. This metric is now defined as a client-observed mapping, not a strongly consistent metadata snapshot. A concurrent deletion may leave a previously observed mapping temporarily, but deleted keyspaces won’t have active storage-size series to join with. Avoiding this window would require extra lifecycle synchronization, which we want to avoid here.
Member
There was a problem hiding this comment. The current head makes this broader than the concurrent window described above: no successful removal path deletes an observed series at all. func (suite *keyspaceTestSuite) TestRemovalClearsObservedInfoMetric() {
re := suite.Require()
resetKeyspaceInfoMetrics()
suite.T().Cleanup(resetKeyspaceInfoMetrics)
created, err := suite.manager.CreateKeyspace(&CreateKeyspaceRequest{
Name: "review_rm_metric", CreateTime: time.Now().Unix(),
})
re.NoError(err)
SetKeyspaceInfoMetrics(created.GetId(), created.GetName())
re.Equal(1, testutil.CollectAndCount(keyspaceInfo))
for _, state := range []keyspacepb.KeyspaceState{
keyspacepb.KeyspaceState_DISABLED,
keyspacepb.KeyspaceState_ARCHIVED,
} {
_, err = suite.manager.UpdateKeyspaceStateByID(
created.GetId(), state, time.Now().Unix())
re.NoError(err)
}
re.NoError(suite.manager.store.RunInTxn(suite.ctx, func(txn kv.Txn) error {
return suite.manager.RemoveKeyspace(txn, created.GetId())
}))
re.Equal(0, testutil.CollectAndCount(keyspaceInfo))
}Run: On This is explicitly required by Issue #11175. The fact that another storage-size metric may currently have no sample does not make stale high-cardinality labels safe: ID reuse makes the stale series ambiguous, and repeated create/remove cycles retain series indefinitely. Please restore post-commit cleanup. |
||
| } | ||
| return meta, err | ||
| } | ||
|
|
||
|
|
@@ -1035,33 +1050,77 @@ | |
| return meta, nil | ||
| } | ||
|
|
||
| // RemoveKeyspace removes the keyspace specified by id and clears its caches after the transaction commits. | ||
| func (manager *Manager) RemoveKeyspace(id uint32) error { | ||
| var removed *keyspacepb.KeyspaceMeta | ||
| err := manager.store.RunInTxn(manager.ctx, func(txn kv.Txn) error { | ||
| var err error | ||
| removed, err = manager.stageRemoveKeyspace(txn, id) | ||
| return err | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| manager.finishRemoveKeyspace(removed) | ||
| return nil | ||
| } | ||
|
|
||
| // stageRemoveKeyspace stages keyspace removal and returns its metadata for post-commit cleanup. | ||
| func (manager *Manager) stageRemoveKeyspace(txn kv.Txn, id uint32) (*keyspacepb.KeyspaceMeta, error) { | ||
| manager.metaLock.Lock(id) | ||
| defer manager.metaLock.Unlock(id) | ||
| if isProtectedKeyspaceID(id) { | ||
| return nil, newModifyProtectedKeyspaceError() | ||
| } | ||
| meta, err := manager.store.LoadKeyspaceMeta(txn, id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if meta == nil { | ||
| return nil, errs.ErrKeyspaceNotFound | ||
| } | ||
| if meta.GetState() == keyspacepb.KeyspaceState_ENABLED || meta.GetState() == keyspacepb.KeyspaceState_DISABLED { | ||
| return nil, errors.Errorf("cannot remove keyspace in state %s", meta.GetState().String()) | ||
| } | ||
| err = manager.store.RemoveKeyspace(txn, id, meta.GetName()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Keep the meta-service group assignment accounting in sync within the same | ||
| // txn. Without this, removed keyspaces leak count and could permanently block | ||
| // deleting an otherwise-empty group. | ||
| if err := manager.unassignKeyspaceFromMetaServiceGroup(txn, meta); err != nil { | ||
| return nil, err | ||
| } | ||
| return meta, nil | ||
| } | ||
|
|
||
| // finishRemoveKeyspace clears the caches and metrics after a keyspace removal transaction commits. | ||
| func (manager *Manager) finishRemoveKeyspace(meta *keyspacepb.KeyspaceMeta) { | ||
| manager.keyspaceNameLookup.Delete(meta.GetId()) | ||
| manager.keyspaceStateLookup.Delete(meta.GetId()) | ||
| keyspaceInfoMetricsMu.Lock() | ||
| defer keyspaceInfoMetricsMu.Unlock() | ||
| if manager.config.IsKeyspaceLevelMetricsEnabled() { | ||
| deleteKeyspaceInfoMetricsLocked(meta.GetId(), meta.GetName()) | ||
| } | ||
| } | ||
|
|
||
| // UpdateKeyspaceInfoMetrics updates the keyspace ID-to-name mapping metric when keyspace-level metrics are enabled. | ||
| func (manager *Manager) UpdateKeyspaceInfoMetrics(meta *keyspacepb.KeyspaceMeta) { | ||
| keyspaceInfoMetricsMu.Lock() | ||
| defer keyspaceInfoMetricsMu.Unlock() | ||
| if meta == nil || !manager.config.IsKeyspaceLevelMetricsEnabled() { | ||
|
nolouch marked this conversation as resolved.
Outdated
|
||
| return | ||
| } | ||
| setKeyspaceInfoMetricsLocked(meta.GetId(), meta.GetName()) | ||
| } | ||
|
|
||
| // DeleteKeyspaceInfoMetrics removes the cached keyspace info series for id. | ||
| func (manager *Manager) DeleteKeyspaceInfoMetrics(id uint32) { | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| keyspaceInfoMetricsMu.Lock() | ||
| defer keyspaceInfoMetricsMu.Unlock() | ||
| deleteKeyspaceInfoMetricsByIDLocked(id) | ||
| } | ||
|
|
||
| // UpdateKeyspaceStateByID updates target keyspace to the given state if it's not already in that state. | ||
|
|
@@ -1190,6 +1249,9 @@ | |
| } | ||
| } | ||
| } | ||
| for _, meta := range keyspaces { | ||
| manager.UpdateKeyspaceInfoMetrics(meta) | ||
| } | ||
| return keyspaces, nil | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: the metric is published later than the metadata it represents.
UpdateKeyspaceStateByIDhas already committed anENABLEDmetadata record beforeUpdateKeyspaceForGroupruns. If that separate group write fails, this method returns here without rolling the metadata back and without reaching the metric update below. The durable keyspace cannot be retried under the same name, yet its mapping remains absent until an unrelated load or watch happens.CreateKeyspaceByIDhas the same ordering.Please publish once the metadata becomes durable and remove the series only when a rollback deletion commits, or make every subsequent failure roll the metadata back. Whether a client has opened
WatchKeyspacesshould not change the result.Unit-test reproducer
Run:
On
247ace82be, the persisted state isENABLED, but the final assertion reportsexpected: 1, actual: 0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer depends on the create flow. The metric is now populated from successful
GetAllKeyspacesandLoadKeyspaceByIDrequests, so partial create failures won’t directly update it.