-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_dashboard: create MetricCommon trait #9857
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
apollo_dashboard: create MetricCommon trait #9857
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
7012545 to
e4e8ade
Compare
78a13b3 to
0f23e90
Compare
e4e8ade to
5897c62
Compare
|
Artifacts upload workflows: |
|
Benchmark movements: tree_computation_flow performance improved 😺 tree_computation_flow time: [13.782 ms 13.939 ms 14.096 ms] change: [-4.9950% -3.5745% -2.0722%] (p = 0.00 < 0.05) Performance has improved. Found 1 outliers among 100 measurements (1.00%) 1 (1.00%) high mild full_committer_flow performance regressed! full_committer_flow time: [15.202 ms 15.323 ms 15.448 ms] change: [+2.0848% +3.3243% +4.5808%] (p = 0.00 < 0.05) Performance has regressed. Found 5 outliers among 100 measurements (5.00%) 5 (5.00%) high mild |
0f23e90 to
b710d56
Compare
5897c62 to
2bdb8f9
Compare
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 31 files reviewed, 1 unresolved discussion
crates/apollo_metrics/src/metrics.rs line 54 at r1 (raw file):
#[derive(Clone, Debug)] struct Metric {
Can also impl MetricCommon
Code quote:
struct Metric
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 31 files reviewed, 2 unresolved discussions (waiting on @matanl-starkware)
crates/apollo_metrics/src/metrics.rs line 79 at r1 (raw file):
initial_value: u64, ) -> Self { Self { metric: Metric { scope, name, description }, initial_value }
wdyt about a new fn?
Code quote:
Metric { scope, name, description }
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 31 files reviewed, 3 unresolved discussions (waiting on @matanl-starkware)
crates/apollo_metrics/src/metrics.rs line 54 at r1 (raw file):
#[derive(Clone, Debug)] struct Metric {
wdyt about:
Metric (struct) -> MetricProperties / MetricCommonProperties / MetricAttributes / MetricCommonAttributes
MetricCommon (trait) -> Metric{whatever-comes-here-according-to-the-previous}Reader
Code quote:
Metric
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @matanl-starkware)
crates/apollo_metrics/src/metrics.rs line 192 at r1 (raw file):
fn get_description(&self) -> &'static str { self.metric.description }
Please implement once for the inner common member, and for the outer one call the fn on the inner
Code quote:
fn get_name(&self) -> &'static str {
self.metric.name
}
fn get_name_with_filter(&self) -> String {
format!("{}{}", self.metric.name, metric_label_filter!())
}
fn get_scope(&self) -> MetricScope {
self.metric.scope
}
fn get_description(&self) -> &'static str {
self.metric.description
}
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @matanl-starkware)
crates/apollo_metrics/src/metrics.rs line 192 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please implement once for the inner common member, and for the outer one call the fn on the inner
Also since this is just boilerplating of calling the inner fn I'd consider a macro:
- the most elegant solution seems like a proc macro, which happens to be already implemented in this https://crates.io/crates/ambassador
- if we prefer to avoid adding a dependency we can impl it ourselfs (not fun but doable)
- we can also go for a simpler macro like:
/// Expands to:
/// fn NAME(&self) -> T { self.inner.NAME() }
///
/// Usage (inside an `impl` block):
/// delegate_inner_fn!(ping -> u32);
macro_rules! delegate_inner_fn {
($name:ident -> $ret:ty) => {
fn $name(&self) -> $ret {
self.inner.$name()
}
};
}
and the invocations will be
impl Metric for MetricGauge {
delegate_inner_fn!(get_name -> &'static str);
...
}
I suggest option 1
2bdb8f9 to
ca12bc4
Compare
matanl-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/apollo_metrics/src/metrics.rs line 54 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Can also impl
MetricCommon
Done.
crates/apollo_metrics/src/metrics.rs line 54 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
wdyt about:
Metric(struct) ->MetricProperties/MetricCommonProperties/MetricAttributes/MetricCommonAttributes
MetricCommon(trait) ->Metric{whatever-comes-here-according-to-the-previous}Reader
Overkill IMO. It can be done someday if necessary.
crates/apollo_metrics/src/metrics.rs line 79 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
wdyt about a
newfn?
Done.
crates/apollo_metrics/src/metrics.rs line 192 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Also since this is just boilerplating of calling the inner fn I'd consider a macro:
- the most elegant solution seems like a proc macro, which happens to be already implemented in this https://crates.io/crates/ambassador
- if we prefer to avoid adding a dependency we can impl it ourselfs (not fun but doable)
- we can also go for a simpler macro like:
/// Expands to: /// fn NAME(&self) -> T { self.inner.NAME() } /// /// Usage (inside an `impl` block): /// delegate_inner_fn!(ping -> u32); macro_rules! delegate_inner_fn { ($name:ident -> $ret:ty) => { fn $name(&self) -> $ret { self.inner.$name() } }; }and the invocations will be
impl Metric for MetricGauge { delegate_inner_fn!(get_name -> &'static str); ... }I suggest option 1
Done.
ca12bc4 to
036a316
Compare
b710d56 to
56dd569
Compare
036a316 to
622dbe7
Compare
aee5cdc to
f55bde5
Compare
622dbe7 to
8904928
Compare
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware reviewed 26 of 31 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @matanl-starkware)
f55bde5 to
48a2feb
Compare
8904928 to
4215e84
Compare
Merge activity
|
matanl-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanl-starkware reviewed 24 of 31 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @matanl-starkware)

No description provided.