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

Enable hero element timing by default #1261

Merged
merged 5 commits into from Mar 22, 2019

Conversation

Projects
None yet
2 participants
@wildlyinaccurate
Copy link
Contributor

commented Mar 20, 2019

SpeedCurve have been capturing hero element timing with wptagent for a while now. I recently performed some analysis to figure out whether the times reported by wptagent were close enough to the times reported by our existing implementation. For around 95% of our tests, the times varied by <10%. I dug into a load of the remaining 5% of tests and we determined that most of the differences were for acceptable reasons - mostly caused by fuzziness, since our implementation works with JPEG frames.

Anyways, we're pretty confident that the implementation in wptagent is good enough that everyone can use it reliably, so I'd like to turn it on by default in webpagetest.org.

There will be an accompanying PR in wptagent.

wildlyinaccurate added some commits Mar 20, 2019

@@ -210,7 +210,7 @@ function DealWithMagicQuotes(&$arr) {
$test['lighthouse'] = $req_lighthouse;
$test['lighthouseTrace'] = isset($_REQUEST['lighthouseTrace']) && $_REQUEST['lighthouseTrace'] ? 1 : 0;
$test['lighthouseThrottle'] = isset($_REQUEST['lighthouseThrottle']) && $_REQUEST['lighthouseThrottle'] ? 1 : GetSetting('lighthouseThrottle', 0);
$test['heroElementTimes'] = isset($_REQUEST['heroElementTimes']) && $_REQUEST['heroElementTimes'] ? 1 : 0;
$test['heroElementTimes'] = isset($_REQUEST['heroElementTimes']) ? (int) $_REQUEST['heroElementTimes'] : 1;

This comment has been minimized.

Copy link
@pmeenan

pmeenan Mar 21, 2019

Contributor

Can you change this instead to operate like the "lighthouseThrottle" line above and pull the default value from the settings file? Also add a sample to settings.ini.sample

@@ -2235,8 +2235,8 @@ function CreateTest(&$test, $url, $batch = 0, $batch_locations = 0)
AddIniLine($testFile, 'lighthouseTrace', '1');
if( isset($test['lighthouseThrottle']) && $test['lighthouseThrottle'] )
AddIniLine($testFile, 'lighthouseThrottle', '1');
if( isset($test['heroElementTimes']) && $test['heroElementTimes'] )
AddIniLine($testFile, 'heroElementTimes', '1');
if( isset($test['heroElementTimes']) )

This comment has been minimized.

Copy link
@pmeenan

pmeenan Mar 21, 2019

Contributor

No need to change this if the agent continues to have it disabled by default and have the system default controlled by settings.ini

wildlyinaccurate added some commits Mar 22, 2019

@wildlyinaccurate

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Thanks for the feedback, Pat! I think these latest changes are what you're looking for.

@pmeenan

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Perfect, thanks. Merging now. I'm about to head out the door to Toronto for the Fitc conference so I'll flip the flag on the public instance when I get back on Sunday.

@pmeenan pmeenan merged commit dcf86bb into WPO-Foundation:master Mar 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.