Problem/Motivation

#3395776: Make POST requests render cacheable didn't show as much of a performance improvement as you'd expect in StandardPerformanceTest, because the standard profile with no content doesn't have much to render.

Steps to reproduce

Proposed resolution

Couple of options:

1. Move login testing to umami which already has some content. This would require enabling the login block in the Umami test (but we also do that in the standard test, it's just not 'pure' Umami to have it enabled).

2. Add ten nodes instead of one to the standard front page so that more rendering happens.

We could also add a test that submits the wrong password for a user so that the login block fails validation, this will cause the entire page to be rendered without a redirect - and is quite important for user-facing performance that this comes back quickly, may affect 'interation to next paint'/INP etc.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 3426303-nr-bot_8sfdze4p.txt98 bytesneeds-review-queue-bot
#16 3426303-nr-bot_kyt0ih11.txt1.29 KBneeds-review-queue-bot

Issue fork drupal-3426303

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

If we start moving more tests to the Umami tests, we might lose track of what the performance of a "clean" Drupal is. Would it make sense to have the same tests run where one is using "demo_umami" and the other is using "standard"? Then with any change to core we can see how it improves performance on a very minimalistic site vs a very realistic one.

catch’s picture

catch
he/him
Primary language English
commented

Given the install profile is a property on the test class it'd probably be easier to duplicate the coverage (or add an extra trait with the test methods), but we can definitely do that and agree it's good to test both cases at least for things that are different. Reminds me we need to add query/cache assertions to all the Umami performance tests now that counts are stabilised etc.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Reminds me we need to add query/cache assertions to all the Umami performance tests now that counts are stabilised etc.

I think I did the query part already for those tests that were previously checking query counts.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👀

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

catch
he/him
Primary language English
commented
Title: Improve the standard performance login test » Moving login performance testing to Umami
Status: Active » Needs work

Re-titling.

I think for login testing we don't need coverage between standard and Umami, just the one should be enough. We should keep some testing of a minimal install though and that's being figured out in #3608939: Move/split remaining parts of StandardPerformanceTest and StandardJavascriptTest

catch opened merge request !16655

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
catch’s picture

catch
he/him
Primary language English
commented

Realised the login test isn't compatible with the OpenTelemetry dashboard because that relies on LCP/TTFB which don't work for multi-request tests, and which is populated via this test. But I think we can move the logic to an UmamiLoginPerformanceTest instead.

if you're ok with not testing the login block anymore then fine by me, I think it's not much different from the regular form?

I guess the question is whether the form itself causes some kind of caching issue by being on the page, but the vast majority of sites these days have a login link rather than a block on every page. So yeah I think it's OK to drop. We found some interesting things via the login test itself but that should be the same (more or less) however it happens.

catch’s picture

catch
he/him
Primary language English
commented

Also building on #11, the 'across pages' tests in OpenTelemetryPerformanceTest also don't really make much sense on the dashboard either. So I think we should go for an UmamiMultipleRequestsPerformanceTest that handles visiting multiple pages, and also logging in which happens across multiple requests.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebuilt the branch/MR here.

- new MultipleRequestsPerformanceTest so that we don't send not-useful things to the open telemetry/gander dashboard

- moved login testing from standard to that new class

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Reviewed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

I think I got all of the review points.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3426303-nr-bot_kyt0ih11.txt1.29 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. I'm not entirely sure this fully this covers the original idea of testing render caching on post requests, we have some render cache hits, less than standard though. No unexpected queries from something like a views block, but not sure we'd get that on the dedicated login page anyway.

But more importantly now, this is part of #3608939: Move/split remaining parts of StandardPerformanceTest and StandardJavascriptTest and moves one of the last bits from StandardPerformanceTest, making it less weird.

amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Needs work

Left a few comments on the MR.

catch’s picture

catch
he/him
Primary language English
commented
Title: Moving login performance testing to Umami » Move login performance testing to Umami
Status: Needs work » Reviewed & tested by the community

Resolved the comments I think. All trivial changes so moving back to RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3426303-nr-bot_8sfdze4p.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased again.

  • amateescu committed e880b4da on main 
    task: #3426303 Move login performance testing to Umami

By: catch
By:...
amateescu’s picture

amateescu commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed e880b4dac7c to main. Thanks!

Needs a rebased MR for 11.x.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think this is not correct now. You renamed the identifier but not the file, Right now it will not fail on a non-existing file and generate it, also on CI. We could change that to always require the UPDATE constant also if the file doesn't exist yet, it was mostly just a convenience thing while I set it up.

catch opened merge request !16951

catch’s picture

catch
he/him
Primary language English
commented

The file was renamed in the MR already but somehow the identifier in the test got out of sync later so I think it's actually OK? Or I'm missing something.

Put up an 11.x backport.

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Reviewed & tested by the community

Moving to RTBC for the backport. We still might need a follow-up for main if my answer to @berdir is wrong.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

You are correct, the final state seems correct, then it was just not inbetween. I've created #3620550: Only update performance test assertion files if environment variable is set anyway.

  • amateescu committed 00de18c9 on 11.x 
    task: #3426303 Move login performance testing to Umami

By: catch
By:...
amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Fixed

Committed 00de18c and pushed to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.