LCP and FID variable values differ greatly from chrome extension values

hannahmahon commented 4 years ago

What's the issue?

The substitute variables for LCP and FID in my amp-analytics script don't appear to be accurate. They are both long ints that look like 1593003422844, and don't match any of the numbers output by my web vitals chrome extension (not simply a matter of moving a decimal point)

How do we reproduce the issue?

My code looks like this:

...
<amp-analytics type="googleanalytics" id="googleanalytics1">
<script type="application/json">
{
   "vars":{
      "account":"UA-XXXXXXX-1"
   },
   "extraUrlParams":{
      "cd53":"Outlook;History"
   },
   "triggers":{
      "webVitals": {
        "on":"visible",
         "request":"event",
         "extraUrlParams":{
            "lcp":"${largestContentfulPaint}",
            "fid":"${firstInputDelay}",
            "cls":"${cumulativeLayoutShift}"
         }
      }
   }
}
</script>
</amp-analytics>

CLS is working beautifully, but when I look at the network call for lcp & fid, they look more like ids than the true vital values. I can't get a jsbin to fire any of the events so I don't have a sandbox for you, but hoping you can still help

What browsers are affected?

At least chrome

Which AMP version is affected?

Version 2006112352001

Ichigo3d commented 4 years ago

cc @zhouyx

zhouyx commented 4 years ago

Thanks for reporting. Taking a look

zhouyx commented 4 years ago

Found two issues.

There's a bug on how we offset the pre-rendering time
largest-contentful-paint is resolved only after the document turned invisible. Will follow up with PRs to fix them.

Edited: 2 is expected. The largest element can change, it makes sense to report after the document is inactive/invisible.

hannahmahon commented 4 years ago

Awesome, thanks for the fast turnaround!!

zhouyx commented 4 years ago

@hannahmahon The issue has been fixed by #29039, it should be shipped in production within two weeks.

(We decided not to request cherry-pick to patch the fix because this is a recently shipped feature without wide adoption yet. And there's no regression, the issue existed when we shipped the feature. ) Please let us know if you feel otherwise.

Thank you.

hannahmahon commented 4 years ago

Thanks for letting me know, sounds good!

hannahmahon commented 4 years ago

Hi @zhouyx , has this been deployed?

zhouyx commented 4 years ago

Hi @hannahmahon, not yet. You can track the deployment in #29171

ampproject / amphtml