Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCI & LRC optimizations should not be applied, even if available, on SaaS visits #6964

Closed
MathieuLamiot opened this issue Sep 10, 2024 · 4 comments · Fixed by #7006
Closed
Assignees
Labels
module: ALR Issues related to the Automatic Lazy Rendering feature module: Beacon
Milestone

Comments

@MathieuLamiot
Copy link
Contributor

MathieuLamiot commented Sep 10, 2024

Context
As part of #6944, we saw that on SaaS visits (?nowprocket=1&no_optimize=1&wpr_imagedimensions=1), LRC and OCI optimizations are applied if possible. This is not needed. We could save some CPU usage on this.

This is due to this line:

'rocket_performance_hints_buffer' => [ 'maybe_apply_optimizations', 17 ],

Instead of running the whole maybe_apply_optimizations and therefore, maybe call the optimize methods of the factories, we should only maybe inject the beacon, and bypass the calls to optimize.

Expected behavior
On SaaS visits (?nowprocket=1&no_optimize=1&wpr_imagedimensions=1):

  • if one of the PerformanceHints data is not in DB, then the beacon must be injected.
  • otherwise, the beacon must not be added. the LRC and OCI optimization must not be applied.
    On visits with wpr_lazyrendercontent and not wpr_imagedimensions, the beacon must not be injected (already the case) and the LRC hashes must be added (already the case).

Acceptance Criteria
On a page with no data stored in DB for OCI/LRC:

  • visit the normal page (no query string): hashes are added, beacon is injected.
  • visit ?nowprocket=1&no_optimize=1&wpr_imagedimensions=1: hashes are added, beacon is injected.
  • visit ?wpr_lazyrendercontent: hashes are added, beacon is not injected.

On a page with data stored in DB for OCI/LRC:

  • visit the normal page (no query string): hashes are not, beacon not injected, OCI/LRC optimizations added..
  • visit ?nowprocket=1&no_optimize=1&wpr_imagedimensions=1: hashes are not added, beacon is not injected, OCI/LRC optimizations not added.
  • visit ?wpr_lazyrendercontent: hashes are added, beacon is not injected, OCI/LRC optimizations not added.
@MathieuLamiot MathieuLamiot added module: Beacon module: ALR Issues related to the Automatic Lazy Rendering feature labels Sep 10, 2024
@piotrbak piotrbak modified the milestone: 3.17.1 Sep 22, 2024
@Khadreal
Copy link
Contributor

@MathieuLamiot the accepted criteria is what's currently happening or maybe I'm missing something here

@MathieuLamiot
Copy link
Contributor Author

@Khadreal reproduced here: http://mathieu.e2e.rocketlabsqa.ovh/?nowprocket=1&no_optimize=1&wpr_imagedimensions=1

@Khadreal
Copy link
Contributor

Khadreal commented Sep 26, 2024

Grooming

in this method

public function maybe_apply_optimizations( $html ): string {

we can add a logic check before optimization process begin, check if nowprocket && no_optimize && wpr_imagedimensions is set.

if( isset( $_GET['wpr_imagedimensions'] ) && isset( $_GET['no_optimize'] ) ) {
...
}
OR
if( isset( $_GET['wpr_imagedimensions'] ) && isset( $_GET['no_optimize'] ) && isset( $_GET['nowprocket'] )) {
...
}

Add same logic to add_hashes method here

Estimation

[S]

PS -- I think we can make the logic a method that can be reused for other sass visit, can't think of any at the moment.

@Miraeld
Copy link
Contributor

Miraeld commented Oct 1, 2024

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: ALR Issues related to the Automatic Lazy Rendering feature module: Beacon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants