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

update Performance Report Average Load Time query #18526

Merged
merged 14 commits into from Jan 7, 2022
Merged

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Dec 20, 2021

Description:

Fixes: #17012.
It seems like one of our queries is wrong, update it should work now. But I guess it won't change the previously achieved data.

Review

update mysql query that return right load time
@peterhashair peterhashair changed the title Update Archiver.php update Performance Report Average Load Time query Dec 20, 2021
Peter Zhang added 2 commits December 20, 2021 16:19
remove specialist code
update archiver sum query
@peterhashair
Copy link
Contributor Author

peterhashair commented Dec 20, 2021

It seems like those 2 queries produce 2 different results, I think the second one is correct, which will load the correct average load time, I guess if column is null, it reset sum back to 0?

Current Query

select sum(time_dom_completion+time_dom_processing+time_network+time_on_load+time_server+time_transfer) as total from log_link_visit_action where server_time like '2021-11-08%';

Convert to Query

select sum(+
IFNULL(`time_dom_completion`, 0) 
+IFNULL(`time_dom_processing`,0)+IFNULL(`time_network`,0)+IFNULL(`time_on_load`,0)+IFNULL(`time_server`,0)+IFNULL(`time_transfer`,0))
as total from log_link_visit_action where server_time like '2021-11-08%';

@peterhashair peterhashair added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Dec 20, 2021
@sgiehl sgiehl added this to the 4.7.0 milestone Dec 20, 2021
plugins/PagePerformance/Archiver.php Show resolved Hide resolved
@@ -6,5 +6,5 @@
<avg_time_dom_processing>0</avg_time_dom_processing>
<avg_time_dom_completion>0</avg_time_dom_completion>
<avg_time_on_load>0</avg_time_on_load>
<avg_page_load_time>0</avg_page_load_time>
<avg_page_load_time>1.67</avg_page_load_time>
Copy link
Member

Choose a reason for hiding this comment

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

This change actually lets me wonder if our calculation for the avg page load time is correct. Why should the page load time be a lot higher then the avg server time in this case?

Actually I think we might maybe also change the calculation of the page_load_hits

$selects[] = sprintf('(SUM(%s)/%s) as page_load_hits', implode(' + ', $hitsColumns), count($hitsColumns));

That actually means that a visit only fully counts if all fields are filled. This works if normally all fields have a value I guess, but as here in the test only one column is filled, causing the avg page load time to be much higher than it should be. I guess we should count a hit as soon as one field is given instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgiehl yes, I was wondering the same, not sure that will work, add a having condition. it actually had PDF test, failed test_TwoVisitors_twoWebsites_differentDays_schedrep_in_pdf_tables_only__ScheduledReports.generateReport_month.original.pdf not sure that's right.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more on that I was actually wondering if we can't simply sum all other average values to get the average page load time. Guess it shouldn't be less accurate then now, but a lot easier to calculate.
The only problem I can see is that it might be inaccurate when the tracking is a bit "abnormal". E.g. you have only one visit where the network time is tracked, but hundreds of visits where the transfer time is tracked. The average network time would be calculated based on that one visit only, while the transfer time out of hundreds. Summing up their averages might then be not accurate. But that might an edge case we can ignore maybe.

Peter Zhang added 4 commits December 20, 2021 23:58
try having condition
update some avg_page
…s_only__ScheduledReports.generateReport_month.original.html

update html test
update html and pdf tests
@@ -6,5 +6,5 @@
<avg_time_dom_processing>0</avg_time_dom_processing>
<avg_time_dom_completion>0</avg_time_dom_completion>
<avg_time_on_load>0</avg_time_on_load>
<avg_page_load_time>0</avg_page_load_time>
<avg_page_load_time>0.13</avg_page_load_time>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be the same as avg_time_server as all other values are 0? Maybe the HAVING doesn't work as expected though?

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2022

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jan 6, 2022
@sgiehl sgiehl merged commit 21c3e78 into 4.x-dev Jan 7, 2022
@sgiehl sgiehl deleted the m-17012-fix-load-time branch January 7, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve average page load time query accuracy (reporting too low in some cases)
2 participants