Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics package #40

Merged
merged 7 commits into from
Jul 1, 2020
Merged

Metrics package #40

merged 7 commits into from
Jul 1, 2020

Conversation

lukasz-zimnoch
Copy link
Member

Added metrics package which provides tools useful for gathering and exposing system metrics for external monitoring tools.

Added metrics package which
provides tools useful for gathering
and exposing system metrics for external
monitoring tools.
@lukasz-zimnoch lukasz-zimnoch requested review from nkuba and pdyraga June 10, 2020 11:36
@lukasz-zimnoch lukasz-zimnoch marked this pull request as draft June 10, 2020 12:36
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review June 10, 2020 13:01
@lukasz-zimnoch lukasz-zimnoch marked this pull request as draft June 12, 2020 12:06
Registry level labels are
redundant and can cause
pointless data-series growth.
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review June 12, 2020 13:59
@Shadowfiend
Copy link
Contributor

Is there an existing package used for similar stuff in any of our existing dependencies? Thinking specifically of libp2p.

@lukasz-zimnoch
Copy link
Member Author

Is there an existing package used for similar stuff in any of our existing dependencies? Thinking specifically of libp2p.

Unfortunately, I didn't find anything fitting our needs.

Copy link
Member

@pdyraga pdyraga left a comment

Choose a reason for hiding this comment

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

There is a metrics package provided by go-ethereum: https://github.com/ethereum/go-ethereum/tree/master/metrics

Is it something we could not use for our needs? If so, can we summarize why for future-us?

If we did not explore this option, I think I am fine merging this PR and looking at go-ethereum metrics in the future.

pkg/metrics/gauge.go Outdated Show resolved Hide resolved
pkg/metrics/registry.go Outdated Show resolved Hide resolved
@lukasz-zimnoch
Copy link
Member Author

There is a metrics package provided by go-ethereum: https://github.com/ethereum/go-ethereum/tree/master/metrics

Is it something we could not use for our needs? If so, can we summarize why for future-us?

If we did not explore this option, I think I am fine merging this PR and looking at go-ethereum metrics in the future.

Yeah, this PR overlaps several things from go-ethereum, but also provides some useful functionality tailored for keep-* clients. Specifically, the observer logic allows us to define reusable metric observation loops in one place (on keep-core side) and avoid polluting the codebase with metric primitives which must be done in case we use bare go-ethereum metric package. Also, this PR is rather minimalistic because for now, we don't need all metric types and other magic. At last, we can always incorporate useful things from go-ethereum in the future if needed.

@pdyraga pdyraga merged commit c1c03e7 into master Jul 1, 2020
@pdyraga pdyraga deleted the metrics-package branch July 1, 2020 10:09
@pdyraga pdyraga added this to the 1.2.0 milestone Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants