feat(metrics): `Metrics` derive macro #592

rkrasiuk · 2022-12-23T13:49:45Z

This PR introduces Metrics derive macro to reduce code duplication for declaring, registering and deriving metrics.
Addresses #584 (comment)

mattsse

I like this a lot.

some nits.

also I'd be great to have some simple tests: ref https://github.com/metrics-rs/metrics/blob/main/metrics-macros/src/tests.rs

re crate name, maybe we'll have additional metrics stuff so I'd suggest we name this metrics-macros?

crates/metrics/src/expand.rs

crates/metrics/src/lib.rs

crates/stages/src/stage_metrics.rs

codecov-commenter · 2022-12-26T14:45:53Z

Codecov Report

Merging #592 (89f053d) into main (ba585cf) will increase coverage by 0.24%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
+ Coverage   72.00%   72.25%   +0.24%     
==========================================
  Files         251      255       +4     
  Lines       25478    25737     +259     
==========================================
+ Hits        18345    18595     +250     
- Misses       7133     7142       +9

Flag	Coverage Δ
unit-tests	`72.25% <94.73%> (+0.24%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bin/reth/src/node/mod.rs	`0.00% <0.00%> (ø)`
crates/stages/src/lib.rs	`100.00% <ø> (ø)`
crates/stages/src/stages/headers.rs	`97.65% <ø> (ø)`
crates/metrics/metrics-derive/src/metric.rs	`82.05% <82.05%> (ø)`
crates/stages/src/stage_metrics.rs	`87.50% <87.50%> (ø)`
crates/metrics/metrics-derive/src/expand.rs	`99.23% <99.23%> (ø)`
crates/metrics/metrics-derive/src/lib.rs	`100.00% <100.00%> (ø)`
crates/metrics/metrics-derive/src/with_attrs.rs	`100.00% <100.00%> (ø)`
crates/transaction-pool/src/test_util/mock.rs	`53.70% <0.00%> (-6.67%)`	⬇️
crates/net/eth-wire/src/ethstream.rs	`84.46% <0.00%> (-0.56%)`	⬇️
... and 10 more

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

rkrasiuk · 2022-12-26T16:19:52Z

@mattsse @onbjerg RFR now

gakonst

Good from me. Nice work. Thinking we should extract some of these general token parsing utils to a separate crate for use inside of Foundry, ethers and here.

gakonst · 2022-12-26T17:03:46Z

crates/metrics/metrics-derive/src/expand.rs

+            /// Describe all exposed metrics. Internally calls `describe_*` macros from
+            /// the metrics crate according to the metric type.
+            /// Ref: https://docs.rs/metrics/0.20.1/metrics/index.html#macros


@mattsse iirc this does not work and requires #[doc = ...]?

gakonst · 2022-12-26T17:10:45Z

crates/metrics/metrics-derive/src/lib.rs

+///     }
+/// }
+/// ```
+#[proc_macro_derive(Metrics, attributes(metrics, metric))]


great docs here

gakonst · 2022-12-26T17:12:39Z

crates/metrics/metrics-derive/tests/metrics.rs

@@ -0,0 +1,180 @@
+use std::{collections::HashMap, sync::Mutex};


great work in these tests. really clear.

gakonst · 2022-12-26T17:17:18Z

crates/metrics/metrics-derive/tests/trybuild.rs

@@ -0,0 +1,5 @@
+#[test]
+fn compile_test() {
+    let t = trybuild::TestCases::new();


also really neat - TIL

First, if a test case is being run as compile_fail but a corresponding *.stderr file does not exist, the test runner will save the actual compiler output with the right filename into a directory called wip within the directory containing Cargo.toml. So you can update these files by deleting them, running cargo test, and moving all the files from wip into your testcase directory.

Alternatively, run cargo test with the environment variable TRYBUILD=overwrite to skip the wip directory and write all compiler output directly in place. You'll want to check git diff afterward to be sure the compiler's output is what you had in mind.

https://github.com/dtolnay/trybuild#workflow - really sick work @dtolnay, thanks for the crate

i've spent a couple of hours battling compiletest-rs and rustc lib linkage before finding this gem. indeed, great work, really easy to use

entropidelic · 2022-12-26T22:00:15Z

bin/reth/src/node/mod.rs

@@ -26,9 +26,8 @@ use reth_network::{
 use reth_primitives::{Account, Header, H256};
 use reth_provider::{db_provider::ProviderImpl, BlockProvider, HeaderProvider};
 use reth_stages::{
+    stage_metrics::HeaderMetrics,


a question about the naming convention for the metrics modules, what do you think about following the structure

<crate>::metrics;

instead of, like in this case,

<crate>::<crate>_metrics;

I think it would be slightly nicer and less verbose

entropidelic · 2022-12-26T22:01:19Z

awesome work! Just a little suggestion about the naming convention of the metrics modules inside each crate

rkrasiuk · 2022-12-26T22:11:00Z

@entropidelic sgtm, ~~we can fix that in follow up PRs~~ fixed

prestwich · 2022-12-26T22:57:34Z

quick heads up, metrics scopes of the form x.y are not compatible with prometheus's data model. Metric names are validated as alphanumeric + _ + :. the period . is not an allowed character. As written, I believe this PR will block any usage of prometheus

gakonst · 2022-12-27T04:46:42Z

Should we rename everything to use underscores and validate that in the macro?

rkrasiuk · 2022-12-27T09:43:21Z

@prestwich you're right, thanks for the heads up

feat(metrics): Metrics derive macro

9907ff0

rkrasiuk added C-enhancement New feature or request A-observability Related to tracing, metrics, logs and other observability tools labels Dec 23, 2022

rkrasiuk requested a review from entropidelic December 23, 2022 13:49

rkrasiuk requested review from onbjerg and gakonst as code owners December 23, 2022 13:49

rkrasiuk marked this pull request as draft December 23, 2022 13:49

mattsse requested changes Dec 23, 2022

View reviewed changes

onbjerg reviewed Dec 24, 2022

View reviewed changes

crates/stages/src/stage_metrics.rs Outdated Show resolved Hide resolved

crates/stages/src/stage_metrics.rs Outdated Show resolved Hide resolved

rkrasiuk added 6 commits December 25, 2022 16:51

rename metrics to metrics-derive

d3342e0

use fully qualified fmt path

1028698

metric vec with capacity

332b92b

favor metrics over simple scope attr, simplify code

44cc618

pull metric description from doc comment

0484667

rm debug log

267db65

gakonst mentioned this pull request Dec 25, 2022

feat: add txpool metrics #584

Merged

rkrasiuk added 4 commits December 26, 2022 11:03

add more docs and sample usage

c5ded9d

Merge branch 'main' into rkrasiuk/feat-metrics-derive

c68917c

link to metrics macros

b5b701c

add tests for metrics derive macro

ae0ee21

rkrasiuk added 3 commits December 26, 2022 16:46

clippy

32ce413

simplify path comparison

72b1249

cleanup

7e14558

rkrasiuk marked this pull request as ready for review December 26, 2022 16:19

remove unused dep

89f053d

gakonst approved these changes Dec 26, 2022

View reviewed changes

entropidelic reviewed Dec 26, 2022

View reviewed changes

entropidelic approved these changes Dec 26, 2022

View reviewed changes

rkrasiuk added 2 commits December 27, 2022 00:17

rename stage metrics

6e6277e

Merge branch 'main' into rkrasiuk/feat-metrics-derive

f9e78c2

rkrasiuk merged commit 663efa8 into main Dec 26, 2022

rkrasiuk deleted the rkrasiuk/feat-metrics-derive branch December 26, 2022 22:27

rkrasiuk mentioned this pull request Dec 27, 2022

fix(metrics): metrics derive separator #625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): `Metrics` derive macro #592

feat(metrics): `Metrics` derive macro #592

rkrasiuk commented Dec 23, 2022 •

edited

Loading

mattsse left a comment •

edited

Loading

codecov-commenter commented Dec 26, 2022 •

edited

Loading

rkrasiuk commented Dec 26, 2022

gakonst left a comment •

edited

Loading

gakonst Dec 26, 2022

gakonst Dec 26, 2022

gakonst Dec 26, 2022

gakonst Dec 26, 2022

gakonst Dec 26, 2022

rkrasiuk Dec 26, 2022

entropidelic Dec 26, 2022

entropidelic commented Dec 26, 2022

rkrasiuk commented Dec 26, 2022 •

edited

Loading

prestwich commented Dec 26, 2022 •

edited

Loading

gakonst commented Dec 27, 2022

rkrasiuk commented Dec 27, 2022

		@@ -0,0 +1,180 @@
		use std::{collections::HashMap, sync::Mutex};

feat(metrics): Metrics derive macro #592

feat(metrics): Metrics derive macro #592

Conversation

rkrasiuk commented Dec 23, 2022 • edited Loading

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Dec 26, 2022 • edited Loading

Codecov Report

rkrasiuk commented Dec 26, 2022

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

gakonst Dec 26, 2022

Choose a reason for hiding this comment

gakonst Dec 26, 2022

Choose a reason for hiding this comment

gakonst Dec 26, 2022

Choose a reason for hiding this comment

gakonst Dec 26, 2022

Choose a reason for hiding this comment

gakonst Dec 26, 2022

Choose a reason for hiding this comment

rkrasiuk Dec 26, 2022

Choose a reason for hiding this comment

entropidelic Dec 26, 2022

Choose a reason for hiding this comment

entropidelic commented Dec 26, 2022

rkrasiuk commented Dec 26, 2022 • edited Loading

prestwich commented Dec 26, 2022 • edited Loading

gakonst commented Dec 27, 2022

rkrasiuk commented Dec 27, 2022

feat(metrics): `Metrics` derive macro #592

feat(metrics): `Metrics` derive macro #592

rkrasiuk commented Dec 23, 2022 •

edited

Loading

mattsse left a comment •

edited

Loading

codecov-commenter commented Dec 26, 2022 •

edited

Loading

gakonst left a comment •

edited

Loading

rkrasiuk commented Dec 26, 2022 •

edited

Loading

prestwich commented Dec 26, 2022 •

edited

Loading