Skip to content

Add performance.mark for next/third-parties for feature measurement#57439

Merged
kodiakhq[bot] merged 12 commits intovercel:canaryfrom
janicklas-ralph:perf-mark
Nov 15, 2023
Merged

Add performance.mark for next/third-parties for feature measurement#57439
kodiakhq[bot] merged 12 commits intovercel:canaryfrom
janicklas-ralph:perf-mark

Conversation

@janicklas-ralph
Copy link
Copy Markdown
Contributor

@janicklas-ralph janicklas-ralph commented Oct 25, 2023

What?

Adding a performance.mark for next/third-parties for feature measurement

Why?

We’re working with the Chrome Speed Metrics team to develop an API to better identify specific features used by frameworks and/or CMS platforms. Instead of a entirely new API surface, we've agreed to use performance.mark as the signal along with mark_use_counter as a signature.

In the past, we've used HTML attributes for Next which have been helpful, but this will improve the accuracy of our feature detection. In this instance, this will better help us analyze and compare sites that use @next/third-parties versus those that don't, and to see if our efforts are actually improving performance.

It's worth noting that this metric will have a public arm, but we're starting to incorporate its usage now to gather early data. performance.mark calls are indeed low-overhead and shouldn't affect loading/rendering performance whatsoever.

}: ScriptEmbed) {
useEffect(() => {
// Useful for feature detection and measurement
performance.mark('next-third-parties', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have to use the special mark_use_counter name so it's picked up by Chrome tooling. The unique name should be in the details field.

performance.mark('mark_use_counter',
    {'detail': {
        'feature': 'next-third-parties'
        }
    }
);


useEffect(() => {
// Useful for feature detection and measurement
performance.mark('next-third-parties', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here - though I like that we're doing a separate name for GTM. WIll help us track diff components and compare

@ijjk
Copy link
Copy Markdown
Member

ijjk commented Nov 2, 2023

Tests Passed

performance.mark('mark_use_counter', {
detail: {
feature: 'next-third-parties',
type: dataNtpc,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of using 'type', can we append the dataNtpc to the main feature name if it exists? e.g. next-third-parties-${dataNtpc}

@janicklas-ralph janicklas-ralph marked this pull request as ready for review November 7, 2023 19:26
Copy link
Copy Markdown
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@ijjk
Copy link
Copy Markdown
Member

ijjk commented Nov 7, 2023

Stats from current PR

Default Build
General
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
buildDuration 20.7s 19.9s N/A
buildDurationCached 12s 13.2s ⚠️ +1.2s
nodeModulesSize 199 MB 199 MB
nextStartRea..uration (ms) 878ms 748ms N/A
Client Bundles (main, webpack)
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
199-HASH.js gzip 29.2 kB 29.2 kB N/A
3f784ff6-HASH.js gzip 53.3 kB 53.3 kB N/A
494.HASH.js gzip 180 B 181 B N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 241 B 240 B N/A
main-HASH.js gzip 31.7 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB
Overall change 46.9 kB 46.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 504 B 506 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB
edge-ssr-HASH.js gzip 253 B 255 B N/A
head-HASH.js gzip 348 B 347 B N/A
hooks-HASH.js gzip 369 B 368 B N/A
image-HASH.js gzip 4.3 kB 4.3 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.65 kB 2.65 kB N/A
routerDirect..HASH.js gzip 311 B 311 B
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 307 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.17 kB 3.17 kB
Client Build Manifests
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
_buildManifest.js gzip 486 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
index.html gzip 528 B 526 B N/A
link.html gzip 541 B 541 B
withRouter.html gzip 524 B 522 B N/A
Overall change 541 B 541 B
Edge SSR bundle Size
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
edge-ssr.js gzip 92.6 kB 92.6 kB N/A
page.js gzip 145 kB 145 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
middleware-b..fest.js gzip 626 B 625 B N/A
middleware-r..fest.js gzip 150 B 151 B N/A
middleware.js gzip 24.8 kB 24.8 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Next Runtimes
vercel/next.js canary janicklas-ralph/next.js perf-mark Change
app-page-exp...dev.js gzip 167 kB 167 kB
app-page-exp..prod.js gzip 93.2 kB 93.2 kB
app-page-tur..prod.js gzip 93.9 kB 93.9 kB
app-page-tur..prod.js gzip 88.5 kB 88.5 kB
app-page.run...dev.js gzip 137 kB 137 kB
app-page.run..prod.js gzip 87.9 kB 87.9 kB
app-route-ex...dev.js gzip 23.8 kB 23.8 kB
app-route-ex..prod.js gzip 16.4 kB 16.4 kB
app-route-tu..prod.js gzip 16.4 kB 16.4 kB
app-route-tu..prod.js gzip 16 kB 16 kB
app-route.ru...dev.js gzip 23.2 kB 23.2 kB
app-route.ru..prod.js gzip 16 kB 16 kB
pages-api-tu..prod.js gzip 9.37 kB 9.37 kB
pages-api.ru...dev.js gzip 9.64 kB 9.64 kB
pages-api.ru..prod.js gzip 9.37 kB 9.37 kB
pages-turbo...prod.js gzip 21.8 kB 21.8 kB
pages.runtim...dev.js gzip 22.5 kB 22.5 kB
pages.runtim..prod.js gzip 21.8 kB 21.8 kB
server.runti..prod.js gzip 49 kB 49 kB
Overall change 923 kB 923 kB
Commit: fa16c2f

Copy link
Copy Markdown
Contributor

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Copy Markdown
Contributor

@gnoff gnoff 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 write up a description that explains how this provides an ability to measure adoption and perf wins? I'm used to using these in dev to debug perf but it seems this would be live in Prod too. I assume this is meant to be low-overhead but would be nice to understand what we're getting for adding this to the production path.

@housseindjirdeh
Copy link
Copy Markdown
Contributor

housseindjirdeh commented Nov 10, 2023

Can you write up a description that explains how this provides an ability to measure adoption and perf wins? I'm used to using these in dev to debug perf but it seems this would be live in Prod too. I assume this is meant to be low-overhead but would be nice to understand what we're getting for adding this to the production path.

We’re working with the Chrome Speed Metrics team to develop an API to better identify specific features used by frameworks and/or CMS platforms. Instead of a entirely new API surface, we've agreed to use performance.mark as the signal along with mark_use_counter as a signature.

In the past, we've used HTML attributes for Next which have been helpful, but this will improve the accuracy of our feature detection. In this instance, this will better help us analyze and compare sites that use @next/third-parties versus those that don't, and to see if our efforts are actually improving performance.

It's worth noting that this metric will have a public arm, but we're starting to incorporate its usage now to gather early data. performance.mark calls are indeed low-overhead and shouldn't affect loading/rendering performance whatsoever.

// Useful for feature detection and measurement
performance.mark('mark_use_counter', {
detail: {
feature: `next-third-parties-${dataNtpc}`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if dataNtpc is not provided the feature will be next-third-parties-. Should we provide a more descriptive placeholder value? Also if the implementor doesn't provide a dataNtpc value should we even do the attribution? If it's actually required then maybe we should warn if Dev if missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I don't think we need to fire attribution if we don't receive dataNtpc, since it won't be helpful

const gtmPreview = preview ? `&gtm_preview=${preview}&gtm_cookies_win=x` : ''

useEffect(() => {
// Useful for feature detection and measurement
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Useful for feature detection and measurement
// performance.mark is being used as a feature use signal. While it is traditionally used for performance
// benchmarking it is low overhead and thus considered safe to use in production and it is a widely available
// existing API.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe update both comments. I interpretted feature detection as being related to whether the enviornment provides a feature and measurement as this being related to preformance measurement given the APIs used. Clarifying should help us keep track of why this code is in here when we rediscover this later :)

@gnoff
Copy link
Copy Markdown
Contributor

gnoff commented Nov 13, 2023

@housseindjirdeh @janicklas-ralph ready to approve but for the question on dataNtpc. LMK what ya'll think there

@janicklas-ralph
Copy link
Copy Markdown
Contributor Author

I've made the changes for dataNtpc. LMK if this looks good

// performance.mark is being used as a feature use signal. While it is traditionally used for performance
// benchmarking it is low overhead and thus considered safe to use in production and it is a widely available
// existing API.
performance.mark('mark_use_counter', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Working group changed their mind on naming:

Suggested change
performance.mark('mark_use_counter', {
performance.mark('mark_feature_usage', {

// benchmarking it is low overhead and thus considered safe to use in production and it is a widely available
// existing API.

performance.mark('mark_use_counter', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
performance.mark('mark_use_counter', {
performance.mark('mark_feature_usage', {

Copy link
Copy Markdown
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

A PR description is missing

import React from 'react'
'use client'

import React, { useEffect } from 'react'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
import React, { useEffect } from 'react'
import type { ReactElement } from 'react'
import { useEffect } from 'react'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably better not to pull in the entire entrypoint as a good practice. not a blocker though

@gnoff gnoff dismissed timneutkens’s stale review November 15, 2023 17:42

PR description was addded

@kodiakhq kodiakhq bot merged commit 3ec0551 into vercel:canary Nov 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants