Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Additional Metrics collected and exposed via prometheus #5414

Merged
merged 37 commits into from
Apr 4, 2020

Conversation

gnunicorn
Copy link
Contributor

@gnunicorn gnunicorn commented Mar 26, 2020

This PR refactors the metrics measuring and Prometheus exposing entity in sc-service into its own submodule and extends the parameters it exposes by:

  • system load average (over one, five and 15min)
  • the TCP connection state of the process (lsof), refs Track additional metrics around resource consumption #5304
  • number of tokio threads
  • block import/validation times*
  • number of known forks
  • count items in each unbounded queue (with internal unbounded channels)
  • number of file descriptors opened by this process (*nix only at this point)
  • number of system threads (*nix only at this point)

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.

@gnunicorn gnunicorn added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 26, 2020
@gnunicorn gnunicorn added this to the 2.0 milestone Mar 26, 2020
@mxinden mxinden self-requested a review April 2, 2020 08:33
@tomaka
Copy link
Contributor

tomaka commented Apr 2, 2020

Are we aware that the content of primitives/utils/src/channels is not going to be maintainable by us?
Is this code destined to stay there long term?

@gnunicorn
Copy link
Contributor Author

@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.

@gnunicorn
Copy link
Contributor Author

because of timing and priority, I postponed

  • block production times
  • count of block productions failure

for a future PR.

@gnunicorn
Copy link
Contributor Author

Removed the Block Import Tracker and internally used ring buffer. Also applied the other requested changes. Please review again.

@gnunicorn gnunicorn added A0-please_review Pull request needs code review. B1-clientnoteworthy and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 2, 2020
Copy link
Contributor

@expenses expenses left a 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!

client/service/src/metrics.rs Outdated Show resolved Hide resolved
primitives/utils/Cargo.toml Outdated Show resolved Hide resolved
primitives/utils/src/mpsc.rs Outdated Show resolved Hide resolved
primitives/utils/src/metrics.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mxinden mxinden left a 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.

client/cli/src/runtime.rs Show resolved Hide resolved
client/finality-grandpa/src/communication/periodic.rs Outdated Show resolved Hide resolved
client/finality-grandpa/src/lib.rs Outdated Show resolved Hide resolved
client/network/src/on_demand_layer.rs Outdated Show resolved Hide resolved
client/service/src/metrics.rs Outdated Show resolved Hide resolved
client/service/src/metrics.rs Outdated Show resolved Hide resolved
client/src/client.rs Outdated Show resolved Hide resolved
primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
primitives/utils/src/metrics.rs Outdated Show resolved Hide resolved
primitives/utils/src/mpsc.rs Outdated Show resolved Hide resolved
gnunicorn and others added 3 commits April 3, 2020 12:13
Co-Authored-By: Max Inden <mail@max-inden.de>
Co-Authored-By: Ashley <ashley.ruglys@gmail.com>
client/service/src/metrics.rs Show resolved Hide resolved
client/service/src/metrics.rs Show resolved Hide resolved
client/service/src/metrics.rs Outdated Show resolved Hide resolved
client/service/src/metrics.rs Outdated Show resolved Hide resolved
client/service/src/metrics.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mxinden mxinden left a 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!

client/service/src/metrics.rs Outdated Show resolved Hide resolved
client/service/src/metrics.rs Show resolved Hide resolved
client/src/in_mem.rs Outdated Show resolved Hide resolved
primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
primitives/utils/src/metrics.rs Show resolved Hide resolved
primitives/utils/src/lib.rs Show resolved Hide resolved
primitives/utils/src/metrics.rs Show resolved Hide resolved
primitives/utils/src/metrics.rs Outdated Show resolved Hide resolved
primitives/utils/src/mpsc.rs Outdated Show resolved Hide resolved
primitives/utils/src/mpsc.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mxinden mxinden left a 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.

primitives/utils/src/metrics.rs Show resolved Hide resolved
primitives/utils/src/metrics.rs Show resolved Hide resolved
primitives/utils/src/lib.rs Outdated Show resolved Hide resolved
primitives/utils/src/metrics.rs Outdated Show resolved Hide resolved
@gnunicorn gnunicorn merged commit 8991aab into master Apr 4, 2020
@gnunicorn gnunicorn deleted the ben-extra-promotheus-info branch April 4, 2020 13:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants