Skip to content
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

Add RedisTxn storage adapter. #107

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

arris-ray
Copy link

@arris-ray arris-ray commented Jan 16, 2023

Problem

The RedisNG storage adapter exhibits significant performance degradation as the number of registered metrics increases. This behavior is most notable with Summary metrics but the statement applies for all metric types. The screenshots below shows {p50, p75, p90, p99} timings for collecting {1000, 2000, 5000, 10000} sample Summary metrics across 10 runs in each case. Generally, we can see it takes roughly 4 seconds for a RedisNG adapter to collect 10000 summary metrics.

This behavior seems to stem from the fact that the collection process for each type of metric involves:

  • pulling a set of keys names from Redis
  • looping over each Redis key in PHP
  • and issuing one or more subsequent Redis commands per metric Redis key

This results in network activity that scales with the number of metrics and becomes prohibitively expensive with thousands or tens-of-thousands of metrics.

Reproducing the Problem

This PR adds a benchmark test which was used to generate the data behind the screenshot above. After running docker-compose up to fire up all the configured services, shell into the phpunit container and run the following command:

vendor/bin/phpunit tests/Test/Benchmark --group Benchmark

Once the test completes, it should generate a benchmark.csv file in the /var/www/html directory that you can inspect or import into your favorite spreadsheet for analysis. I've imported a sample run into Google Sheets here for convenience.

Proposed Solution

Optimize network communication with Redis such that all update and collect operations occur within a single transaction.

Changelist

  • Adds RedisTxn storage adapter and supporting classes which:
    • Optimizes network communication with Redis
    • Implements TTL support for all metric types
  • Adds a Benchmark test framework
  • Adds/updates test coverage for new RedisTxn storage adapter
  • Updates php-fpm Dockerfile to vendor Composer dependencies into the Docker image

Benchmark Timings

Screenshot taken from this spreadsheet which also contains the source timing data.

image

image

image

image

@arris-ray arris-ray force-pushed the ar/add-new-redis-adapter branch 3 times, most recently from d5b7ee6 to 9257384 Compare January 16, 2023 22:58
*
* @todo Only summary metrics have been refactored so far. Complete refactor for counter, gauge, and histogram metrics.
*/
class RedisTxn implements Adapter
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adapter is based on RedisNg and currently, the only changes are in the following methods:

  • updateSummary()
  • collectSummaries()

If there is interest from the repo maintainers, I'd like to apply the same style of refactoring for the remaining metric types:

  • counters
  • gauges
  • histograms

Copy link
Author

@arris-ray arris-ray Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All metric types have been optimized. Based on benchmark timing, there may be further opportunity to optimize histograms but that will probably be left as an exercise for a future contributor.

@arris-ray
Copy link
Author

@LKaemmerling You seem like an active maintainer on this project and I was curious to gauge level of interest in a change like this PR. No rush but would love feedback on whether y'all think this is worth pursuing or general advice on making this worth your consideration to merge.

@LKaemmerling
Copy link
Member

Hey @arris-ray,

this sounds interesting and of course, we (or better said I, as at the moment I'm the only maintainer) are open to every contribution!

@arris-ray arris-ray force-pushed the ar/add-new-redis-adapter branch 3 times, most recently from 21673b9 to cd6ca11 Compare January 22, 2023 05:29
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
Signed-off-by: Arris Ray <[email protected]>
@arris-ray arris-ray marked this pull request as ready for review January 23, 2023 02:57
@arris-ray
Copy link
Author

Thanks for the encouragement, @LKaemmerling! This is probably ready for review when you're available.

Apologies in advance for the massive changeset. If there's anything I can do to help make the review more manageable, please don't hesitate to ask!

@pluk77
Copy link
Contributor

pluk77 commented Oct 5, 2023

@LKaemmerling Is there a reason this PR is not being merged other than time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants