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

3.16 Bug - Visits from the SaaS do not store the results in the DB #6521

Closed
MathieuLamiot opened this issue Mar 27, 2024 · 8 comments · Fixed by #6557 or #6598
Closed

3.16 Bug - Visits from the SaaS do not store the results in the DB #6521

MathieuLamiot opened this issue Mar 27, 2024 · 8 comments · Fixed by #6557 or #6598
Assignees
Labels
effort: [S] 1-2 days of estimated development time
Milestone

Comments

@MathieuLamiot
Copy link
Contributor

MathieuLamiot commented Mar 27, 2024

Context

With the 3.16 pre-alpha, when the SaaS visits a page, the results of the beacon script are not stored in the DB.
This issue needs #6518 first.

How to reproduce

  • Install 3.16 on a website.
  • Pick a page, clear its cache and its critical images.
  • Trigger a visit from the SaaS, with Postman for instance (or with the warm-up mechanism). For staging, the hostname is https://rucss-director-staging.public-default.live2-k8s-cph3.one.com/
image - Check that the visit is successful on Metabase for instance (for staging [here](https://data.wp-media.me/question#eyJkYXRhc2V0X3F1ZXJ5Ijp7InR5cGUiOiJuYXRpdmUiLCJuYXRpdmUiOnsicXVlcnkiOiJzZWxlY3QgKiBmcm9tIHNhYXNfam9iIGpvaW4gc2Fhc19vcHRpbWl6YXRpb25yZXF1ZXN0IG9uIHNhYXNfam9iLlwib3B0aW1pemF0aW9uUmVxdWVzdF9pZFwiID0gc2Fhc19vcHRpbWl6YXRpb25yZXF1ZXN0LnV1aWQgb3JkZXIgYnkgc2Fhc19qb2IuY3JlYXRlZF9hdCBkZXNjIiwidGVtcGxhdGUtdGFncyI6e319LCJkYXRhYmFzZSI6MTR9LCJkaXNwbGF5IjoidGFibGUiLCJwYXJhbWV0ZXJzIjpbXSwidmlzdWFsaXphdGlvbl9zZXR0aW5ncyI6e319)) - Check the website's DB. There is no data for LCP/ATF.

Expected behavior

  • After a visit from the SaaS, LCP/ATF data must be filled in the database.

Acceptance Criteria

  • Reproduce the "how to reproduce". At the last step, valid data must be stored in the LCP/ATF table for the page, without manual visits.
@MathieuLamiot MathieuLamiot added this to the 3.16 milestone Mar 27, 2024
@MathieuLamiot MathieuLamiot added the effort: [S] 1-2 days of estimated development time label Mar 27, 2024
@MathieuLamiot
Copy link
Contributor Author

MathieuLamiot commented Mar 27, 2024

There is work to be done on the SaaS and on the plugin.

Root cause & suggested solutions

There is a main issue and a secondary one.
The main issue is that, on the SaaS, we intercept the request from the browser, so the AJAX call never makes it to the endpoint. This issue has been solved by removing that part of the code on the SaaS. It should be done with a proper exception. We could use the url that contains admin-ajax.php, or check the content of postData? We add this inside for instance in the script: data.append('action', 'rocket_lcp');

The secondary issue is that the script might not have time to complete in the time the SaaS browses and close the page. To solve this:

  • On the plugin, I added a data-name attribute to the beacon script when injecting it in inject_beacon: $script_tag = "<script data-name=\"wpr-lcp-beacon\" src='{$script_url}' async></script>";. This will allow to identify the script in the following steps.
  • In the script, after the fetch call, I added:
    .then((data) => {
        var beaconscript = document.querySelector('[data-name="wpr-lcp-beacon"]');
        beaconscript.setAttribute('beacon-completed', 'true');
	console.log(data);
	})

This adds the attribute beacon-completed to the script tag when the script finished, so that the SaaS can detect it. Maybe we can use document.currentScript instead of the first line, I don't know...

  • On the SaaS, in navigateToURL, just after the gotoPage, in the same try/Catch, I added:
          await page.waitForFunction(
                "document.querySelector('[data-name=\"wpr-lcp-beacon\"]').getAttribute('beacon-completed') === 'true'",
                { 
                    timeout: 10000
                }
            );

This ensures the SaaS waits for the script to be completed, or wait up to 10 seconds. There is one thing to add: the SaaS should not wait 10s if the beacon script is not found. So maybe first, check that the script is found. If it is, waitForFunction, else just continue. To do this, we can use something like

let scriptFound = await page.evaluate( () => {
                return document.querySelector('[data-name="wpr-lcp-beacon"]') !== null;
            });

Effort: I think this is a XS for the SaaS, and XS for the plugin. So S.

@remyperona
Copy link
Contributor

LGTM

@MathieuLamiot
Copy link
Contributor Author

MathieuLamiot commented Apr 9, 2024

SaaS side: branch ready, waiting for plugin for complete test. feature/6521-3-16-compat-beacon-script

  • Request interception handling based on script name (interceptor.url) -> To validate
  • Added check for LCP beacon found and wait for completion.

On the first point, I'd like to investigate a more sustainable approach with a header. To.whoever implements this on the plugin, let's test rogether once you have a working version

@MathieuLamiot
Copy link
Contributor Author

@Khadreal I managed to validate the behavior on new.rocketlabs with your branch and the one on SaaS. I am putting the SaaS one ready for review, I'll let you wrap-up your branch and put it in review as well :)

@MathieuLamiot MathieuLamiot self-assigned this Apr 12, 2024
@MathieuLamiot MathieuLamiot linked a pull request Apr 12, 2024 that will close this issue
19 tasks
@hanna-meda hanna-meda self-assigned this Apr 24, 2024
@MathieuLamiot
Copy link
Contributor Author

@Khadreal I think there is an issue with your PR: The beacon-completed attribute is set only if the beacon script completes with data to send to the AJAX endpoint.
In case of early bail-out, it is not set, hence the SaaS keeps waiting for it and times out in the end.

Now that we have bail-outs possible, this part of the code should be moved so that it is always done when the script completes:

const t = document.querySelector('[data-name="wpr-lcp-beacon"]');
t.setAttribute("beacon-completed", "true"),
console.log(e)

@hanna-meda It could be linked to the issue you reported with homepage on new.rocketlabs. To re-check after fixing this.

@hanna-meda
Copy link
Contributor

Related Test Run HERE

@MathieuLamiot
Copy link
Contributor Author

@hanna-meda I think the test plan should also ensure that valid results are stored in the DB (see the original description). The SaaS visit can be successful but nothing in the DB if there is a bug on the plugin side for instance.

@hanna-meda
Copy link
Contributor

@MathieuLamiot, you're right. Adding steps to check DB - RUCSS/ATF tables (depending on scenarios) in TCs where this check is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment