Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest

Created on 14 December 2023, over 1 year ago

Problem/Motivation

๐Ÿ“Œ Allow assertions on the number of database queries run during tests RTBC added a way to assert database query and cache get/set/delete counts to PerformanceTestBase tests, however it only added assertions to StandardPerformanceTest.

Since we already have performance tests for Umami, we should add assertions to the existing tests. OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest

Steps to reproduce

Proposed resolution

Remaining tasks

Copy how StandardPerformanceTest asserts on database queries, cache hits, sets and gets. Sometimes the number of queries isn't the same each time, so greater and less than assertions have to be used.

 $performance_data = $this->collectPerformanceData(function () {
      $this->drupalGet('');
    });
    // This test observes a variable number of cache operations and database
    // queries, so to avoid  random test failures, assert greater than equal
    // the highest and lowest number of observed during test runs.
    // See https://www.drupal.org/project/drupal/issues/3402610
    $this->assertGreaterThanOrEqual(58, $performance_data->getQueryCount());
    $this->assertLessThanOrEqual(67, $performance_data->getQueryCount());
    $this->assertGreaterThanOrEqual(129, $performance_data->getCacheGetCount());
    $this->assertLessThanOrEqual(132, $performance_data->getCacheGetCount());
    $this->assertGreaterThanOrEqual(59, $performance_data->getCacheSetCount());
    $this->assertLessThanOrEqual(68, $performance_data->getCacheSetCount());
    $this->assertSame(0, $performance_data->getCacheDeleteCount());

The easiest way to find out the correct assertions, is to add obviously wrong assertions to the tests (like asserting 0), run the individual test method, then change the number to the one you get from the assertion failure, then keep running the tests until they stop failing. Adding debug (like dump($performance_data->getCacheSetCount()); will speed this process up.

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ“Œ Task
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component
PHPUnitย  โ†’

Last updated about 3 hours ago

Created by

๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • ๐Ÿ‡ธ๐Ÿ‡ฎSlovenia slashrsm
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom daniel.j

    daniel.j โ†’ made their first commit to this issueโ€™s fork.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom daniel.j

    daniel.j โ†’ changed the visibility of the branch 11.x to hidden.

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone

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

  • Pipeline finished with Failed
    about 1 month ago
    Total: 80s
    #434795
  • Pipeline finished with Failed
    about 1 month ago
    #434798
  • Pipeline finished with Running
    about 1 month ago
    #434817
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom daniel.j

    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!

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    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!

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • Pipeline finished with Failed
    14 days ago
    Total: 414s
    #452113
  • Pipeline finished with Failed
    14 days ago
    Total: 456s
    #452125
  • Pipeline finished with Failed
    14 days ago
    Total: 598s
    #452131
  • Pipeline finished with Running
    14 days ago
    Total: 438s
    #452162
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom daniel.j

    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!

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

Production build 0.71.5 2024