feat: add stagedsync headers metrics #498

entropidelic · 2022-12-16T19:51:52Z

This PR adds the following metrics:

stages.headers.counter: Number of headers successfully retrived
stages.headers.timeout_error: Number of timeout errors while requesting headers
stages.headers.validation_errors: Number of validation errores while requesting headers
stages.headers.unexpected_errors: Number of unexpected errors while requesting headers
stages.headers.request_time: Elapsed time of successful header requests

The prometheus exporter is modularized intro its own module. Also, a description for the metrics is added and the metrics docs is updated.

codecov-commenter · 2022-12-16T20:06:23Z

Codecov Report

Merging #498 (5b27e7f) into main (80b34a9) will increase coverage by 0.36%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main     #498      +/-   ##
==========================================
+ Coverage   72.53%   72.90%   +0.36%     
==========================================
  Files         243      247       +4     
  Lines       24584    24688     +104     
==========================================
+ Hits        17833    17998     +165     
+ Misses       6751     6690      -61

Impacted Files	Coverage Δ
bin/reth/src/lib.rs	`100.00% <ø> (ø)`
bin/reth/src/metrics_describer.rs	`0.00% <0.00%> (ø)`
bin/reth/src/node/mod.rs	`0.00% <0.00%> (ø)`
bin/reth/src/prometheus_exporter.rs	`0.00% <0.00%> (ø)`
crates/stages/src/lib.rs	`100.00% <ø> (ø)`
crates/stages/src/stages_metrics_describer.rs	`0.00% <0.00%> (ø)`
crates/stages/src/stages/headers.rs	`97.68% <100.00%> (+0.03%)`	⬆️
crates/stages/src/stages_metrics.rs	`100.00% <100.00%> (ø)`
crates/net/network/src/state.rs	`51.03% <0.00%> (ø)`
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mattsse

I'm supportive of this in general, but defer to @onbjerg here.

what are the implications if we run this without prometheus? are these just no-ops then?

mattsse · 2022-12-16T23:49:36Z

bin/reth/src/prometheus_exporter.rs

+use metrics_exporter_prometheus::PrometheusBuilder;
+use metrics_util::layers::{PrefixLayer, Stack};
+
+pub(crate) fn initialize_prometheus_exporter() {


instead of panicking here, let's add a try_ option as well, we should have eyre as error handling here so letting this return a result instead is nicer.

mattsse · 2022-12-16T23:51:18Z

crates/stages/src/stages/headers.rs

+                    histogram!("stages.headers.request_time", start.elapsed());
+                    counter!("stages.headers.counter", res.len() as u64);


we probably want these metrics, but it would be nicer to extract this to a separate call, so it doesn't clutter this function here.

mattsse · 2022-12-16T23:51:23Z

crates/stages/src/stages/headers.rs

@@ -107,20 +113,23 @@ impl<DB: Database, D: HeaderDownloader, C: Consensus, H: HeadersClient, S: Statu
                }
                Err(e) => match e {
                    DownloadError::Timeout => {
+                        increment_counter!("stages.headers.timeout_errors");


mattsse · 2022-12-16T23:52:09Z

bin/reth/src/metrics_describer.rs

+
+use metrics::{describe_counter, describe_histogram};
+
+pub(crate) fn describe_metrics() {


please add brief, one-line, docs here.

onbjerg · 2022-12-17T20:43:12Z

bin/reth/src/metrics_describer.rs

+
+pub(crate) fn describe_metrics() {
+    // Describe stagedsync headers metrics
+    describe_counter!("stages.headers.counter", "Number of headers successfully retrived");


hmm, not entirely sure if this should be here. we should find a way to describe the metrics in the crates they belong to for maximum reusability

onbjerg · 2022-12-17T20:44:21Z

crates/stages/src/stages/headers.rs

@@ -92,6 +95,9 @@ impl<DB: Database, D: HeaderDownloader, C: Consensus, H: HeadersClient, S: Statu
        while let Some(headers) = stream.next().await {
            match headers.into_iter().collect::<Result<Vec<_>, _>>() {
                Ok(res) => {
+                    // Register elapsed time of request
+                    histogram!("stages.headers.request_time", start.elapsed());


Should probably be in the downloader itself since we might replace this with a concurrent downloader (and in that case this metric is wrong)

entropidelic · 2022-12-20T17:43:51Z

thank you @mattsse and @onbjerg for the feeback. The PR is now ready for a second review.

I removed the headers request time metric for now, we will be addressing that in another PR, directly in the downloader itself as suggested by @onbjerg

gakonst

I like this - let's just inline the functions so we don't pay overheads when the macro is not enabled

gakonst · 2022-12-21T09:18:10Z

crates/stages/src/stages_metrics.rs

+use metrics::{counter, increment_counter};
+use reth_interfaces::p2p::error::DownloadError;
+
+pub(crate) fn update_headers_metrics(n_headers: u64) {


Should we inline(always) these calls?

Instead of individual calls, I'd rather have a StageMetrics struct with Gauge/Counters as fields that are mapped to the registered metrics. And this struct then encapsulates all the metrics that we have, this is easier to maintain than standalone metric functions.

I'm not sure how the exact workflow looks like in metrics but imo it should be possible to register them and create a Gauge/Counter for it.

Let me know if this sounds good:
I will implement a struct HeaderMetrics like so,

struct HeaderMetrics { pub headers_counter: Counter, pub headers_timeout_errors: Counter, pub headers_validation_errors: Counter, pub headers_unexpected_errors: Counter, }

This struct could be initialized inside the HeaderStage, in a field metrics and when a metric should be updated, e.g. the headers counter, in the code this could be done with

self.metrics.headers_counter.increment(res.len() as u64);

Is this consitent with your suggestions?

This sounds right. @mattsse?

I have refactored the metrics to encapsulate them in the HeaderMetrics struct now. Let me know what you think @mattsse!

gakonst · 2022-12-21T09:19:09Z

docs/design/metrics.md

@@ -53,6 +56,14 @@ How the metrics are exposed to the end-user is determined by the CLI.

 [^1]: The top-level namespace is added by the CLI using [`metrics_util::layers::PrefixLayer`][metrics_util.PrefixLayer].

+### Current metrics
+#### StagedSync Headers
+- `stages.headers.counter`: Number of headers successfully retrived


Suggested change

- `stages.headers.counter`: Number of headers successfully retrived

- `stages.headers.counter`: Number of headers successfully retrieved

mattsse · 2022-12-21T09:41:39Z

bin/reth/src/prometheus_exporter.rs

+use metrics_util::layers::{PrefixLayer, Stack};
+use std::net::SocketAddr;
+
+pub(crate) fn initialize(listen_addr: SocketAddr) -> eyre::Result<()> {


eventually, the exporter will be spawned with our TaskManager.

I'd like a separate function that returns the recorder and exporter.

perhaps even a type that bundles those and allows to configure the Stack, like a builder or something

Would it be ok to create a separate issue for this and tackle it in another PR?

mattsse · 2022-12-21T09:52:07Z

crates/stages/src/stages_metrics.rs

+use metrics::{counter, increment_counter};
+use reth_interfaces::p2p::error::DownloadError;
+
+pub(crate) fn update_headers_metrics(n_headers: u64) {


Instead of individual calls, I'd rather have a StageMetrics struct with Gauge/Counters as fields that are mapped to the registered metrics. And this struct then encapsulates all the metrics that we have, this is easier to maintain than standalone metric functions.

I'm not sure how the exact workflow looks like in metrics but imo it should be possible to register them and create a Gauge/Counter for it.

rkrasiuk

Nice work overall! Supportive of @mattsse's suggestion to use structs for metrics.

In the future, we might want to start exporting the time spent on downloading each batch

docs/design/metrics.md

mattsse

smol nit

mattsse · 2022-12-22T11:42:59Z

crates/stages/src/stages_metrics.rs

+    pub fn new() -> Self {
+        Self {
+            headers_counter: register_counter!("stages.headers.counter"),
+            headers_timeout_errors: register_counter!("stages.headers.timeout_errors"),
+            headers_validation_errors: register_counter!("stages.headers.validation_errors"),
+            headers_unexpected_errors: register_counter!("stages.headers.unexpected_errors"),
+        }
+    }


redundant, new, move to default

mattsse

lgtm, just needs rebase

This reduces image build times by ~50% after the first one. Uses cargo-chef inside the image. More info in: https://morioh.com/p/987a2bda4526

mattsse · 2022-12-22T14:30:44Z

final blocker:

https://github.com/paradigmxyz/reth/actions/runs/3758567946/jobs/6387061410

replace new with default

entropidelic marked this pull request as ready for review December 16, 2022 22:03

Rjected added A-staged-sync Related to staged sync (pipelines and stages) C-enhancement New feature or request labels Dec 16, 2022

mattsse requested changes Dec 16, 2022

View reviewed changes

onbjerg reviewed Dec 17, 2022

View reviewed changes

entropidelic force-pushed the add-headers-metrics branch from e35a021 to 95841ea Compare December 20, 2022 14:25

entropidelic requested review from rkrasiuk and gakonst as code owners December 20, 2022 14:25

entropidelic force-pushed the add-headers-metrics branch from 95841ea to ffcc993 Compare December 20, 2022 14:29

gakonst approved these changes Dec 21, 2022

View reviewed changes

mattsse requested changes Dec 21, 2022

View reviewed changes

entropidelic force-pushed the add-headers-metrics branch from d34e682 to 835be6f Compare December 21, 2022 15:37

rkrasiuk approved these changes Dec 21, 2022

View reviewed changes

docs/design/metrics.md Outdated Show resolved Hide resolved

mattsse requested changes Dec 22, 2022

View reviewed changes

entropidelic force-pushed the add-headers-metrics branch from 56b4fa6 to c309f9a Compare December 22, 2022 13:53

mattsse approved these changes Dec 22, 2022

View reviewed changes

MegaRedHand and others added 12 commits December 22, 2022 11:20

add dockerfile for reth

80214ea

Add docker compose for prometheus

1bdaff7

Add some metrics

499a6fd

Add p2p connection errors metric

25b0b62

Add dependency caching for dockerfile

da27dde

This reduces image build times by ~50% after the first one. Uses cargo-chef inside the image. More info in: https://morioh.com/p/987a2bda4526

Add --metrics flag to docker-compose config file

33c596a

add Cargo.lock

230c15c

Move docker-compose.yml to docker directory

392a21d

Apply formatting

9f81e32

Remove docker folder

4398f4d

Remove .dockerignore file

c5e77ee

Add 'reth' prefix to metric names

97ef550

entropidelic added 24 commits December 22, 2022 11:20

Add headers errors and request time metrics

e099203

Modularize metrics exporter functionality and describe metrics

202feee

Format files

b6f3c79

Add metrics documentation in metrics.md

bfe0678

Fix metrics doc title

2f64c59

Commit changes after rebase

62089a6

Solve conflict

e72e078

Modularize metrics describers

3640cab

Add stages_metrics_describer

ad08b95

Rearrange header error metrics

5d365c3

Add update_headers_metrics function

2a56e63

Add one-line docs to describers

4f25917

Remove commented line

d8e153a

Refactor metrics describer

344bbd1

Update metrics doc

24f7e7e

Fix import

ed89ae0

Add header metrics struct

e7c4a4b

add new metrics format in the headers execute method

dc9ae56

Add default implementation for HeaderMetrics

b1b3b07

Fix typo

cb1d60c

Fix another typo

4b3d065

Fix more typos

ebcf789

Move new HeaderMetrics meathod to default

372b6b3

Solve conflicts

30f6f6b

entropidelic force-pushed the add-headers-metrics branch from 1d9c8b2 to 30f6f6b Compare December 22, 2022 14:24

Fix test

e0a8065

mattsse merged commit b12939d into paradigmxyz:main Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add stagedsync headers metrics #498

feat: add stagedsync headers metrics #498

entropidelic commented Dec 16, 2022 •

edited

codecov-commenter commented Dec 16, 2022 •

edited

mattsse left a comment

mattsse Dec 16, 2022

mattsse Dec 16, 2022

mattsse Dec 16, 2022

mattsse Dec 16, 2022

onbjerg Dec 17, 2022

onbjerg Dec 17, 2022

entropidelic commented Dec 20, 2022

gakonst left a comment

gakonst Dec 21, 2022

mattsse Dec 21, 2022

gakonst Dec 21, 2022

entropidelic Dec 21, 2022 •

edited

gakonst Dec 21, 2022

entropidelic Dec 21, 2022

gakonst Dec 21, 2022

entropidelic Dec 21, 2022

mattsse Dec 21, 2022

entropidelic Dec 21, 2022

mattsse Dec 21, 2022

rkrasiuk left a comment

mattsse left a comment

mattsse Dec 22, 2022

entropidelic Dec 22, 2022

mattsse left a comment •

edited

mattsse commented Dec 22, 2022

		histogram!("stages.headers.request_time", start.elapsed());
		counter!("stages.headers.counter", res.len() as u64);


		use metrics::{describe_counter, describe_histogram};

		pub(crate) fn describe_metrics() {

	- `stages.headers.counter`: Number of headers successfully retrived
	- `stages.headers.counter`: Number of headers successfully retrieved

feat: add stagedsync headers metrics #498

feat: add stagedsync headers metrics #498

Conversation

entropidelic commented Dec 16, 2022 • edited

codecov-commenter commented Dec 16, 2022 • edited

Codecov Report

mattsse 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

entropidelic commented Dec 20, 2022

gakonst 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

entropidelic Dec 21, 2022 • edited

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

rkrasiuk left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment • edited

Choose a reason for hiding this comment

mattsse commented Dec 22, 2022

entropidelic commented Dec 16, 2022 •

edited

codecov-commenter commented Dec 16, 2022 •

edited

entropidelic Dec 21, 2022 •

edited

mattsse left a comment •

edited