Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

client/beefy: add more metrics for production visibility #12910

Merged
merged 16 commits into from
Feb 16, 2023
Merged

client/beefy: add more metrics for production visibility #12910

merged 16 commits into from
Feb 16, 2023

Conversation

dharjeezy
Copy link
Contributor

@dharjeezy dharjeezy commented Dec 12, 2022 •
edited by acatangiu
Loading

More metrics for Beefy.

Closes #12893

Polkadot companion: paritytech/polkadot#6706

@dharjeezy
Copy link
Contributor Author

dharjeezy commented Jan 24, 2023 •
edited
Loading

Hello @acatangiu

I have an issue, when i try to implement metric for when no voting session is initialized

I try to include the metrics property in the struct VoterOracle which i will then use to set the metric.
But then, VoterOracle implements the Decode, Encode, and PartialEq which i can't actually impelement for the Metrics struct due to these error

  --> client/beefy/src/metrics.rs:24:24
   |
24 | #[derive(Clone, Debug, Encode, Decode, PartialEq)]
   |                        ^^^^^^ the trait `WrapperTypeEncode` is not implemented for `GenericGauge<substrate_prometheus_endpoint::U64>`
   |
   = help: the following other types implement trait `WrapperTypeEncode`:


  --> client/beefy/src/metrics.rs:50:2
   |
50 | /     /// Trying to set Best Beefy block to old block
51 | |     pub beefy_best_block_to_old_block: Gauge<U64>,
   | |_________________________________________________^ the trait `WrapperTypeDecode` is not implemented for `GenericGauge<substrate_prometheus_endpoint::U64>`
   |

error[E0369]: binary operation `==` cannot be applied to type `GenericGauge<substrate_prometheus_endpoint::U64>`
  --> client/beefy/src/metrics.rs:27:2
   |
24 | #[derive(Clone, Debug, Encode, Decode, PartialEq)]
   |                                        --------- in this derive macro expansion
...
27 |     pub beefy_validator_set_id: Gauge<U64>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)

I am curious how to go about this?

@dharjeezy
fix tests
c4b0bc7
@dharjeezy
Merge branch 'master' of https://github.com/dharjeezy/substrate into …
648670b 
…dharjeezy/beefy-metrics

� Conflicts:
�	client/beefy/src/communication/request_response/incoming_requests_handler.rs
�	client/beefy/src/communication/request_response/outgoing_requests_engine.rs
�	client/beefy/src/import.rs
�	client/beefy/src/worker.rs
@dharjeezy
merge changes
04400a2
@dharjeezy dharjeezy marked this pull request as ready for review January 26, 2023 11:38
@dharjeezy
Copy link
Contributor Author

@acatangiu this is ready for review

@acatangiu acatangiu added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jan 26, 2023
acatangiu
acatangiu reviewed Jan 26, 2023
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I've added some changes and additions suggestions below.

client/beefy/src/communication/request_response/incoming_requests_handler.rs Outdated Show resolved Hide resolved
client/beefy/src/communication/request_response/incoming_requests_handler.rs Outdated Show resolved Hide resolved
client/beefy/src/import.rs Outdated Show resolved Hide resolved
client/beefy/src/import.rs Outdated Show resolved Hide resolved
client/beefy/src/lib.rs Outdated Show resolved Hide resolved
client/beefy/src/metrics.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Show resolved Hide resolved
client/beefy/src/metrics.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Show resolved Hide resolved
client/beefy/src/worker.rs Show resolved Hide resolved
@acatangiu acatangiu mentioned this pull request Feb 13, 2023
Merged
serban300
serban300 reviewed Feb 14, 2023
View reviewed changes
Copy link
Contributor

@serban300 serban300 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ! Just 2 nits so far, but I'll do another pass on the PR later. Just want to look more carefully on the places where the metrics are updated.

client/beefy/src/communication/request_response/incoming_requests_handler.rs Outdated Show resolved Hide resolved
serban300
serban300 reviewed Feb 14, 2023
View reviewed changes
client/beefy/src/worker.rs Show resolved Hide resolved
client/beefy/src/worker.rs Show resolved Hide resolved
Lederstrumpf
Lederstrumpf approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@Lederstrumpf Lederstrumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

client/beefy/src/metrics.rs Show resolved Hide resolved
client/beefy/src/metrics.rs Outdated Show resolved Hide resolved
client/beefy/src/worker.rs Outdated Show resolved Hide resolved
@acatangiu
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 46c1bd5 into paritytech:master Feb 16, 2023
rossbulat pushed a commit that referenced this pull request Feb 19, 2023
@dharjeezy @acatangiu
client/beefy: add more metrics for production visibility (#12910)
b4cae2f 
* few beefy metrics

* more beefy metrics

* some beefy metrics

* some beefy metrics

* more metrics

* other metrics

* fix tests

* merge changes

* Apply suggestions from code review

* client/beefy: fix metrics

* client/beefy: separate metrics per component, avoid double registering

* client/beefy: deduplicate metrics registration code

* remove unused metric

* impl review suggestions

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
rossbulat pushed a commit that referenced this pull request Feb 19, 2023
Revert "client/beefy: add more metrics for production visibility (#12910
9e47aab 
)"

This reverts commit b4cae2f.
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
@dharjeezy @acatangiu @ltfschoen
client/beefy: add more metrics for production visibility (paritytech#…
034562c 
…12910)

* few beefy metrics

* more beefy metrics

* some beefy metrics

* some beefy metrics

* more metrics

* other metrics

* fix tests

* merge changes

* Apply suggestions from code review

* client/beefy: fix metrics

* client/beefy: separate metrics per component, avoid double registering

* client/beefy: deduplicate metrics registration code

* remove unused metric

* impl review suggestions

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@dharjeezy @acatangiu @ark0f
client/beefy: add more metrics for production visibility (paritytech#…
6001a32 
…12910)

* few beefy metrics

* more beefy metrics

* some beefy metrics

* some beefy metrics

* more metrics

* other metrics

* fix tests

* merge changes

* Apply suggestions from code review

* client/beefy: fix metrics

* client/beefy: separate metrics per component, avoid double registering

* client/beefy: deduplicate metrics registration code

* remove unused metric

* impl review suggestions

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Ank4n pushed a commit that referenced this pull request Feb 28, 2023
@dharjeezy @acatangiu @Ank4n
client/beefy: add more metrics for production visibility (#12910)
ea3de0e 
* few beefy metrics

* more beefy metrics

* some beefy metrics

* some beefy metrics

* more metrics

* other metrics

* fix tests

* merge changes

* Apply suggestions from code review

* client/beefy: fix metrics

* client/beefy: separate metrics per component, avoid double registering

* client/beefy: deduplicate metrics registration code

* remove unused metric

* impl review suggestions

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@kiltbot kiltbot mentioned this pull request Apr 3, 2023
Closed
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
@dharjeezy @acatangiu @ukint-vs
client/beefy: add more metrics for production visibility (paritytech#…
ff75c59 
…12910)

* few beefy metrics

* more beefy metrics

* some beefy metrics

* some beefy metrics

* more metrics

* other metrics

* fix tests

* merge changes

* Apply suggestions from code review

* client/beefy: fix metrics

* client/beefy: separate metrics per component, avoid double registering

* client/beefy: deduplicate metrics registration code

* remove unused metric

* impl review suggestions

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@dharjeezy @acatangiu @nathanwhit
client/beefy: add more metrics for production visibility (paritytech#…
654d218 
…12910)

* few beefy metrics

* more beefy metrics

* some beefy metrics

* some beefy metrics

* more metrics

* other metrics

* fix tests

* merge changes

* Apply suggestions from code review

* client/beefy: fix metrics

* client/beefy: separate metrics per component, avoid double registering

* client/beefy: deduplicate metrics registration code

* remove unused metric

* impl review suggestions

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@MOZGIII MOZGIII mentioned this pull request Nov 8, 2023
Closed
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acatangiu acatangiu acatangiu left review comments

@Lederstrumpf Lederstrumpf Lederstrumpf approved these changes

@serban300 serban300 serban300 approved these changes

Assignees

@dharjeezy dharjeezy

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

client/beefy: add more metrics for production visibility
4 participants
@dharjeezy @acatangiu @Lederstrumpf @serban300