- Issue created by @berdir
- Merge request !10958Add methods to assert cache get operations by bin and allow to assert specific cids → (Open) 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',
- 🇨🇭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.
- 🇨🇭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.
- 🇪🇸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. - 🇬🇧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..