✨Performance task locally served pages & wg handlers #27283

micajuine-ho · 2020-03-18T07:22:26Z

~~Adding functionality to the performance gulp task to track how long it takes for requests (pageview and ini-load) to fire for pages with amp-analytics tags.~~

This PR is now for supporting locally served pages, along with building a structure for wgs easily add in handlers to measure requests (i.e. wg-ads/wg-analytics measuring delay til the visible ping is sent out). This is one of the steps in adding the performance task into our CI (since our test pages will be locally served pages).

Changes:

Config changes to support handlers for different wgs
- Each handler will test their own defined metrics and include values relevant to those measurements (timeout value, threshold, etc...)
Changes to rewrite-script-tags:
- Parse script tags of locally hosted websites and download the correct files from CDN or local
- Download documents that are not explicitly stated in script tags (auto-amp-lightbox-0.1.js)
Creation of rewrite-analytics-tags:
- Add special identifier to inline config to track visibility pings
- Fetch and merge vendor configs, that would otherwise fail
Changes to measureDocuments:
- Capture all outgoing networks requests
- Apply request handlers, timeout promise, and metric measurer based upon the url being tested
- Redirect requests for documents that are not explicitly stated in script tags (auto-amp-lightbox-0.1.js), to our downloaded version

build-system/tasks/performance/measure-documents.js

zhouyx · 2020-03-18T18:42:49Z

build-system/tasks/performance/README.md

@@ -2,6 +2,6 @@

 Use `gulp performance` to run this test. It measures performance for the current branch compared to the current release. By default, the command first needs to compile minified runtime and components from the current branch by executing the `gulp dist` task. To skip this step, use the `--nobuild` flag, but the task will throw an error if the dist files are missing. Only the runtime and components used by documents from the configured URLs are compiled.

-The `config.json` file contains settings. If you set `headless` to `false`, you will be able to watch the test run in Chrome. The default value for `runs` is 100, which takes ~10 minutes to run, and generates results with ~5% margin of error. Differences larger than this should be investigated. The `urls` array of strings must be set before running the test, and each URL will be tested separately.
+The `config.json` file contains settings. If you set `headless` to `false`, you will be able to watch the test run in Chrome. The default value for `runs` is 100, which takes ~10 minutes to run, and generates results with ~5% margin of error. Differences larger than this should be investigated. The `urls` array of strings must be set before running the test, and each URL will be tested separately. If you wish to test `urls` hosted locally, use `gulp --cdn` to serve the binaries from cdn.


I don't think we should ask all to use gulp serve --compiled when testing locally.
It should be up to user to decide to use gulp serve --compiled or gulp serve --cdn.

Well if you use the --compiled version the task won't work for locally served pages. It only works with --cdn.

Why doesn't it worked with --compiled?

To download the amp scripts (local and master), we look for the CDN_URL (https://cdn.ampproject.org/), and that's only present when serving with --cdn

Hmm I see, that's interesting.

I see. I wonder why don't we use the local server proxy /proxy.

kevinkimball · 2020-03-18T19:15:18Z

build-system/tasks/performance/measure-documents.js

+  const matchArray = interceptedUrl.match(CDN_ANALYTICS_REGEXP);
+  if (matchArray) {


Wondering if this is a good opportunity to setup an extensible pattern here, possibly some mapping of URLs to handlers since it seems likely there would be more cases?

That's a good point! I know the ads team want to collect two requests delay. Not sure if they'll use this specific task. Maybe useful to expand the feature here.
@powerivq Any comment? Thanks

I am thinking of this: each team still implements its own handleRequest to respond the payload it desires, and we make another interceptor to record the timing of the request. Does it make sense? @zhouyx

@kevinkimball good point!

@powerivq I tested out multiple request handlers and it seems to work fine. Seems like a good route to go down in the future.

Looked at the definition of networkidle0. I don't know if this would satisfy ads team's request. ActiveView requires the ad to be visible for at least 50% for 1 second. It's very possible that the task ends before activeView. @kevinkimball @micajuine-ho any workaround?

I believe just adding a setTimeout would do the trick, however I'm not sure if we want to always include it. Maybe we could add a flag (--set-timeout=500) that would set how long to wait for, if any. Thoughts @zhouyx ?

A race between timeout, and the analytics request sounds good to me.
My recommendation is to always include the timeout, but set it to a value that's long enough. People will go with the default setting most of the time, and a timeout will serve as a fallback and reduce flakiness.

BTW this is something I'm testing with, please ignore the console log part : )

let timeout_count = 0; const promise = new Promise(resolve => { r = resolve; setTimeout(() => { timeout_count++; resolve(); }, TIMEOUT); }); page.on('console', message => { if (String(message._text).includes('request delay')) { const delay = Number(message._text.replace('analytics request delay is ', '')); all.push(delay); r(); } }); ..... await promise;

@zhouyx Ok that sounds good to me

Add @estherkim for the best way to customize timeout for different test cases.

rsimha · 2020-03-19T18:39:57Z

Adding @estherkim to review changes to the gulp performance task.

build-system/tasks/performance/README.md

build-system/tasks/performance/measure-documents.js

build-system/tasks/performance/config.json

build-system/tasks/performance/measure-documents.js

zhouyx · 2020-03-27T23:46:53Z

build-system/tasks/performance/config.json

@@ -1,5 +1,15 @@
 {
  "headless": true,
  "runs": 100,
-  "urls": []
+  "urls": [],


what does the top level urls do? should this be?

"noHandlers": { "urls": [], .... }

@zhouyx Yes, that's what it does. Would it be more clear to have noHandlerUrls: [] instead of making it an object, because right now I don't see any other config data that fits in to the noHandlers category (headless and runs applies to all urls)?

I find the declaration a bit confusing because headless and runs applies to all urls, but the top level urls doesn't. The mix and match makes the configuration structure not intuitive imo.

I don't have strong opinion as long as everything agrees with the config setting.

Hmm what do you guys think of the following:

urls: { 'http://normal.test', 'http://analytics-1.test': {handler: analyticsHandler}, 'http://analytics-2.test': {handler: analyticsHandler}, 'http://ads-1.test': {handler: adsHandler}, }

I got the inspo from eslint configs.

@estherkim I like it, but would it have 'http://normal.test': {},?

Yeah hmm. Could be ok?

I would still prefer categorize by handler, because it is more often that one wants to test all handler related urls locally. But this would also work!

@kevinkimball Any preference?

Edit: @estherkim actually after thinking about this for a I while I think the eslint config isn't optimal here. Since each handler could be applied many urls, we would end up having duplicate configs for a lot of urls. Also I don't think that urls with the same handler
would have any different values (aka timeout: 2000 vs timeout:5000). Also, having urls in within the handler definition ensures that each url gets treated the same way with less chance for flake or bias.

build-system/tasks/performance/rewrite-analytics-tags.js

zhouyx · 2020-03-28T00:08:53Z

build-system/tasks/performance/index.js

  await cacheDocuments(urls);
  await compileScripts(urls);
  await rewriteScriptTags(urls);
-  await getMetrics(urls, {headless, runs});
+  await rewriteAnalyticsTags(urls);


I think rewriteScriptTags and rewriteAnalyticsTags can be merged to one to avoid read and write files multiple times.
Another optimization would be to not invoke rewriteAnalyticsTags for urls without the analytics handler defined.
Not blocking, but may be a good optimization to have once we start to run the task on travis. cc @estherkim

Another optimization would be to not invoke rewriteAnalyticsTags for urls without the analytics handler defined.

Sounds good to me.

I think rewriteScriptTags and rewriteAnalyticsTags can be merged to one to avoid read and write files multiple times.

Maybe once the ads team checks in their handler, we can see how we can group the rewrites (assuming that they need to rewrite something).

build-system/tasks/performance/measure-documents.js

micajuine-ho · 2020-04-02T18:42:57Z

Ready for another round of review. Changes since last review:

Config change to include defaultHandler
Rewrite-script changes to find locally served scripts and correctly pull them from the CDN or dist
Refactor measureDocuments to only have 1 request listener and then call upon sub handlers (for analytics, ads, etc...)
Added requestsFailed metric to give us more insight into the task

zhouyx · 2020-04-03T00:33:52Z

build-system/tasks/performance/helpers.js

 const CDN_URL = 'https://cdn.ampproject.org/';
+const AMP_JS_PATH = '/dist/amp.js';
+const LOCAL_PATH_REGEXP = /dist\/(v0\/amp-[A-Za-z\-0-9\.]+).max(.js)/;


Could you explain when would it fetch non compiled script? My understanding is that this should never happen.

Originally, we thought that gulp --cdn to run local pages would work, since that changes the script tags to cdn.ampproject.... and we can parse out the filepath.

However, to use gulp --cdn, we have to run gulp dist --fortesting, which then causes amp-analytics/config.js to not work correctly; it requests file://...v0/analytics-vendors/gtag.json, which is blocked because AMP doesn't allow requests using the file protocol (it uses the file protocol because this.win.location.protocol is file on puppeteer.

So the fix is to use neither gulp --cdn to serve local pages, nor gulp --fortesting, but just parse out the filepaths manually. Also the change wasn't too bad.

Please verify with @estherkim that running gulp dist without --fortesting works on travis.

But this doesn't explain why you need .max.js and also amp.js, they should never appear when you run gulp dist.

But this doesn't explain why you need .max.js and also amp.js, they should never appear when you run gulp dist.

Turns out I was making mistakes when running the gulp dist --forstesting + gulp serve --compiled.

Verified that we will be running gulp dist --forstesting + gulp serve --compiled on Travis.

build-system/tasks/performance/helpers.js

build-system/tasks/performance/measure-documents.js

zhouyx · 2020-04-03T01:05:03Z

build-system/tasks/performance/index.js

@@ -28,11 +29,12 @@ const {printReport} = require('./print-report');
 */
 async function performance() {
  installPackages(__dirname);
-  const {headless, runs, urls} = new loadConfig();
+  const {headless, runs, urls, handlers} = new loadConfig();


This is so so confusing.
We first flat the config object to an array of urls and the raw handlers config.
Then later in measureDocument we iterate the handler config and find the corresponding handler type again?
Could you please optimize the design a bit. Thanks.

Oops sorry for the confusion!

Changed the config API: handlers (Object) => handlers (List)

Changed loadConfing() to do a little more preprocessing to avoid extra work in measureDocuments

Moved analytic specific parameters to the config and removed necessary helpers

Passed in full config to measureDocuments (we were basically doing this before)

Split up setUpRequestHandlers()

build-system/tasks/performance/load-config.js

zhouyx · 2020-04-06T20:26:06Z

build-system/tasks/performance/load-config.js

+    });
+    return mapping;
+  }, {});
+  if (Object.keys(config.urlToHandlers).length < 1) {


So running gulp performance without changing the config will end up with this error? Is that expected?

Yes and yes. Plan is that before we integrate with our CI, we add urls.

zhouyx · 2020-04-06T20:35:17Z

build-system/tasks/performance/config.json

+      "extraUrlParam": {
+        "analytics": "amp-analytics-performance-param"
+      },
+      "analyticsParam": "analytics=amp-analytics-performance-param"


Two general recommendation.

The config should be simple and only contains essential information. analyticsParam should be able to be generated from extraUrlParam in code.

extraUrlParam and analyticsParam don't apply to all handlers. Specifying here may be confusing, a nested handlerConfig may be helpful.

This is fine with only three handler types.

@zhouyx Agree with removing analyticsParam from the config.

extraUrlParam and analyticsParam don't apply to all handlers.

Yep, they already live in the analyticsHandler. Is that good enough?

zhouyx · 2020-04-06T20:37:40Z

build-system/tasks/performance/helpers.js

 const CDN_URL = 'https://cdn.ampproject.org/';
+const AMP_JS_PATH = '/dist/amp.js';
+const LOCAL_PATH_REGEXP = /dist\/(v0\/amp-[A-Za-z\-0-9\.]+).max(.js)/;


Please verify with @estherkim that running gulp dist without --fortesting works on travis.

But this doesn't explain why you need .max.js and also amp.js, they should never appear when you run gulp dist.

zhouyx · 2020-04-06T21:08:33Z

build-system/tasks/performance/measure-documents.js

+ */
+function setUpAddionalHandlers(handlersList, handlerOptions) {
+  // Setting up timing and adding specified handler
+  if (handlerOptions.handlerName === 'analyticsHandler') {


Correct me if this wrong.
All urls within the same handlers config share the same handlerOptions config. (we're not creating new instances of the handlerOptions object within load-config.js file). I'm afraid rewriting the handlerOptions object would affect other instances that fall within the same handler. Have you tried the test with multiple urls.

Yes I have tried it with multiple urls; within measureDocument(), we create handlerOptionsForUrl by creating a shallow copy of the original handlerOptions config. This works fine for storing the startTime and endTime, but would cause some bugs if someone were to mutate objects within the handlerOptions. Instead of shallow copy, should I create a deep copy?

build-system/tasks/performance/measure-documents.js

zhouyx

looks good if this runs correctly, please also get infra team's approval.

build-system/tasks/performance/README.md

zhouyx · 2020-04-15T23:17:38Z

build-system/tasks/performance/helpers.js

@@ -19,13 +19,16 @@ const fs = require('fs');
 const path = require('path');

 const CDN_URL = 'https://cdn.ampproject.org/';
+const V0_PATH = '/dist/v0.js';
+const LOCAL_PATH_REGEXP = /dist\/(v0\/amp-[A-Za-z\-0-9\.]+.js)/;
+const ANALYTICS_VENDORS_PATH = '../../../extensions/amp-analytics/0.1/vendors/';


Let's use the compiled vendor which lives under v0/analytics-vendors/xxx.json

Ok, if we're using that path then we should also add the gulp vendor-configs command as part of this task...

That command will be run in the Travis build, but I think it's good to have it for local testing.

I believe we always run that with gulp build or gulp dist, so it should be fine?

build-system/tasks/performance/measure-documents.js

build-system/tasks/performance/rewrite-analytics-tags.js

micajuine-ho · 2020-04-16T19:35:22Z

@estherkim PTAL

estherkim

LGTM, thanks for working on this!

zhouyx · 2020-04-16T23:12:56Z

build-system/tasks/performance/compile-scripts.js

@@ -28,6 +29,7 @@ async function compileScripts(urls) {
  if (!argv.nobuild) {


--nobuild is only used when you have already built the binary and don't want to build it again.
There's no need to await for vendorConfigs.

micajuine-ho requested review from zhouyx and kevinkimball March 18, 2020 07:22

googlebot added the cla: yes label Mar 18, 2020

amp-owners-bot bot requested a review from ajwhatson March 18, 2020 07:22

init

2ab806c

micajuine-ho requested review from rsimha and removed request for ajwhatson March 18, 2020 07:24

zhouyx reviewed Mar 18, 2020

View reviewed changes

kevinkimball reviewed Mar 18, 2020

View reviewed changes

micajuine-ho force-pushed the analytics_performance branch from 69df27f to 2ab806c Compare March 19, 2020 16:38

suggested changes

f8ec297

rsimha requested review from estherkim and removed request for rsimha March 19, 2020 18:38

estherkim reviewed Mar 19, 2020

View reviewed changes

build-system/tasks/performance/README.md Outdated Show resolved Hide resolved

micajuine-ho force-pushed the analytics_performance branch from f9e03f6 to e15f0ab Compare March 20, 2020 18:25

update README

f963c68

micajuine-ho force-pushed the analytics_performance branch from e15f0ab to f963c68 Compare March 20, 2020 18:28

Micajuine Ho added 3 commits March 23, 2020 17:13

Measure only first request

1900ab9

Merge branch 'master' into analytics_performance

eba630f

set-timeout flag added

df9de6f

estherkim reviewed Mar 26, 2020

View reviewed changes

build-system/tasks/performance/measure-documents.js Outdated Show resolved Hide resolved

move timeout to config

bb35f2e

zhouyx reviewed Mar 26, 2020

View reviewed changes

build-system/tasks/performance/config.json Outdated Show resolved Hide resolved

Request Handlers

a1aba4e

micajuine-ho force-pushed the analytics_performance branch from b4ae9fd to a1aba4e Compare March 27, 2020 21:35

zhouyx suggested changes Mar 28, 2020

View reviewed changes

Micajuine Ho added 2 commits March 30, 2020 09:58

Suggested changes + bug fix

a01e2ce

Refactor into 1 promise based listener

82bb16c

micajuine-ho force-pushed the analytics_performance branch from dace9d0 to 5ac22f0 Compare March 31, 2020 22:46

micajuine-ho force-pushed the analytics_performance branch from 5ac22f0 to e234f8e Compare March 31, 2020 23:07

Rewrite scripts for local, better measurements

83bc02c

zhouyx reviewed Apr 3, 2020

View reviewed changes

Refacor and simplify

19fee99

zhouyx reviewed Apr 6, 2020

View reviewed changes

estherkim mentioned this pull request Apr 7, 2020

Serve pages locally instead of using a file cache for gulp performance #27628

Closed

New config

33d787b

micajuine-ho changed the title ~~✨Analytics request performance measurer~~ ✨Performance task locally served pages & wg handlers Apr 10, 2020

micajuine-ho force-pushed the analytics_performance branch from 40fa4b7 to 157e12e Compare April 10, 2020 23:14

zhouyx reviewed Apr 15, 2020

View reviewed changes

micajuine-ho force-pushed the analytics_performance branch from 157e12e to 57b7840 Compare April 16, 2020 19:33

estherkim approved these changes Apr 16, 2020

View reviewed changes

zhouyx reviewed Apr 16, 2020

View reviewed changes

Locally server urls

3b84fde

micajuine-ho force-pushed the analytics_performance branch from 57b7840 to 3b84fde Compare April 17, 2020 19:49

micajuine-ho closed this Apr 20, 2020

micajuine-ho reopened this Apr 20, 2020

micajuine-ho removed the cla: yes label Apr 21, 2020

googlebot added the cla: yes label Apr 21, 2020

micajuine-ho removed the cla: yes label Apr 21, 2020

googlebot added the cla: yes label Apr 21, 2020

zhouyx approved these changes Apr 21, 2020

View reviewed changes

micajuine-ho merged commit 1098976 into ampproject:master Apr 21, 2020

ampprojectbot added the PR use: In Beta / Experimental label Apr 28, 2020

micajuine-ho added this to In progress in Performance Improvements via automation Apr 28, 2020

micajuine-ho moved this from In progress to Done in Performance Improvements Apr 28, 2020

ampprojectbot added PR use: In Stable PR use: In LTS labels May 5, 2020

		const matchArray = interceptedUrl.match(CDN_ANALYTICS_REGEXP);
		if (matchArray) {

		@@ -28,6 +29,7 @@ async function compileScripts(urls) {
		if (!argv.nobuild) {

✨Performance task locally served pages & wg handlers #27283

✨Performance task locally served pages & wg handlers #27283

Conversation

micajuine-ho commented Mar 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha commented Mar 19, 2020

Choose a reason for hiding this comment

micajuine-ho Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho Apr 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Apr 16, 2020

estherkim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micajuine-ho commented Mar 18, 2020 •

edited

micajuine-ho Mar 30, 2020 •

edited

micajuine-ho Mar 30, 2020 •

edited

micajuine-ho Apr 10, 2020 •

edited