-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Additional Metrics collected and exposed via prometheus #5414
Conversation
Are we aware that the content of |
@tomaka hopefully not, as the FIXME said. Unfortunately neither std nor futures-rs exposes the internally used queue, otherwise I would have taking a much slimmer approach than copying the entire folder over from the PR – we only want the non-blocking ringbuffer feature here after all. Either way, once they accept and release or reject that PR, this part is prone for being overthrown. |
because of timing and priority, I postponed
for a future PR. |
Removed the Block Import Tracker and internally used ring buffer. Also applied the other requested changes. Please review again. |
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.
Some nitpicks, but these are great changes overall!
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.
Thanks for reducing the scope of this pull request. As a general wish I would prefer multiple small pull requests instead of one large pull request whenever the changes are not directly related. E.g. instrumenting our unbounded channels is not directly related to the service metric refactorings. Having multiple pull requests makes reviewing easier and does not have one controversial change block an easy one.
I have added a couple of comments inline.
Would you mind taking a look at the additional newlines introduced in this pull request? As far as I remember we e.g. don't start an impl
with a newline before the first function. Please correct me if I am wrong.
Co-Authored-By: Max Inden <mail@max-inden.de> Co-Authored-By: Ashley <ashley.ruglys@gmail.com>
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 last round is just a set of alignment issues, typos or unnecessary newlines.
Thanks for all the work you have put into this!
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.
Comments don't need to block merging.
This PR refactors the metrics measuring and Prometheus exposing entity in
sc-service
into its own submodule and extends the parameters it exposes by:lsof
), refs Track additional metrics around resource consumption #5304block import/validation times*unbounded
queue (with internal unbounded channels)refs #4679
*
While this would be a good case for a histogram, the way this works in promotheus doesn't fit our use case for two reasons: 1. it assumes a set of pre-known buckets the value should fall under; 2. it locks and calculates this bucket right there and then – which leads to a significant performance drop (50% bps here). Thus, this uses an internal ringbuffered queue (based off this future-rs patch) to track them without locking and then calculate count, average and medians when we tick.