Metrics #1102
Metrics #1102
Conversation
ghost
marked this pull request as ready for review
ghost
requested review from
connormullett,
creativcoder,
cryptoquick and
ec2
as code owners
|
Nice! This looks like it measures block time and a few other things. How would you recommend I run it? |
|
@cryptoquick At the moment those metrics are collected but are not viewable. Working to make them viewable today. |
|
Okay, and this works without prometheus running, correct? |
|
@cryptoquick that's correct |
There was a problem hiding this comment.
Got an error when trying to run this:
thread 'async-std/runtime' panicked at 'calledResult::unwrap()on anErrvalue: InconsistentCardinality { expect: 7, got: 1 }', /home/hunter/.cargo/registry/src/github.com-1ecc6299db9ec823/prometheus-0.12.0/src/vec.rs:258:49
|
Also, you're missing the following metric as defined in the issue ACs:
Maybe try measuring this after every flush: |
|
@cryptoquick I didn't realize that moving this from draft to open would auto request review, that's my bad. I still have much to do, including the items you mentioned. I don't see a button that will move this from open -> draft again. |
|
Oh, gotcha! Be sure to let us know when it's RFR. Also LMK if you think I could help in some way! |
|
i know this isnt R4R yet, but you've incorrectly grouped Libp2p metrics (eg HELLO_REQUEST) as Gossip metrics. |
There was a problem hiding this comment.
Just found one obvious typo.
I'm able to compile this and it runs fine, but I'm not able to test it on my laptop because, although I have docker configured on my desktop, I'm reluctant to do similar on my laptop, so my feedback on this will be somewhat limited for now.
|
It LGTM. Im just setting up Docker on BigChungus right now to see this in live action |
There was a problem hiding this comment.
The feedback I've provided has been addressed. @ec2, be sure to let us know how your Docker deploy works out.
There was a problem hiding this comment.
Looks great! The only thing I felt is if Metrics could be global so we wouldn't have to pass it around everywhere. Alternatively, it could be a trait with default methods, which structs could then implement.
I tried running docker compose on aws machine but it fails. Posted in detail on slack.
This was an issue with the |
| Histogram, HistogramOpts, | ||
| }; | ||
|
|
||
| lazy_static! { |
There was a problem hiding this comment.
Kudos for making this a singleton! One more suggestion, why don't we try once_cell: async-rs/async-std#406 as people speak highly of it compared to lazy_static.
There was a problem hiding this comment.
Yeah, I looked into it, but the stdlib impl is only available in nightly and despite that, it's a container type that would require every routine that needs the OnceCell<T> to call a getter method to retrieve the inner T.
There was a problem hiding this comment.
I'm all ears if you understand the benefits we might get from adding that getter every time we collect a metric.
There was a problem hiding this comment.
From my understanding, the benefits would be:
- Lack of macros
- Faster hot get
I am not sure if that is enough for us to switch over at this point for this.
There was a problem hiding this comment.
Why is it faster than accessing a global static variable?
Is the concern with macros the incremental increase in compilation cost?
There was a problem hiding this comment.
oh, you don't need to use the OnceCell container. Instead you can use: https://docs.rs/once_cell/1.8.0/once_cell/#lazy-initialized-global-data. Anyways if you feel the refactor is quite complex, then lazy_static it is! As for perf improvements, we will have to measure to see the benefits in context with forest's access patterns.
There was a problem hiding this comment.
Not sure why its faster on hot get. That's just what someone said. But i dont think its significant.
The concern with macros is increased compilation cost as well as the fact that there is code being generated. That being said, those are "meh" concerns for me... I'm ok with lazy_static. We use lazy_static in other parts of the codebase
There was a problem hiding this comment.
I don’t feel enticed, seems more fashion than function
Summary of changes
Changes introduced in this pull request:
forestprometheus&grafana) using DockerReference issue to close (if applicable)
Closes #1096 & #1097
Other information and links