Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache()

MR appears to have failures.

Failures were false. MR passes tests after a relaunch

Will leave for @catch but same question if we really need a super long query

The long query array comparison is useful, if we have a change that simultaneously adds and removes a query, the count by itself would not detect the change, but this will.

One small comment on the MR, but this looks good otherwise.

Leaning on @catch eyes but feedback appears to be addressed.

This looks fine to me. I am not keen on the wall of text for $expected_queries but another solution is not coming to mind. I did use the suggestion feature to add some comments to explain why the actual queries are asserted, taken from catch's comment above. I then more comments to break up the assertions into groups so it was easier to read.

Leaving at RTBC.

This looks great! Nice to see the issues from the DrupalCon performance testing workshop get to RTBC.

Committed/pushed to 11.x, thanks!

catch → closed merge request !9598

This is change is causing failed test runs - see https://git.drupalcode.org/project/drupal/-/pipelines/316946/test_report...…]B1%EF%B8%8F%EF%B8%8F%EF%B8%8F+PHPUnit+Functional+Javascript for an example.

Even if I change the 252 to 251 locally the test still fails because $this->assertCountBetween(6500, 7500, $performance_data->getScriptBytes()); fails. We need to ensure that this tests are runnable locally and if they have preconditions (like apcu) that must be met then we error with something decent.

berdir → changed the visibility of the branch 3476422-add-assertions-to to active.

I suspect the test fails here were related to the JS problems we had with not running tests. The script bytes count was way off.

I also looked at adding an explicit CacheTagGroupedLookups assertion, but it's a lot of cache tags. many, many blocks, many image styles and entities.

Like in a similar issue I commented on, I'm not too fond on asserting those 170 queries 1:1. It's a lot. If umami changes it's default content, resulting in different ids then it's going to be a pain to update those performance tests. Or a new field or something like that. Also, the default_content api of recipes/default_content module doesn't guarantee ids, so if we switch to recipes, then we can't really rely on that anymore. My vague idea is that we'd filter out any entity load queries and just replace them with counts. So we'd assert 172 queries total, 100 node load queries, 20 media load queries. Or maybe even go a step further and handle it like cache lookups. we don't track single node load queries (except maybe one specific test, but just track how many nodes we've loaded, in groups, similar to how we assert cache and cache tag operations now. Or yet another option, we run the test on warm entity caches by loading all nodes upfront.

I added the grouped cache tag lookups now to this, mostly to test other issues, such as 📌 Use a single per-theme cache tag for block config entities Active .

Still have concerns about the level of detail we have here and it's stability and how annoying it will be to maintain it.

As visible from the cache tags, there's a lot going on on those pages, including 3 different views for example.

Or yet another option, we run the test on warm entity caches by loading all nodes upfront.

I think we could be more selective in the ::clearCaches() call such as only clearing the dynamic page cache and render caches maybe.

Going to take a chance and assume this one is good to go. Difficult to review fully admit.

No see #20 and #21 this is still under active discussion.

I played a bit with this. FWIW, even with not clearing entity caches, there are still 120 queries left. Looking a bit closer, what's interesting in that query list is that we load the same node *revision* 7 times. Because loadRevision() has neither static nor persistent cache: 📌 ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() Needs work They're all from the local tasks block route upcasting, because we need to do that to figure out access to the route. And we have a lot of routes that now want the active entity. That's around 80 queries just for that.

@berdir that seems worth adding to the performance test here so we can see the improvement in that issue. Tedious in the test, but it will reflect the medium of our poor database servers doing the same thing over and over again unnecessarily.

I'm torn on this. It's a lot, but we can also use this in several issues such as 📌 Improve cache dependencies in ContentTranslationDeleteAccess Active to show the improvements.

I'd say we get it in and if it turns out to be annoying or causes random fails, we remove it again.

Yeah we could either remove it or change the scope of the test to clear less caches if there are unpredictable ones.

Per #26 since I didn’t work on this one

The failed test in Open Telemetry Node Page Performance (Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformance) -= so probably related to this change :)

Just needs the assertion updating because core has shifted since it was added, but yes.

Always find it hard with those ranges, went 1k down. Or should we set it to thst specific value?

Changed on my phone from Fondue dinner at DrupalMountainCamp.

@berdir I usually set it to the nearest 50 bytes more or less - there's intentional leeway built in so that small refactorings of CSS/Js don't result in test failures,so avoiding the specific number of bytes makes it more obvious we're dealing with an approximation I think, but also it doesn't really matter as long as it's somewhere close to the middle.

That makes sense, adjusted 41738 to 41750.

Moving this back to RTBC after the assertion update.

Only needed a rebase and trivial changes since the RTBC so I am going to go ahead and commit this one - will make it a lot easier to see changes via other issues with the additional test coverage. Committed/pushed to 11.x, thanks!

catch → closed merge request !11240

Automatically closed - issue fixed for 2 weeks with no activity.