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
Conversation
update mysql query that return right load time
remove specialist code
update archiver sum query
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 Queryselect 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 Queryselect 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%'; |
update tests xml
@@ -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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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> |
There was a problem hiding this comment.
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?
Co-authored-by: Stefan Giehl <stefan@matomo.org>
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
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