-
Notifications
You must be signed in to change notification settings - Fork 362
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
Adapt Periodic Puma Metrics #3521
Changes from all commits
b467d90
5ea1f64
bcf4bca
3b1e543
64bb224
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
require 'cloud_controller/secrets_fetcher' | ||
require 'cloud_controller/runners/thin_runner' | ||
require 'cloud_controller/runners/puma_runner' | ||
require 'prometheus/client/data_stores/direct_file_store' | ||
|
||
module VCAP::CloudController | ||
class Runner | ||
|
@@ -95,6 +96,7 @@ def run! | |
private | ||
|
||
def setup_cloud_controller | ||
setup_metrics | ||
setup_logging | ||
setup_telemetry_logging | ||
setup_db | ||
|
@@ -112,6 +114,24 @@ def create_pidfile | |
raise "ERROR: Can't create pid file #{@config.get(:pid_filename)}" | ||
end | ||
|
||
def setup_metrics | ||
return if @setup_metrics | ||
|
||
@setup_metrics = true | ||
|
||
return unless @config.get(:webserver) == 'puma' | ||
|
||
prometheus_dir = File.join(@config.get(:directories, :tmpdir), 'prometheus') | ||
FileUtils.mkdir_p(prometheus_dir) | ||
|
||
# Resetting metrics on startup | ||
Dir["#{prometheus_dir}/*.bin"].each do |file_path| | ||
File.unlink(file_path) | ||
end | ||
|
||
Prometheus::Client.config.data_store = Prometheus::Client::DataStores::DirectFileStore.new(dir: prometheus_dir) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for using a file datastore instead of in memory, if we want to reset the metrics on startup anyways? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The in-memory data store does not work for metrics collected by different processes (i.e. the outstanding requests), thus we need some kind of "central" storage; the easiest is the direct file store. |
||
end | ||
|
||
def setup_logging | ||
return if @setup_logging | ||
|
||
|
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.
Would be interesting to see how the metrics look like if we don't aggregate them. Would the different values emitted by each worker be stored in the same time series, using a label "worker", or would they simply overwrite each other and lead to wrong 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.
The metrics would be labelled with the
pid
of the worker processes. I'm in favor of aggregating them to be compatible with the Thin metrics, i.e. have one metric per vm. In the end we are not even interested in the outstanding requests per vm, but for all apis, so I don't see a use case for having metrics per process.