feat: add Prometheus metrics #8728

VanBarbascu · 2023-03-14T15:04:55Z

Current block height since the start of the current epoch
shards tracked by the current node. (currently all shards but in the feature it might change)
whether the node is a block producer
whether the node is a chunk producer and for which shard

Tested on localnet and mainnet on a canary rpc node.
The gist of metrics from the node in prod. link
For additional test results check each commit message

VanBarbascu · 2023-03-15T17:31:11Z

I have removed the number of expected blocks/chunks to be produced in current epoch item and I will create a different PR for it.

wacban

looks good overall, just a few nits here and there

wacban · 2023-03-16T08:50:56Z

CHANGELOG.md

@@ -8,6 +8,8 @@

 * Experimental option to dump state of every epoch to external storage. [#8661](https://github.com/near/nearcore/pull/8661)
 * State-viewer tool to dump and apply state changes from/to a range of blocks [#8628](https://github.com/near/nearcore/pull/8628)
+* Add prometheus metrics for tacked shards, block height within epoch, if is block/chunk producer


tick tack, it should say "track"
Also what exactly do you mean by block height within epoch?

wacban · 2023-03-16T08:54:59Z

chain/client/src/client_actor.rs

                last_final_block_height,
                last_final_ds_block_height,
                epoch_height,
+                block_height_within_epoch,


nit: For consistency I would rename block_height_within_epoch -> last_final_block_height_in_epoch and maybe move it up so the all block heights are together.

wacban · 2023-03-16T08:56:04Z

chain/client/src/info.rs

@@ -119,6 +120,57 @@ impl InfoHelper {
        metrics::FINAL_BLOCK_HEIGHT.set(last_final_block_height as i64);
        metrics::FINAL_DOOMSLUG_BLOCK_HEIGHT.set(last_final_ds_block_height as i64);
        metrics::EPOCH_HEIGHT.set(epoch_height as i64);
+        metrics::BLOCK_HEIGHT_WITHIN_EPOCH.set(block_height_within_epoch as i64);


ditto, how about FINAL_BLOCK_HEIGHT_IN_EPOCH ?

wacban · 2023-03-16T08:56:49Z

chain/client/src/info.rs

+        metrics::BLOCK_HEIGHT_WITHIN_EPOCH.set(block_height_within_epoch as i64);
+    }
+
+    fn record_tracked_shards(head: &Tip, client: &crate::client::Client) {


nit: can you add a small comment?

wacban · 2023-03-16T08:58:50Z

chain/client/src/info.rs

+
+    fn record_tracked_shards(head: &Tip, client: &crate::client::Client) {
+        if let Ok(num_shards) = client.runtime_adapter.num_shards(&head.epoch_id) {
+            (0..num_shards).for_each(|shard_id| {


nit: personally I think for loops are much more readable but that's just my opinion, up to you
for shard_id in (0..num_shards) {

+1 to @wacban . For-each is unnecessary here.

wacban · 2023-03-16T09:04:34Z

chain/client/src/info.rs

+            let epoch_info = client.runtime_adapter.get_epoch_info(&head.epoch_id);
+            epoch_info.map(|epoch_info| {
+                for (shard_id, validators) in
+                    epoch_info.chunk_producers_settlement().into_iter().enumerate()


Not 100% sure but I think you may not need enumerate and the for loop should handle it.

That array holds the validators for each shard_id. I think that this is the easiest way to get the shard_id.

wacban · 2023-03-16T09:08:21Z

chain/client/src/info.rs

+                    epoch_info.chunk_producers_settlement().into_iter().enumerate()
+                {
+                    let is_chunk_producer_for_shard = validators.iter().any(|&validator_id| {
+                        epoch_info.validator_account_id(validator_id).clone() == account_id


Do you need that clone just for the comparison?

No, Thanks for catching this!

wacban · 2023-03-16T09:09:22Z

chain/client/src/metrics.rs

+pub(crate) static IS_CHUNK_PRODUCER_FOR_SHARD: Lazy<IntGaugeVec> = Lazy::new(|| {
+    try_create_int_gauge_vec(
+        "near_is_chunk_producer_for_shard",
+        "Number of blocks expected to be produced by a validator",


Is that so? :)

and please clarify that it's for the current epoch.

nikurt · 2023-03-16T09:20:33Z

chain/client/src/client_actor.rs

@@ -1280,6 +1280,12 @@ impl ClientActor {
                .runtime_adapter
                .get_epoch_height_from_prev_block(block.hash())
                .unwrap_or(0);
+            let epoch_start_height = self


This function does a tiny bit of work, and a whole lot of metrics work. Can that be moved to a separate function?
Also move the last_final_stuff into that function.

nikurt · 2023-03-16T09:22:14Z

chain/client/src/info.rs

+
+    fn record_tracked_shards(head: &Tip, client: &crate::client::Client) {
+        if let Ok(num_shards) = client.runtime_adapter.num_shards(&head.epoch_id) {
+            (0..num_shards).for_each(|shard_id| {


+1 to @wacban . For-each is unnecessary here.

nikurt · 2023-03-16T09:22:45Z

chain/client/src/info.rs

+                    false,
+                );
+                metrics::TRACKED_SHARDS
+                    .with_label_values(&[shard_id.to_string().as_str()])


Suggested change

.with_label_values(&[shard_id.to_string().as_str()])

.with_label_values(&[&shard_id.to_string()])

nikurt · 2023-03-16T09:27:57Z

chain/client/src/info.rs

+    fn record_block_producers(head: &Tip, client: &crate::client::Client) {
+        let me = client.validator_signer.as_ref().map(|x| x.validator_id().clone());
+        let is_bp = me
+            .map(|account_id| {


Would it be better to replace map(stuff).unwrap_or(false) with map_or(false, stuff)?

nikurt · 2023-03-16T09:29:29Z

chain/client/src/info.rs

+        let me = client.validator_signer.as_ref().map(|x| x.validator_id().clone());
+        me.map(|account_id| {
+            let epoch_info = client.runtime_adapter.get_epoch_info(&head.epoch_id);
+            epoch_info.map(|epoch_info| {


More natural way would be:

if let Ok(epoch_info) = get_epoch_info() { do stuff with epoch_info }

Maybe do the same for let me =

nikurt · 2023-03-16T09:32:15Z

chain/client/src/info.rs

+                    let is_chunk_producer_for_shard = validators.iter().any(|&validator_id| {
+                        epoch_info.validator_account_id(validator_id).clone() == account_id
+                    });
+                    metrics::IS_CHUNK_PRODUCER_FOR_SHARD


Please update the metric unconditionally. Currently it's only updated if the node has a validator key and if there is an epoch. Don't want to accidentally miss setting the metric to 0.

Good catch!

nikurt · 2023-03-16T09:33:25Z

chain/client/src/metrics.rs

+pub(crate) static IS_BLOCK_PRODUCER: Lazy<IntGauge> = Lazy::new(|| {
+    try_create_int_gauge(
+        "near_is_block_producer",
+        "Bool to denote if the node is currently a block producer",


currently is ambiguous. This block? This epoch?

nikurt · 2023-03-16T09:33:55Z

chain/client/src/metrics.rs

+pub(crate) static IS_CHUNK_PRODUCER_FOR_SHARD: Lazy<IntGaugeVec> = Lazy::new(|| {
+    try_create_int_gauge_vec(
+        "near_is_chunk_producer_for_shard",
+        "Number of blocks expected to be produced by a validator",


and please clarify that it's for the current epoch.

We need to export this to improve the blockchain monitoring. Tested on a localnet: Run: python3 nearup run localnet --binary-path ../nearcore/target/debug/ --interactive Open http://127.0.0.1:3030/metrics - # HELP block_height_within_epoch Height of the block within the epoch. - # TYPE block_height_within_epoch gauge - near_block_height_within_epoch 1

* say which shards are tracked from the current node Testing * run a localnet with 3 validators and 3 shards. * go to localhost:3030/metrics - ** skipped lines ** - # HELP near_client_tracked_shards Tracked shards - # TYPE near_client_tracked_shards gauge - near_client_tracked_shards{shard_id="0"} 1 - near_client_tracked_shards{shard_id="1"} 1 - near_client_tracked_shards{shard_id="2"} 1 - ** skipped lines **

* say which shards is this node producing chunks for Testing * run a localnet with 10 validators and 4 shards. * go to localhost:3030/metrics - # HELP near_is_block_producer Bool to denote if the node is currently a block producer in the current epoch - # TYPE near_is_block_producer gauge - near_is_block_producer 1 - # HELP near_is_chunk_producer_for_shard Bool to denote if the node is a chunk producer for a shard in the current epoch - # TYPE near_is_chunk_producer_for_shard gauge - near_is_chunk_producer_for_shard{shard_id="0"} 0 - near_is_chunk_producer_for_shard{shard_id="1"} 1 - near_is_chunk_producer_for_shard{shard_id="2"} 0 - near_is_chunk_producer_for_shard{shard_id="3"} 0

VanBarbascu

Addressed comments and refactored the process_accepted_blocks function to separate the monitoring logic.

VanBarbascu · 2023-03-16T16:25:26Z

chain/client/src/info.rs

+            let epoch_info = client.runtime_adapter.get_epoch_info(&head.epoch_id);
+            epoch_info.map(|epoch_info| {
+                for (shard_id, validators) in
+                    epoch_info.chunk_producers_settlement().into_iter().enumerate()


That array holds the validators for each shard_id. I think that this is the easiest way to get the shard_id.

VanBarbascu · 2023-03-16T16:25:58Z

chain/client/src/info.rs

+                    epoch_info.chunk_producers_settlement().into_iter().enumerate()
+                {
+                    let is_chunk_producer_for_shard = validators.iter().any(|&validator_id| {
+                        epoch_info.validator_account_id(validator_id).clone() == account_id


No, Thanks for catching this!

VanBarbascu · 2023-03-16T16:38:31Z

chain/client/src/info.rs

+                    let is_chunk_producer_for_shard = validators.iter().any(|&validator_id| {
+                        epoch_info.validator_account_id(validator_id).clone() == account_id
+                    });
+                    metrics::IS_CHUNK_PRODUCER_FOR_SHARD


Good catch!

VanBarbascu · 2023-03-16T16:44:41Z

chain/client/src/metrics.rs

+pub(crate) static IS_CHUNK_PRODUCER_FOR_SHARD: Lazy<IntGaugeVec> = Lazy::new(|| {
+    try_create_int_gauge_vec(
+        "near_is_chunk_producer_for_shard",
+        "Number of blocks expected to be produced by a validator",


nikurt · 2023-03-16T17:13:01Z

CHANGELOG.md

@@ -9,6 +9,8 @@
 * Experimental option to dump state of every epoch to external storage. [#8661](https://github.com/near/nearcore/pull/8661)
 * State-viewer tool to dump and apply state changes from/to a range of blocks [#8628](https://github.com/near/nearcore/pull/8628)
 * Node can restart if State Sync gets interrupted [#8732](https://github.com/near/nearcore/pull/8732)
+* Add prometheus metrics for tracked shards, block height within epoch, if is block/chunk producer
+>>>>>>> f4538a8ce (feat: changelog for PR#8728)


merge artifact

VanBarbascu requested a review from a team as a code owner March 14, 2023 15:04

VanBarbascu requested a review from mzhangmzz March 14, 2023 15:04

VanBarbascu force-pushed the master branch 5 times, most recently from b37d0d0 to f4538a8 Compare March 15, 2023 15:57

wacban reviewed Mar 16, 2023

View reviewed changes

nikurt reviewed Mar 16, 2023

View reviewed changes

VanBarbascu force-pushed the master branch from f4538a8 to bc1265b Compare March 16, 2023 16:54

Razvan Barbascu and others added 4 commits March 16, 2023 16:55

feat: changelog for PR#8728

7dd24ba

VanBarbascu commented Mar 16, 2023

View reviewed changes

VanBarbascu force-pushed the master branch from bc1265b to 4420e45 Compare March 16, 2023 17:04

VanBarbascu requested review from wacban and nikurt March 16, 2023 17:11

nikurt approved these changes Mar 16, 2023

View reviewed changes

VanBarbascu force-pushed the master branch 2 times, most recently from 557d152 to 495a828 Compare March 16, 2023 17:31

refactor block processing metrics function

8f9d672

VanBarbascu force-pushed the master branch from 495a828 to 8f9d672 Compare March 16, 2023 17:40

VanBarbascu merged commit 73cb5fb into near:master Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Prometheus metrics #8728

feat: add Prometheus metrics #8728

VanBarbascu commented Mar 14, 2023 •

edited

Loading

VanBarbascu commented Mar 15, 2023

wacban left a comment

wacban Mar 16, 2023

wacban Mar 16, 2023

wacban Mar 16, 2023

wacban Mar 16, 2023

wacban Mar 16, 2023

nikurt Mar 16, 2023

wacban Mar 16, 2023

VanBarbascu Mar 16, 2023

wacban Mar 16, 2023

VanBarbascu Mar 16, 2023

wacban Mar 16, 2023

nikurt Mar 16, 2023

VanBarbascu Mar 16, 2023

nikurt Mar 16, 2023

nikurt Mar 16, 2023

nikurt Mar 16, 2023

nikurt Mar 16, 2023

nikurt Mar 16, 2023

nikurt Mar 16, 2023

VanBarbascu Mar 16, 2023

nikurt Mar 16, 2023

nikurt Mar 16, 2023

VanBarbascu left a comment

VanBarbascu Mar 16, 2023

VanBarbascu Mar 16, 2023

VanBarbascu Mar 16, 2023

VanBarbascu Mar 16, 2023

nikurt Mar 16, 2023

	.with_label_values(&[shard_id.to_string().as_str()])
	.with_label_values(&[&shard_id.to_string()])

feat: add Prometheus metrics #8728

feat: add Prometheus metrics #8728

Conversation

VanBarbascu commented Mar 14, 2023 • edited Loading

VanBarbascu commented Mar 15, 2023

wacban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanBarbascu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanBarbascu commented Mar 14, 2023 •

edited

Loading