Fix cc.requests.outstanding.gauge when using puma web server #3841
+251
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Samze
force-pushed
the
puma_cc.requests.outstanding.gauge_redis_refactor
branch
2 times, most recently
from
cb4dbaf
to
8a280d7
Compare
cloudfoundry#1312 introduced `cc.requests.outstanding.gauge` which holds the counter in memory. With the introduction of puma there may be multiple processes, so each would emit its own value for this metric. This would cause the gauge to flop between values. This metric is listed as an important kpi for capi scaling https://docs.cloudfoundry.org/running/managing-cf/scaling-cloud-controller.html#cloud_controller_ng. This fix for puma will instead uses Redis for the gauge. * Move requests_metric to use statsd_updater * Statsd_updater now contains statsd request logic * Add Redis/Inmemory store to statsd_updager * Add a missing mutux to counter for thread safety for the in memory implementation. * Chose to prefix the entry in redis with `metrics:` but open to feedback here. Inspired by cloudfoundry@4539e59 An alternative considered, was to read the prometheus metric and re-emit that to StatsD, however we observed performance degradation. Presumably because of the number of reads from disk for the [DirectFileStorage](https://github.com/prometheus/client_ruby?tab=readme-ov-file#directfilestore-caveats-and-things-to-keep-in-mind) to aggregate the metric across processes.
Samze
force-pushed
the
puma_cc.requests.outstanding.gauge_redis_refactor
branch
from
8a280d7
to
9b0ed8f
Compare
|
Code looks good, I'll do acceptance tomorrow |
philippthun
approved these changes
Jun 14, 2024
joaopapereira
approved these changes
Jun 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#1312 introduced
cc.requests.outstanding.gaugewhich holds the counter in memory. With the introduction of puma there may be multiple processes, so each would emit its own value for this metric. This would cause the gauge to flop between values. This metric is listed as an important kpi for capi scaling https://docs.cloudfoundry.org/running/managing-cf/scaling-cloud-controller.html#cloud_controller_ng.This fix for puma will instead uses Redis for the gauge.
metrics:but open to feedback here.Inspired by 4539e59
An alternative considered, was to read the prometheus metric and re-emit that to StatsD, however we observed performance degradation. Presumably because of the number of reads from disk for the DirectFileStorage to aggregate the metric across processes and so Redis seemed the best approach.
cc @sethboyles / @pivotalgeorge
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests