Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest

daniel.j → made their first commit to this issue’s fork.

daniel.j → changed the visibility of the branch 11.x to hidden.

daniel.j → closed merge request !11245

quietone → reopened merge request !11245

Re-opened the MR so that others can use it to work on this issue.

Hey, I've added performance assertions in 'OpenTelemetryFrontPagePerformanceTest' and 'OpenTelemetryNodePagePerformanceTest'.

I started by asserting exact values and correcting them until the tests passed. Where performance metrics varied on each run, mostly with the cold cache tests, they were moved to using 'assertCountBetween', gradually expanding the acceptable range until the tests passed reliably.

The exact database query strings have not been included (like they are in 'StandardPerformanceTest') as the query counts are either 0 of 100+, which seems like a lot to have hardcoded. However, the exact 'CacheTagGroupedLookups' have been included where the values are reliably the same across test runs.

Additionally, this work introduced the word 'languageswitcher' which failed with 'cspell'. I can see that elsewhere this has been marked 'cspell:ingore'. Instead I've added 'languageswitcher' to the 'core/misc/cspell/dictionary.txt'. This looked like the best place to put it. Is this correct? Would it be good to remove the 'cspell:ignore' with this change?

Are there any additional performance assertions that would be good to include? Are the ones added suitable?

There seem to be some JavaScript tests failing, but these look to be unrelated to this work. I've triggered the performance test on the MR, but as of writing this job is stuck (https://git.drupalcode.org/issue/drupal-3408713/-/jobs/4488014).

It would be great for this work to be reviewed! Thanks!

Appears to need a rebase

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

It looks like this is a duplicate of several child issues of 🌱 [meta] Add additional performance test coverage to core Active one of which just was committed. that one actually happened to add a 100+ list of queries, after quite a bit of discussion around it. Unsure if we should proceed with this or the child issues over there.

Rebased onto 11.x, mostly taking the changes from 11.x including the SQL statements. Also run run the tests and adjusted the metrics as they were failing after the rebase. Removed the uses of the depricated CacheTag functions.

Unsure if we should proceed with this or the child issues over there.

I don't think I'm the right person to answer this, but this work does include some tests that were not added as part of the other duplicate issue, namely limited testing for cold and cool caches as well as listing out in full the expected 'CacheTagGroupedLookups' for several tests. Are these useful additions?

Either way, the MR should be up-to-date and mergable now. Even if it's not merged it was still a good way to get more familiar with Gander. Thanks!

I don't think I'm the right person to answer this, but this work does include some tests that were not added as part of the other duplicate issue, namely limited testing for cold and cool caches as well as listing out in full the expected 'CacheTagGroupedLookups' for several tests. Are these useful additions?

Yes, but the issue that was committed is just one of multiple issues in the meta parent. If you check the child issues listed in the sidebar, there's a dedicated one for each method basically.

I think it's probably OK to merge extending the existing methods into issue, but then we should comment on the 2-3 that are now duplicate and close them to avoid further duplicate work. I also updated this to make it a child of that meta.

Marked 📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageCoolCache() Active as duplicate.

Rebased this one and removed some of the finer details per @berdir's comments which I agree risk duplicating similar coverage elsewhere.

One minor remark.

Removed the redundant dictionary.txt entry.

The query and cache count on those assertions is sometimes very large, so I expect that's going to change quite a bit in some of the ongoing performance related issues.

Might also not hurt to use https://git.drupalcode.org/project/drupal/-/jobs/4929825 to run those tests 100x to check if there are random fails, I'm not allowed to do that though. There were a bunch of random fails in recent pipelines, but none on those two tests.

In some ways, I'd like to see (and assert) for example those queries on a cold cache, but it's just too much right now. We can possibly revisit this after things stabilize and some performance issues such as 📌 ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() Needs work are in. but even then, it's a lot of entities that we're loading on a cold cache.

Started looking over the queries here and found 🐛 Config overrides are loaded for English even when translate_english is false Active plus 📌 Static cache not found entities Active so far, this is on top of the other pre-existing issues that it validates the improvements from.