Add prometheus registry to transaction pool, with couple of initial metrics #5657

NikVolf · 2020-04-16T09:23:01Z

bin/node-template/node/src/service.rs

bin/node/cli/src/service.rs

bkchr · 2020-04-16T09:53:19Z

client/transaction-pool/src/metrics.rs

+/// Transaction pool prometheus metrics.
+pub struct Metrics {
+	pub validation_pending: Gauge<U64>,
+	pub total_validated: Counter<U64>,


I think it would be nice to have some median over the time it takes to validate a transaction?

Probably, I will think of what I need most while resolving current issues or others to come

I have a bit of experience with Prometheus now. 🙃

In my opinion, validation_pending should be replaced with a validations_started of type Counter and that counts the number of validations that have started. Then you can do total_validated - validation_started to see the number of pending validations.

Gauges in general are not necessarily a good idea. Since metrics are collected only every couple of seconds, you don't know how many times inc() and dec() have been called in this interval. If instead you use two Counters and use Grafana to calculate a - b then you know.

Also, I don't know how difficult it is to implement that, but total_validated could instead be a Histogram, and when a validation is finished you would call observe() with the time it took to validate.
Histograms don't just report the time it took but also the number of times observe() has been called, so this can serve as a counter for the number of finished validations (and you can also get averages, quantiles, and so on).

Instead of exposing a median I would suggest exposing a Prometheus Histogram. Medians might be misleading as they can hide long tail latencies.

hmm.. Thanks for heads up about Gauges, probably saved me some time.
I thought even metrics are scrapped every couple of seconds, they also scrap all changes that were made between scraps, not just immediate value, but according to docs this seems to not be the case.

I will change to counter.

As for time, I'm not interested in it currently, but will probably add later.

they also scrap all changes that were made between scraps

No, this is a bit of a pitfall at the beginning. Prometheus is making a tradeoff between resolution and performance favoring performance in this place. E.g. recording every single change would require an allocated vector whereas only taking snapshots enables one to use cheap atomic variables.

tomaka · 2020-04-16T12:57:11Z

bin/node-template/node/src/service.rs

 				Ok(sc_client::LongestChain::new(backend.clone()))
 			})?
-			.with_transaction_pool(|config, client, _fetcher| {
+			.with_transaction_pool(|config, client, _fetcher, prometheus_registry| {


I know this is the most straight-forward solution for this PR, but ugh this API. It doesn't make sense at all.
We should really remove these closures and simply have prometheus_registry (and all the other components) be a local variable.
We can't just continue adding parameters to these closures (which is a breaking change every single time) whenever we need something that is in the builder.

I'm not requesting any change on this PR in particular, but I thought I'd comment on that in general.

Have you seen this PR?
#5557

mxinden

This looks good to me overall. I got a couple of comments. Thanks for keeping the instrumentation style (new Metrics struct which has register function, ...) consistent with the remaining code base!

client/transaction-pool/src/metrics.rs

mxinden · 2020-04-16T13:16:46Z

client/transaction-pool/src/metrics.rs

+/// Transaction pool prometheus metrics.
+pub struct Metrics {
+	pub validation_pending: Gauge<U64>,
+	pub total_validated: Counter<U64>,


Instead of exposing a median I would suggest exposing a Prometheus Histogram. Medians might be misleading as they can hide long tail latencies.

client/transaction-pool/src/metrics.rs

Co-Authored-By: Max Inden <[email protected]>

client/transaction-pool/src/lib.rs

client/transaction-pool/src/metrics.rs

Co-Authored-By: Max Inden <[email protected]>

tomusdrw · 2020-04-16T09:58:46Z

client/transaction-pool/src/metrics.rs

+
+/// Transaction pool prometheus metrics.
+pub struct Metrics {
+	pub validation_pending: Gauge<U64>,


I'd traitify the accessors to the fields and implement them for Option<Metrics> so that you don't have to do the annoying if let Some all the time.

Not sure what you meant by traitifying accessors, but refactored to avoid if let Some everywhere

He meant that you implement a trait for Option<Arc<Metrics>>, but your solution is achieving the same simplifications ;)

…e into nv-txpool-prometheus # Conflicts: # client/transaction-pool/src/lib.rs

# Conflicts: # client/transaction-pool/Cargo.toml

tomusdrw · 2020-04-17T08:15:35Z

client/transaction-pool/src/metrics.rs

+use prometheus_endpoint::{register, Counter, PrometheusError, Registry, U64};
+
+#[derive(Clone, Default)]
+pub struct MetricsLink(Arc<Option<Metrics>>);


Would be good to add some basic docs at least, but since it's not public it's not really enforced.

Yeah, seems pretty self-documenting to me 🤷‍♀️

make new contructor

4224837

NikVolf added the A0-please_review Pull request needs code review. label Apr 16, 2020

NikVolf added this to the 2.0 milestone Apr 16, 2020

NikVolf force-pushed the nv-txpool-prometheus branch from 026c5c2 to 376c74d Compare April 16, 2020 09:32

bkchr reviewed Apr 16, 2020

View reviewed changes

bin/node-template/node/src/service.rs Outdated Show resolved Hide resolved

bin/node/cli/src/service.rs Outdated Show resolved Hide resolved

bin/node/cli/src/service.rs Outdated Show resolved Hide resolved

bkchr requested a review from mxinden April 16, 2020 09:46

add metrics to txpool

3a6eae0

NikVolf force-pushed the nv-txpool-prometheus branch from 376c74d to 3a6eae0 Compare April 16, 2020 09:49

NikVolf requested a review from tomusdrw as a code owner April 16, 2020 09:49

fix review

2640fe7

bkchr reviewed Apr 16, 2020

View reviewed changes

NikVolf added 2 commits April 16, 2020 12:55

fix doc comment

bd28557

Merge remote-tracking branch 'origin/master' into nv-txpool-prometheus

54fb501

tomaka reviewed Apr 16, 2020

View reviewed changes

mxinden reviewed Apr 16, 2020

View reviewed changes

NikVolf and others added 3 commits April 16, 2020 16:30

change to counters

4736970

Update client/transaction-pool/src/metrics.rs

6ec97d9

Co-Authored-By: Max Inden <[email protected]>

Update client/transaction-pool/src/metrics.rs

04d89df

Co-Authored-By: Max Inden <[email protected]>

mxinden approved these changes Apr 16, 2020

View reviewed changes

client/transaction-pool/src/lib.rs Outdated Show resolved Hide resolved

client/transaction-pool/src/lib.rs Outdated Show resolved Hide resolved

client/transaction-pool/src/metrics.rs Outdated Show resolved Hide resolved

NikVolf and others added 3 commits April 16, 2020 17:18

Update client/transaction-pool/src/metrics.rs

dc83269

Co-Authored-By: Max Inden <[email protected]>

Update client/transaction-pool/src/lib.rs

a7be54c

Co-Authored-By: Max Inden <[email protected]>

Update client/transaction-pool/src/lib.rs

cefb1ff

Co-Authored-By: Max Inden <[email protected]>

NikVolf added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 16, 2020

bkchr mentioned this pull request Apr 16, 2020

Add performance tracing to validate_transaction #5671

Merged

tomusdrw reviewed Apr 16, 2020

View reviewed changes

tomusdrw added A6-mustntgrumble and removed A8-mergeoncegreen labels Apr 16, 2020

tomusdrw added A6-mustntgrumble and removed A5-grumble labels Apr 16, 2020

kirushik mentioned this pull request Apr 16, 2020

More CPU-thrifty handling of transaction verification queue #5674

Closed

JoshOrndorff mentioned this pull request Apr 16, 2020

Remove CI to build rustdocs on each release #5652

Closed

NikVolf added 3 commits April 17, 2020 09:45

use dedicated wrapper

237556a

Merge branch 'nv-txpool-prometheus' of github.com:paritytech/substrat…

dc83587

…e into nv-txpool-prometheus # Conflicts: # client/transaction-pool/src/lib.rs

Merge remote-tracking branch 'origin/master' into nv-txpool-prometheus

8df8971

# Conflicts: # client/transaction-pool/Cargo.toml

tomusdrw approved these changes Apr 17, 2020

View reviewed changes

tomusdrw added A8-mergeoncegreen and removed A6-mustntgrumble labels Apr 17, 2020

tomusdrw reviewed Apr 17, 2020

View reviewed changes

NikVolf added A8-mergeoncegreen and removed A7-needspolkadotpr labels Apr 17, 2020

bkchr merged commit b7b60fa into master Apr 17, 2020

bkchr deleted the nv-txpool-prometheus branch April 17, 2020 09:02

Add prometheus registry to transaction pool, with couple of initial metrics #5657

Add prometheus registry to transaction pool, with couple of initial metrics #5657

Uh oh!

Conversation

NikVolf commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomaka Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomaka Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikVolf Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NikVolf commented Apr 16, 2020 •

edited

Loading

tomaka Apr 16, 2020 •

edited

Loading

tomaka Apr 16, 2020 •

edited

Loading

NikVolf Apr 16, 2020 •

edited

Loading