Re-enable the observer custom metric

[rviscomi]

We had to disable the observer custom metric because it was interfering with the page rendering in some cases.

Investigate how that interference happened and come up with a workaround so that we can re-enable this feature.

[kevinfarrugia]

Related to HTTPArchive/data-pipeline#84

[kevinfarrugia]

There is issue related to "Object.prototype.*" although possibly others too.

Here is a test with an observer on Object.prototype.* and an equivalent test with the following observers:

  const OBSERVERS = [
    "navigator.__proto__.*",
    "performance.__proto__.*",
    "performance.timing.__proto__.*",
    "Array.prototype.*",
    "String.prototype.*",
//    "Object.prototype.*",
    "CSSStyleDeclaration.prototype.*",
    "document.featurePolicy",
    "document.write",
    "queueMicrotask",
    "requestIdleCallback",
    "scheduler.postTask",
  ];

Baseline for comparison. The baseline occassionally has more requests, but I think this depends on whether the top ad is shown, but it isn't shown on all tests.

@rviscomi is it possible to test this at scale instead of relying on manual testing?

[rviscomi]

Thanks for looking into this @kevinfarrugia!

The only way to test this at scale is to enable the custom metric in the crawl, which I'm not totally comfortable doing yet, even with this change. Disabling the Object observer seems to fix the CNN case but I worry about the effects of the other observers.

Here's a challenge for you: is there an idempotent way to observe Object such that it doesn't break CNN? I'd love to find a way to safely observe anything without worrying about side effects on the page.

[kevinfarrugia]

OK, so I think the root cause of the issue is that we are not defining a setter for the observed properties. If you inject the observer.js JavaScript to a page, you will see this error:

Uncaught TypeError: Cannot set property constructor of [object Object] which has only a getter

This can be fixed by adding a set to the Object.defineProperty. I created PR #57 which adds a setter (but doesn't observer how many times the setter is called). This seems to fix the issue.

I have some concerns though:

• Are we modifying the default JavaScript behaviour too much? Is there a measurable impact of the snippet?
• Should we also observe the set property?
• How do we find out if there are other issues which we have not yet discovered?

Before: https://webpagetest.httparchive.org/result/221103_JD_5/
After: https://webpagetest.httparchive.org/result/221103_GY_6/

[kevinfarrugia]

I am also unsure about the call_stacks and this bit of code:

        if (PROPERTIES_TO_TRACE.has(pathname)) {
          // Construct a stack trace.
          let stack;
          try {
            throw new Error();
          } catch (e) {
            stack = e.stack;
          }
          let stackCounter = httparchive_observers.call_stacks[pathname];
          if (!stackCounter[stack]) {
            stackCounter[stack] = 0;
          }
          stackCounter[stack]++;
        }

What is it intended to measure? Currently I am getting a length stack trace but I cannot understand the valule:

{"call_stacks":{"navigator.vendor":{"Error\n at Navigator.get (<anonymous>:150:19)\n at Object.isSafari (https:\/\/secure.quantserve.com\/quant.js:2:1658)\n at https:\/\/secure.quantserve.com\/quant.js:2:22780\n at Object.then (
// ...

Test run without above snippet: https://webpagetest.httparchive.org/result/221103_G6_8/1/details/

[rviscomi]

OK, so I think the root cause of the issue is that we are not defining a setter for the observed properties.

That sounds promising!

Are we modifying the default JavaScript behaviour too much? Is there a measurable impact of the snippet?

We could try measuring this with A/B tests in WPT with and without the custom metric enabled. Maybe TBT is a good proxy metric. I suspect it's fine, but if not we could minimize the number of observed properties and avoid the .* expansions.

Should we also observe the set property?

That could be interesting but I can't think of a use for it right now. So let's stick with get.

How do we find out if there are other issues which we have not yet discovered?

That's a good question. We could re-import the problematic June 1, 2022 crawl into BigQuery and write a query to find failure cases. Then we can test a sample of those failed pages with the old and new custom metric code to verify that we're getting expected results.

Alternatively, if we know that pages failed because they were attempting to override the getter/setter methods, we might be able to regex search the scripts in the latest dataset to find potentially problematic pages and run the old and new custom metric code against that sample.

I am also unsure about the call_stacks and this bit of code:
What is it intended to measure? Currently I am getting a length stack trace but I cannot understand the valule:

The call stack tells us which part of the code accessed the observed property. So we can distinguish between 1P and 3P or inspect individual lines of code to better understand what it's trying to do.