Add epoch version & nodes count sensors in NodeBroker & DynamicNameserver #20625
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR instruments the NodeBroker and DynamicNameserver components with new metrics, including epoch version and active/expired/removed node counts.
- Extended
counters_node_broker.protowith four new simple counters. - Added
UpdateCommittedStateCountersinTNodeBrokerand invoked it after state-changing transactions. - Introduced metric registration and
UpdateCounterslogic inTDynamicNameserver, plus HTML reporting of protocol state.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ydb/core/protos/counters_node_broker.proto | Added COUNTER_EPOCH_VERSION, ACTIVE_NODES, EXPIRED_NODES, REMOVED_NODES. |
| ydb/core/mind/node_broker_impl.h | Declared UpdateCommittedStateCounters(). |
Multiple node_broker__*.cpp transaction files |
Inserted calls to UpdateCommittedStateCounters() after each commit. |
| ydb/core/mind/node_broker.cpp | Implemented UpdateCommittedStateCounters(). |
| ydb/core/mind/dynamic_nameserver_mon.cpp | Added ToString(EProtocolState) and rendered protocol state in HTML. |
| ydb/core/mind/dynamic_nameserver_impl.h | Declared counters and UpdateCounters(). |
| ydb/core/mind/dynamic_nameserver.cpp | Registered counters, embedded calls to UpdateCounters(), and implemented it. |
Comments suppressed due to low confidence (3)
ydb/core/mind/dynamic_nameserver.cpp:248
- The counter key string "ExpireDynamicNodes" is missing a trailing 'd' compared to the variable name
ExpiredDynamicNodesCounter. Consider renaming the counter to "ExpiredDynamicNodes" for consistency.
ExpiredDynamicNodesCounter = counters->GetCounter("ExpireDynamicNodes");
ydb/core/protos/counters_node_broker.proto:13
- New counters are added in the proto but there are no corresponding unit or integration tests to validate their registration and values. Consider adding tests to cover these metrics.
COUNTER_EPOCH_VERSION = 3 [(CounterOpts) = {Name: "EpochVersion"}];
ydb/core/mind/node_broker.cpp:789
- [nitpick] It would be helpful to add a brief comment above this method describing the purpose of each counter (
ActiveNodes,ExpiredNodes,RemovedNodes,EpochVersion) and when this method should be invoked.
void TNodeBroker::UpdateCommittedStateCounters() {
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Changelog category
Description for reviewers
Этот PR добавляет в компоненты
NodeBrokerиDynamicNameserverновые метрики:Основные изменения:
counters_node_broker.protoчетырьмя новыми простыми счетчикамиUpdateCommittedStateCountersвTNodeBroker, который вызывается после транзакций, изменяющих состояниеUpdateCountersвTDynamicNameserver, который вызывается после изменения состояния