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

feat(metrics): derive with dynamic scope #785

Merged
merged 4 commits into from
Jan 14, 2023

Conversation

rkrasiuk
Copy link
Member

@rkrasiuk rkrasiuk commented Jan 9, 2023

closes #782

Provide additional dynamic kv entry for Metrics derive macro. If dynamic = true, the macro will generate fns for dynamic scope instead of operating on a preset one.

Examples

#[derive(Metrics)]
#[metrics(scope = "some_scope")]
struct StaticScopeMetrics {
     // fields...
}

// Describe
StaticScopeMetrics::describe();
// Instantiate
let metrics = StaticScopeMetrics::default();

#[derive(Metrics)]
#[metrics(dynamic = true)]
struct DynamicScopeMetrics {
     // fields...
}

let local_scope = "some_scope";
// Describe
StaticScopeMetrics::describe(local_scope);
// Instantiate
let metrics = DynamicScopeMetrics::new(local_scope);

NOTE: still need to update the docs.

@rkrasiuk rkrasiuk requested review from onbjerg and akirillo January 9, 2023 21:00
@rkrasiuk rkrasiuk requested a review from gakonst as a code owner January 9, 2023 21:00
@rkrasiuk rkrasiuk marked this pull request as draft January 9, 2023 21:00
@onbjerg onbjerg added C-enhancement New feature or request A-observability Related to tracing, metrics, logs and other observability tools labels Jan 9, 2023
@akirillo
Copy link
Contributor

Ok, this is making sense to me at a high level, and LGTM as it's passing tests!

A couple non-blocking questions from a Rust newbie:

  1. It feels like there's a lot of duplicate code between the MetricsScope::Static and MetricsScope::Dynamic branches, the only differences being how metric_name is constructed and what the final quote! block is that's returned - I'm curious, why not just match against the MetricScope enum when defining these two variables, instead of around the entire iteration over metric_fields? I guess the way you do it here avoids branching within each iteration, which is nice.
  2. In line with this, why move the generation of metric_name outside of the impl Metric block in metric.rs, and into derive in expand.rs? Doesn't change the API so it doesn't matter much, I guess

@akirillo akirillo self-assigned this Jan 13, 2023
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

@joshieDo wanna take this over the line once you have some bandwidth from the proptests? @rkrasiuk is sprinting on the new downloader in #881

@rkrasiuk
Copy link
Member Author

@akirillo expressed interest in taking it over as well

@rkrasiuk
Copy link
Member Author

we simply need to fix some docs & comments here, that's it

@akirillo
Copy link
Contributor

yup, will take over on this

@gakonst
Copy link
Member

gakonst commented Jan 13, 2023

Ah ACK!

@akirillo
Copy link
Contributor

@rkrasiuk added a quick usage example to lib.rs, any other areas of documentation I should touch on? Couldn't see anything else at a glance

@akirillo akirillo marked this pull request as ready for review January 13, 2023 23:20
@codecov-commenter
Copy link

Codecov Report

Merging #785 (f46990f) into main (a1c8a34) will decrease coverage by 0.00%.
The diff coverage is 62.39%.

@@            Coverage Diff             @@
##             main     #785      +/-   ##
==========================================
- Coverage   73.45%   73.44%   -0.01%     
==========================================
  Files         280      280              
  Lines       29697    29856     +159     
==========================================
+ Hits        21813    21928     +115     
- Misses       7884     7928      +44     
Flag Coverage Δ
unit-tests 73.44% <62.39%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
crates/metrics/metrics-derive/src/lib.rs 100.00% <ø> (ø)
crates/metrics/metrics-derive/src/expand.rs 80.59% <61.06%> (-18.78%) ⬇️
crates/metrics/metrics-derive/src/metric.rs 78.12% <100.00%> (-2.37%) ⬇️
crates/consensus/src/config.rs 50.00% <0.00%> (-1.29%) ⬇️
crates/net/network/src/manager.rs 49.05% <0.00%> (-0.22%) ⬇️
crates/net/network/src/peers/manager.rs 82.55% <0.00%> (-0.19%) ⬇️
crates/executor/src/config.rs 94.87% <0.00%> (+0.04%) ⬆️
crates/executor/src/executor.rs 90.63% <0.00%> (+2.40%) ⬆️

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

@rkrasiuk rkrasiuk merged commit d5fefa3 into main Jan 14, 2023
@rkrasiuk rkrasiuk deleted the rkrasiuk/feat-metrics-derive-dynamic branch January 14, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability Related to tracing, metrics, logs and other observability tools C-enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow setting metric scope at the time of instantiation
5 participants