PromPHP / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
415 stars 91 forks source link

Add RedisTxn storage adapter. #107

Open arris-ray opened 1 year ago

arris-ray commented 1 year ago

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:

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

Benchmark Timings

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

image
image image image
arris-ray commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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