Closes #6571: Reduce the processing on not-cached page when checking the LCP/ATF data #6610

Miraeld · 2024-05-03T11:02:06Z

Description

Documentation

User documentation

It won't try to check if LCP data are already existing for non-cached URLs.

Technical documentation

We check if the page is a cached version, if it isn't we will bailout before sending an AJAX call to check if LCP datas are already existing for the URL.

Type of change

Bug fix (non-breaking change which fixes an issue).
Enhancement (non-breaking change which improves an existing functionality).

New dependencies

N/a

Risks

N/a

Checklists

Feature validation

I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
I triggered all changed lines of code at least once without new errors/warnings/notices.
I implemented built-in tests to cover the new/changed code.

Documentation

I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

I wrote self-explanatory code about what it does.
I wrote comments to explain why it does it.
I named variables and functions explicitely.
I protected entry points against unexpected inputs.
I did not introduce unecessary complexity.
I listed the introduced external dependencies explicitely on the PR.
I validated the repo-specific guidelines from CONTRIBUTING.md.

… above the fold table

codacy-production · 2024-05-03T11:07:17Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.00% (target: -0.10%)	✅ ∅ (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`74186d3`)	37122	14325	38.59%
Head commit (`24e6a11`)	37122 (+0)	14325 (+0)	38.59% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6610)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

jeawhanlee · 2024-05-03T11:19:59Z

assets/js/lcp-beacon.js

+		if (document.documentElement.nextSibling)  {
+			signature = document.documentElement.nextSibling.data;
+		}
+		if ( signature && signature.includes( 'Debug: cached' ) ) {


I think we should check for something more reliable maybe like the whole sentence as there are some cases where we do not generate timestamp in the footprint.

I used the same condition as on WPR Inspector (https://github.com/wp-media/wpr-inspector/blob/bd71796388de3c9cc0cff45488de23ce920df6d1/wpr-inspector/inspector.js#L25)

assets/js/lcp-beacon.js

wordpressfan

We need to bear in mind that in some hosts our cache is disabled by default also the signature can be removed in cases like when having the whitelabel constant or so.

I need to understand more how do u detect the cached page in depth.

assets/js/lcp-beacon.js

Miraeld · 2024-05-03T12:45:37Z

@wordpressfan for these cases, I guess we won't have the choice to do the DB request we want to avoid which is the one checking if LCP data does exists for the page while it isn't even cached yet.

We can't check through the DB as it would add another DB Request, which is the opposite of what we want, and there are no other way to determine if a page is cached or not. Except if there is something else I'm missing.

The way the current detection work is through the signature, where if the page is cached, we get the following text: Debug: cached@1714733510
Where 1714733510 is a timestamp.

Miraeld · 2024-05-10T20:36:10Z

@jeawhanlee & @wordpressfan Are you okay with the latest changes ?

jeawhanlee · 2024-05-11T20:05:39Z

For me I have issues with how reliable it can be detecting a cached page with our footprint signature, what if we tried checking the directory if the cache file exists and pass the result (boolean) to the script.

jeawhanlee · 2024-05-11T20:19:47Z

For me I have issues with how reliable it can be detecting a cached page with our footprint signature, what if we tried checking the directory if the cache file exists and pass the result (boolean) to the script.

The above won't work, when the page is cached, it would bail out and render the cache.

In my opinion, If we would use the signature, we should use parts that must always be present instead of just Debug: cached
Here is an example of a page without the timestamp:

MathieuLamiot · 2024-05-13T14:58:38Z

@Miraeld Looking at wp-rocket's code, there are versions of the code that don't have the timestamp in the signature I think. Typically in case of 3rd party hosting, white label and if time is empty.
In that case, it would make sense indeed to look for:

'<!-- Optimized for great performance'
OR
'<!-- This website is like a Rocket, isn't it? Performance optimized by '

Then, of course, it is not perfect because sometimes, comments are removed from cached files. But better done than perfect.

WDYT of matching on those two rather than Debug + timestamp? In any cases, better done than perfect: Can we just change the matching to those 2 strings, move forward with this PR and eventually open a follow-up GH issue if there are edge cases you still foresee?

Miraeld · 2024-05-14T06:55:38Z

So based on our conversation yesterday, I guess we have to merge this PR and prepare another issue to enhance it,
@wordpressfan as you had the idea for the enhancement, can you create the new issue, and when it's done can you merge this PR ?

MathieuLamiot · 2024-05-14T15:31:43Z

@wordpressfan @jeawhanlee Since @Miraeld will be off end of this week, can you make sure to close this? Thanks 🙏

wordpressfan

Approved with very minor enhancement

assets/js/lcp-beacon.js

codacy-production · 2024-05-15T10:33:52Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01% (target: -0.10%)	✅ ∅ (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`74186d3`)	37122	14325	38.59%
Head commit (`02032cd`)	37174 (+52)	14349 (+24)	38.60% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6610)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

MathieuLamiot · 2024-05-21T15:38:02Z

Expected behavior and observation from QA team discussed here: https://wp-media.slack.com/archives/CUT7FLHF1/p1716202320587489

Mai-Saad · 2024-05-23T08:40:40Z

The issue mentioned here #6610 (comment) is fixed, but on PR, nothing is added to the database with warmup (neither after a fresh install, nor after clear critical images)

steps:
1-fresh install of PR => check ATF table, nothing added although WPR debug contains some URLs to add to queue
2- clear critical images => check ATF, still nothing is added to database

Note: related test plan https://wpmediaqa.testrail.io/index.php?/runs/view/856&group_by=cases:section_id&group_order=asc

wordpressfan · 2024-05-23T10:44:03Z

All jobs from that site in SaaS are returning timeout error, I'm checking with Mostafa now what is going on there.

wordpressfan · 2024-05-23T10:57:03Z

This was because in SaaS we load the not cached page which doesn't include the signature and this means that the following line returns js error in the console:

wp-rocket/assets/js/lcp-beacon.js

Line 51 in a6b7622

    
           const signature = document.documentElement.nextSibling.data ? document.documentElement.nextSibling.data : '';

because document.documentElement.nextSibling is null in this case, pushing a fix now.

Mai-Saad

Working as expected
testrail-report-605.pdf

Closes #6571: Adds a bailout if the page isn't cached before checking…

d753fca

… above the fold table

Miraeld added type: bug Indicates an unexpected problem or unintended behavior type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels May 3, 2024

Miraeld added this to the 3.16 milestone May 3, 2024

Miraeld requested a review from a team May 3, 2024 11:02

Miraeld self-assigned this May 3, 2024

jeawhanlee requested changes May 3, 2024

View reviewed changes

wordpressfan requested changes May 3, 2024

View reviewed changes

assets/js/lcp-beacon.js Outdated Show resolved Hide resolved

Miraeld added 3 commits May 3, 2024 14:38

Fix log remove comment

fa5f462

Comply feedback

cf4e0e0

Comply feedback

24e6a11

piotrbak removed this from the 3.16 milestone May 3, 2024

Miraeld requested review from jeawhanlee and wordpressfan May 3, 2024 13:28

wordpressfan approved these changes May 15, 2024

View reviewed changes

assets/js/lcp-beacon.js Outdated Show resolved Hide resolved

minor enhancement

c22d938

build minified script

269235a

MathieuLamiot mentioned this pull request May 17, 2024

Reduce the processing on not-cached page when checking the LCP/ATF data #6571

Closed

wordpressfan mentioned this pull request May 17, 2024

Enhance the way we detect the cached page for lcp beacon script #6637

Open

4 tasks

use async/wait correctly

a6b7622

fix not found signature error

02032cd

Mai-Saad approved these changes May 23, 2024

View reviewed changes

Mai-Saad added this pull request to the merge queue May 23, 2024

Merged via the queue into develop with commit 3877532 May 23, 2024
15 checks passed

Mai-Saad deleted the fix/6571-add-bail-out-if-page-is-not-cached branch May 23, 2024 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #6571: Reduce the processing on not-cached page when checking the LCP/ATF data #6610

Closes #6571: Reduce the processing on not-cached page when checking the LCP/ATF data #6610

Miraeld commented May 3, 2024

codacy-production bot commented May 3, 2024 •

edited

jeawhanlee May 3, 2024

Miraeld May 3, 2024

wordpressfan left a comment

Miraeld commented May 3, 2024

Miraeld commented May 10, 2024

jeawhanlee commented May 11, 2024

jeawhanlee commented May 11, 2024

MathieuLamiot commented May 13, 2024 •

edited

Miraeld commented May 14, 2024

MathieuLamiot commented May 14, 2024

wordpressfan left a comment

codacy-production bot commented May 15, 2024 •

edited

MathieuLamiot commented May 21, 2024

Mai-Saad commented May 23, 2024 •

edited

wordpressfan commented May 23, 2024

wordpressfan commented May 23, 2024

Mai-Saad left a comment •

edited

Closes #6571: Reduce the processing on not-cached page when checking the LCP/ATF data #6610

Closes #6571: Reduce the processing on not-cached page when checking the LCP/ATF data #6610

Conversation

Miraeld commented May 3, 2024

Description

Documentation

User documentation

Technical documentation

Type of change

New dependencies

Risks

Checklists

Feature validation

Documentation

Code style

codacy-production bot commented May 3, 2024 • edited

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

jeawhanlee May 3, 2024

Choose a reason for hiding this comment

Miraeld May 3, 2024

Choose a reason for hiding this comment

wordpressfan left a comment

Choose a reason for hiding this comment

Miraeld commented May 3, 2024

Miraeld commented May 10, 2024

jeawhanlee commented May 11, 2024

jeawhanlee commented May 11, 2024

MathieuLamiot commented May 13, 2024 • edited

Miraeld commented May 14, 2024

MathieuLamiot commented May 14, 2024

wordpressfan left a comment

Choose a reason for hiding this comment

codacy-production bot commented May 15, 2024 • edited

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

MathieuLamiot commented May 21, 2024

Mai-Saad commented May 23, 2024 • edited

wordpressfan commented May 23, 2024

wordpressfan commented May 23, 2024

Mai-Saad left a comment • edited

Choose a reason for hiding this comment

codacy-production bot commented May 3, 2024 •

edited

MathieuLamiot commented May 13, 2024 •

edited

codacy-production bot commented May 15, 2024 •

edited

Mai-Saad commented May 23, 2024 •

edited

Mai-Saad left a comment •

edited