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

Closes #7161: Host Google Fonts - Measure time #7163

Merged

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Dec 3, 2024

Description

Fixes #7161
Nothing will impact the user.

Type of change

  • Chore

Detailed scenario

No special scenario, just enable the feature host google font and load a page. Within WP Rocket debug file you'll find the execution time logged.

Technical description

Documentation

This pull request introduces performance measurement and logging for the rewrite_fonts method in the Frontend/Controller.php file. The most important changes include adding start and end time measurements and logging the total execution time for the method.

Performance measurement and logging:

New dependencies

None

Risks

None

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots 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.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Unticked items justification

No tests required for this tasks.

@Miraeld Miraeld added this to the 3.18 milestone Dec 3, 2024
@Miraeld Miraeld requested a review from a team December 3, 2024 17:07
@Miraeld Miraeld self-assigned this Dec 3, 2024
@Miraeld Miraeld linked an issue Dec 3, 2024 that may be closed by this pull request
Copy link

codacy-production bot commented Dec 3, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.01% (target: -0.10%) 100.00% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (b96427d) 38431 16873 43.90%
Head commit (37ad77a) 38438 (+7) 16880 (+7) 43.91% (+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 (#7163) 7 7 100.00%

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 stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@MathieuLamiot
Copy link
Contributor

cc @piotrbak @DahmaniAdame This PR should allow you to further investigate timing of the feature if you want.

@Miraeld Miraeld force-pushed the chore/7161-add-execution-time-mesurement branch from 5be349b to 07f9ca3 Compare December 4, 2024 08:16
@Miraeld
Copy link
Contributor Author

Miraeld commented Dec 4, 2024

Update status of this PR:

This pull request introduces changes to the rewrite_fonts function in the inc/Engine/Media/Fonts/Frontend/Controller.php file to add performance measurement and logging for test purposes. The most important changes include the addition of time measurement, font counting, and logging of execution time and font counts.

Performance measurement and logging:

  • Added start time measurement at the beginning of the rewrite_fonts function to track execution time.
  • Counted the number of fonts processed, including separate counts for v1_fonts and v2_fonts.
  • Added end time measurement and logged the total execution time along with the number of fonts processed and their breakdown.

Copy link
Contributor

@hanna-meda hanna-meda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Miraeld, for this PR.
We can now see measurements in logs for duration & fonts processed:
Screenshot 2024-12-04 at 18 18 58

FYI, @DahmaniAdame & @piotrbak

@remyperona remyperona added the type: sub-task Indicates the issue is a sub-task linked to an epics card label Dec 4, 2024
@hanna-meda hanna-meda merged commit 0c3c6fa into feature/host-google-fonts Dec 5, 2024
14 checks passed
@hanna-meda hanna-meda deleted the chore/7161-add-execution-time-mesurement branch December 5, 2024 06:07
@DahmaniAdame
Copy link
Contributor

The count count( $v1_fonts ) and count( $v1_fonts ) is wrong.

You are basically counting how many items are on this array:

array (
    0 => array (
        0 => '',
        1 => '"',
        'url' => 'https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Montserrat+Underline:ital,wght@0,100..900;1,100..900&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Montserrat+Underline:ital,wght@0,100..900;1,100..900&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Yuji+Mai&family=Montserrat+Underline:ital,wght@0,100..900;1,100..900&family=Open+Sans:ital,wght@0,300..800;1,300..800&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Yuji+Mai&display=swap',
        2 => 'https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Montserrat+Underline:ital,wght@0,100..900;1,100..900&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Montserrat+Underline:ital,wght@0,100..900;1,100..900&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Yuji+Mai&family=Montserrat+Underline:ital,wght@0,100..900;1,100..900&family=Open+Sans:ital,wght@0,300..800;1,300..800&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&family=Yuji+Mai&display=swap',
    ),
)

It will always return 1.

What needs to be counted is how many font URL are on the font CSS file referred to on url. It can be done by checking the recurrence of fonts.gstatic.com on it.

@DahmaniAdame
Copy link
Contributor

Also, to get a meaningful result, we need to show on the logs an average loading time per font. Basically $duration / $total_fonts in addition to individual $duration and $total_fonts`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: sub-task Indicates the issue is a sub-task linked to an epics card
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Host Google Fonts - Measure time
6 participants