Aggregate cache operations per bin to allow for more specific asserts

Created on 19 January 2025, 3 days ago

Problem/Motivation

Per 📌 Review cache bin and cache tags of access policy caching Active , performance tests currently throw all cache get/set/delete operations in one single count per operation.

However, different bins have very different performance implications, specifically fast chained bins are way faster. Issues like 📌 Review cache bin usage of ThemeExtensionList and Theme cache context Active that move caches from regular to fast bins are not "seen" by performance tests at all.

Additionally, just having the count is a bit of a blackbox still. It's not always clear what/why there is a change. Maybe we should just store all cids as well so to simplify debugging and allow to compare/assert more specifically if so desired?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component

phpunit

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • 🇨🇭Switzerland berdir Switzerland

    Pushed a proposal, right now PerformanceData is a pure value object, I added a tiny bit of logic to sum up the counts, I also kept the existing set method calls and aggregations for BC.

    I also updated a handful of tests to show this including an additional assert on the cache gets on default. The second non-chained-fast bin with several get operations (7) is data, but I think we don't want to explicitly assert that, no idea how stable those hashes are:

    0 => 'route:[language]=en:[query_parameters]=:/en',
    1 => 'css:umami:umami:enhDWZUCRilCXGGt6daBK7eVBZ-TLhK-AMv4cB0y_qG7c1',
    2 => 'js:umami:en:u_rNnKlC9ds79tQ_PVLa3cfvZ4SZfSwy9XHF-AfxWqA11',
    3 => 'route_provider.route_load:e12969cb977b5b9f14e8b365ac441ec725a833d6586e87d7bcfa7b2c5fc214e2e814196e86f1e6d007e3e8b5703635782521189223a9fa225bcebdbdaf38d833',
    4 => 'js:umami:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11',
    5 => 'js:umami:en:u_rNnKlC9ds79tQ_PVLa3cfvZ4SZfSwy9XHF-AfxWqA11',
    6 => 'route_provider.route_load:61d3f1526762a7ca8ed3aa9d059d2b475e3fb6a5c3f584efd6daaf7d5b942ab3268da173e50ca03f4166dbbb9e821bebb435727ee84eec58c3d67dc1811b5a39',
    
  • Pipeline finished with Canceled
    3 days ago
    Total: 741s
    #400084
  • 🇨🇭Switzerland berdir Switzerland

    There is definitely something very funky going on with those performance test assertions on CI, similar to the language caching issue.

    I accidentally did this on top of 📌 Review cache bin and cache tags of access policy caching Active . The counts are definitely, 100%, wrong. But the test didn't fail?

    Also need to rerun the other tests, they are also wrong.

  • Pipeline finished with Success
    3 days ago
    Total: 3112s
    #400088
  • Pipeline finished with Success
    2 days ago
    Total: 1025s
    #400237
  • 🇨🇭Switzerland berdir Switzerland

    Updated the tests.

    I've pulled in the performance test changes from Introduce a list of "common cache tags" to reduce lookup query amount Active and updated the counts to reflect 11.x, then we have a baseline to be able to see improvements.

  • Pipeline finished with Success
    2 days ago
    Total: 2344s
    #400292
  • 🇪🇸Spain plopesc Valladolid

    Made a quick review and code looks good to me. This is a great enhancement that will help identify scenarios where the current metrics feel somewhat opaque.

    Left a comment about how these new metrics could be checked in assertMetrics() method, but that could be discussed in a follow up if necessary, I think.

  • Pipeline finished with Success
    1 day ago
    Total: 527s
    #401120
  • 🇬🇧United Kingdom catch

    Detail like cache tags I was mostly relying on the grafana/tempo integration (via https://github.com/tag1consulting/ddev-gander) for this kind of granular information, so that it wouldn't be too overwhelming for people to write tests. However, as with database queries, once the tests are written and you can see what's going on much easier, it is all worth it and really helps to understand the impact of a change.

    Where it will not be that useful to keep track of, we can just not assert on it if we don't want to, so why not?

    Haven't reviewed the MR properly yet, but general +1 for getting more granular..

Production build 0.71.5 2024