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

✨ Performance Measurement Chrome Extension #26333

Merged
merged 13 commits into from
Jan 24, 2020

Conversation

wassgha
Copy link
Contributor

@wassgha wassgha commented Jan 14, 2020

Implements a chrome extension that helps with performance measurement. Currently supports the following metrics:

  • Time to Visible
  • First Paint
  • First Contentful Paint
  • Largest Contentful Paint
  • Maximum First Input Delay
  • Time to Interactive
  • Cumulative Layout Shift on Load
  • Instantaneous Cumulative Layout Shift

Overlays the measurements on the page as follows:
Screen Shot 2020-01-14 at 10 59 19 AM

@wassgha wassgha changed the title ✨ Performance measurement extension ✨ Performance Measurement Chrome Extension Jan 14, 2020
@wassgha wassgha marked this pull request as ready for review January 14, 2020 18:57
@kristoferbaxter
Copy link
Contributor

Can we limit the items shown to FID, LCP, and CLS?

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an OWNERS file?

@rsimha –  Do you think wg-performance or wg-infrastructure should maintain this going forward?

@rsimha
Copy link
Contributor

rsimha commented Jan 15, 2020

@rsimha –  Do you think wg-performance or wg-infrastructure should maintain this going forward?

Good question, @kristoferbaxter.

I'm assuming that:

  • This chrome extension is user-facing and isn't likely to be in the critical path for AMP CI / releases
  • Its purpose is to track metrics that are defined and owned by the Performance working group

Based on this, I think it should be owned and maintained by @ampproject/wg-performance. If in future, it becomes feasible for this tool to be used as part of releases / CI, someone from @ampproject/wg-infra can certainly help with integrating this into our current workflow.

WDYT?

@kristoferbaxter
Copy link
Contributor

@rsimha – I agree. Makes sense to me!

* limitations under the License.
*/

function renderMeasurement(container, label, count) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: innerHTML, since we control the label and count?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I return the countSpan here so if we use innerHTML there will be an extra querySelector so pretty much the same amount of work... Leaving it for now.

This comment was marked as resolved.

testing/amp-performance-extension/measure.js Outdated Show resolved Hide resolved
testing/amp-performance-extension/measure.js Outdated Show resolved Hide resolved
testing/amp-performance-extension/measure.js Outdated Show resolved Hide resolved
Copy link
Member

@mrjoro mrjoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming the others approve. :)

* limitations under the License.
*/

function renderMeasurement(container, label, count) {

This comment was marked as resolved.

@wassgha wassgha merged commit 695fd66 into ampproject:master Jan 24, 2020
@wassgha wassgha deleted the performance branch January 24, 2020 04:49
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Jan 27, 2020
* master: (62 commits)
  📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491)
  Revert 'Move mutator implementations out to a standalone service' (ampproject#26479)
  Fix syntax error (ampproject#26481)
  Add pespective back. (ampproject#26486)
  More user friendly errors in layout.js (ampproject#26448)
  ✨ Start logging AMP URL on SwG Pages (ampproject#26480)
  Fix border around desktop amp-story-pages. (ampproject#26449)
  Fix Story tests. (ampproject#26464)
  ✨ Performance Measurement Chrome Extension (ampproject#26333)
  amp-consent restrict iframe fullScreen if no focus  (ampproject#26461)
  Add performance benchmark task (ampproject#26026)
  ♻️ amp-script: emit warning if zero height and width. (ampproject#26444)
  ✨ Launch minimal-wrapper native CEv1 (ampproject#26360)
  ♻️ Lint: include externs (round 2) (ampproject#26446)
  amp-script: Create 'fill content' container for responsive/fluid (ampproject#26400)
  amp-consent remove cmp iframe focus (ampproject#26437)
  Disable macro-after-long-task in inabox. (ampproject#26459)
  Launch layoutbox-invalidate-on-scroll (ampproject#26430)
  Add amp-ad support for ByPlay (ampproject#25663)
  🏗 Add specific RTV opt-in to experiments.html (ampproject#26434)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants