-
Notifications
You must be signed in to change notification settings - Fork 28
Integrate metrics sender into state machine #488
Conversation
8419820
to
7ddbf8c
Compare
I was able to implement a Function-like procedural macros for the code |
10e4bcc
to
1bc7f03
Compare
@Robert-Steiner should I review now or do you think it's worth waiting for you to rebase against the changes in #492 ? |
@little-dude I think it makes sense to merge #492 first. Then I can rebase and finish the implementation of the missing message metrics. |
#build |
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.
looks good to me so far! left a few comments that caught me eye. BTW when trying to build it earlier, I got several
error[E0658]: procedural macros cannot be expanded to statements
errors - but perhaps this is expected.
941a519
to
7d8233e
Compare
You right, function-like procedural macros were added recently. I forgot to mention that you need at least rust v1.45.0. ( |
I would like to briefly summarize again what the idea behind the macro is and how it works. #[cfg(feature = "metrics")]
sender.send(metrics) It would be more elegant if we could reduce the two lines into one line. metrics!(sender, metric::update()); First, I tried to solve this with macro_rules! metrics {
($sender:expr, $metric:expr) => {
#[cfg(feature = "metrics")]
$sender.send($metric);
};
}
metrics!(sender, metric::update()); However it has two downsides.
Next, I tried to solve it with
#[proc_macro]
pub fn metrics(input: TokenStream) -> TokenStream {
let Send { sender, metrics } = parse_macro_input!(input as Send);
TokenStream::from(quote! {
#[cfg(feature = "metrics")]
{
#(#sender.send(#metrics);)*
}
})
}
However, this approach also has its disadvantages. We need to publish an additional crate and it is a bit hacky. #[cfg(feature = "metrics")]
sender.send(metrics) first. |
7d8233e
to
e1a531d
Compare
Codecov Report
@@ Coverage Diff @@
## master #488 +/- ##
==========================================
+ Coverage 53.75% 54.43% +0.68%
==========================================
Files 65 67 +2
Lines 3099 3187 +88
==========================================
+ Hits 1666 1735 +69
- Misses 1433 1452 +19
Continue to review full report at Codecov.
|
#build |
50fc361
to
3ee8a52
Compare
rust/xaynet-metrics-macro/src/lib.rs
Outdated
let Send { sender, metrics } = parse_macro_input!(input as Send); | ||
|
||
TokenStream::from(quote! { | ||
#[cfg(feature = "metrics")] |
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.
👍
8719ea0
to
75ef2e1
Compare
#build |
#build |
1 similar comment
#build |
Very, very nice! 👏 |
#build Last one prior to it being merged (but please, if you already feel like merging it, go ahead!) |
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.
very nice. Would be nice to split this in three commits for merging: the macros, the removal of the yaml files, and the actual integration in the state machine.
19a7049
to
1a21f56
Compare
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.
the use of the metrics!
macro is very nice indeed!
1a21f56
to
6ab7491
Compare
implements a new `metrics!` macro
6ab7491
to
7631a06
Compare
The second part of the
Implement data collection to InfluxDB
task.Integration of the MetricsSender into the state machine.
My first idea was to put the metrics sender into the CoordinatorState so that we don't have to add another field to the
PhaseState
. However, I didn't like it because the metrics sender has nothing to do with the coordinator state. Therefore, I ended up putting the metrics sender into thePhaseState
.In my opinion the same applies for
EventPublisher
. It may make sense to decouple theEventPublisher
from theCoordinatorState
in a separate PR. We might have two structs: astate
struct and anio
struct. Both are fields of acoordinator
struct which then used in the state machine.Future improvements
My original plan was to have something like a log macro e.g.
metric!(round_id, 1)
which you can add anywhere in the code without having to carry a metrics sender around. However, I have no glue how to do that. I know that themetrics-rs
crate does just that, but it is at a very early stage of development. It could be possible to do this with#[proc_macro_hack]
.