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

Prometheus metrics #11823

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Prometheus metrics #11823

wants to merge 34 commits into from

Conversation

adria0
Copy link

@adria0 adria0 commented Jul 17, 2020

Implements #11725

  • Updates rust version to 2018, uses async
  • Uses tokio-runtime 0.1.2 with tokio 0.2 backend.
  • New trait PrometheusMetrics implemented it in Client and EthSync
  • Hyper server for serve metrics in parity/metrics.rs, metrics be accessed via http://...:3000/metrics, only for full client (not implemented for light client)
  • Command line flags for metrics, disabled by default, port and interface can be configurated
  • Docker-compose for starting the client with metrics in scripts/prometheus, contains grafana/prometheus visualization. Go to the folder and run docker-compose up.
  • New snapshot creation status .restoration_status() , so renamed .status() to .creation_status().

ethcore/snapshot/src/service.rs Outdated Show resolved Hide resolved
parity/metrics.rs Outdated Show resolved Hide resolved
parity/metrics.rs Outdated Show resolved Hide resolved
parity/metrics.rs Outdated Show resolved Hide resolved
ethcore/src/client/client.rs Outdated Show resolved Hide resolved
util/stats/src/lib.rs Outdated Show resolved Hide resolved
ethcore/sync/src/api.rs Outdated Show resolved Hide resolved
ethcore/sync/src/api.rs Outdated Show resolved Hide resolved
parity/metrics.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,1576 @@
{
Copy link

Choose a reason for hiding this comment

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

Are these configs something we really want to ship inside the repo? cc @denisgranha

@vorot93
Copy link

vorot93 commented Jul 17, 2020

Looks good overall.

Consider making an optional feature flag for this. Also needs a clean rebase onto master.

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Great work, @adria0! 👏 👏 👏

ethcore/snapshot/src/service.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,705 @@
##################### Grafana Configuration Defaults #####################
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file should live wherever devops/deployment things are maintained

util/stats/src/lib.rs Outdated Show resolved Hide resolved
util/stats/src/lib.rs Outdated Show resolved Hide resolved
@adria0 adria0 force-pushed the adria0/prometheus-tokio-compat branch from e49f538 to b9b6758 Compare July 18, 2020 15:08
@adria0 adria0 force-pushed the adria0/prometheus-tokio-compat branch 2 times, most recently from c2200f5 to 16d89fc Compare July 20, 2020 07:50
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM overall. I'm not sure what @vorot93 means by making an optional feature flag. Would you mind to explain?


let state_db = self.state_db.read();
prometheus_gauge(r, "statedb_mem_used", "State DB memory used", state_db.mem_used() as i64);
prometheus_gauge(r, "statedb_cache_size", "State DB cache size", state_db.cache_size() as i64);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah consider putting this under a scope { }.

@vorot93
Copy link

vorot93 commented Jul 29, 2020

@sorpaas Not everyone needs prometheus metrics. Therefore it would be a good idea to make this a cargo feature which we could enable by default.

@sorpaas
Copy link
Collaborator

sorpaas commented Jul 29, 2020

@vorot93 I honestly think it's not easy to do that. A lot of the prometheus changes require additional parameters to be set. And if we use flag for that I believe it might be really complicated.

Always building prometheus might result in slightly larger binary size, but other than that, as long as the service can be disabled runtime, it wouldn't bring any noticeable performance penalty. The prometheus service is also enabled in Substrate by default.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants