catalyst / moodle-tool_excimer

A Moodle tool to find bottlenecks in your code safely in production
https://moodle.org/plugins/tool_excimer
GNU General Public License v3.0
13 stars 9 forks source link

Probabilistic counting of pages is not working correctly #336

Closed brendanheywood closed 7 months ago

brendanheywood commented 7 months ago

If I load a single page:

delete from mdl_tool_excimer_page_groups;

purge caches

ab -c 1 -n 4 https://master.localhost/admin/tool/heartbeat/croncheck.php

This loads 4 pages, in serial so there shouldn't be any issues with concurrency

This results in an approx count of 2 - 4 which is about right. If I repeat those same commands another 3 times then I'd expect 8-16 on average but its still on 2-4.

If I run 64 of them then still nothing:

ab -c 1 -n 64 https://master.localhost/admin/tool/heartbeat/croncheck.php

I think refactor approximate_increment so that it takes 2 values which is the current value and the increment value so we don't need to call it in a loop.

For example if the current value is 4, which means we have roughly 2^4 "things" = 16 and we add an increment of 16 seconds then we know for sure that we should increment to 2^5 = 32. We don't need to call the increment 16 times.

We should also write tests for this and have the increment function also optionally accept a random number so we can make the unit tests deterministic.

brendanheywood commented 7 months ago

There is probably a couple bugs with this, at minimum the cache is not being saved when the increment happens, so it is resaving over the old, lower value, on top of the new incremented value so its not ever going up

bwalkerl commented 7 months ago

The main bug here appears to be $month having a string type in get_page_group(string $name, string $month). This is serialized to create a key, but outside of this function $month is stored as an int, so the cachekeys this function creates don't match the cachekey getting updated.