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

fix: Roll back to previous FirstInteraction implementation #1359

Merged
merged 2 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/common/vitals/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,9 @@
export const VITAL_NAMES = {
FIRST_PAINT: 'fp',
FIRST_CONTENTFUL_PAINT: 'fcp',
FIRST_INTERACTION: 'fi',
FIRST_INPUT_DELAY: 'fi',
LARGEST_CONTENTFUL_PAINT: 'lcp',
CUMULATIVE_LAYOUT_SHIFT: 'cls',
INTERACTION_TO_NEXT_PAINT: 'inp',
TIME_TO_FIRST_BYTE: 'ttfb'
}

export const PERFORMANCE_ENTRY_TYPE = {
FIRST_INPUT: 'first-input'
}
28 changes: 28 additions & 0 deletions src/common/vitals/first-input-delay.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright 2020-2025 New Relic, Inc. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import { onFID } from 'web-vitals/attribution'
import { VitalMetric } from './vital-metric'
import { VITAL_NAMES } from './constants'
import { initiallyHidden, isBrowserScope } from '../constants/runtime'

export const firstInputDelay = new VitalMetric(VITAL_NAMES.FIRST_INPUT_DELAY)

if (isBrowserScope) {
onFID(({ value, attribution }) => {
if (initiallyHidden || firstInputDelay.isValid) return
const attrs = {
type: attribution.eventType,
fid: Math.round(value),
eventTarget: attribution.eventTarget,
loadState: attribution.loadState
}

// CWV will only report one (THE) first-input entry to us; fid isn't reported if there are no user interactions occurs before the *first* page hiding.
firstInputDelay.update({
value: attribution.eventTime,
attrs
})
})
}
38 changes: 0 additions & 38 deletions src/common/vitals/first-interaction.js

This file was deleted.

4 changes: 2 additions & 2 deletions src/features/page_view_timing/aggregate/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import { FEATURE_NAMES } from '../../../loaders/features/features'
import { AggregateBase } from '../../utils/aggregate-base'
import { cumulativeLayoutShift } from '../../../common/vitals/cumulative-layout-shift'
import { firstContentfulPaint } from '../../../common/vitals/first-contentful-paint'
import { firstInputDelay } from '../../../common/vitals/first-input-delay'
import { firstPaint } from '../../../common/vitals/first-paint'
import { firstInteraction } from '../../../common/vitals/first-interaction'
import { interactionToNextPaint } from '../../../common/vitals/interaction-to-next-paint'
import { largestContentfulPaint } from '../../../common/vitals/largest-contentful-paint'
import { timeToFirstByte } from '../../../common/vitals/time-to-first-byte'
Expand All @@ -37,8 +37,8 @@ export class Aggregate extends AggregateBase {
this.waitForFlags(([])).then(() => {
firstPaint.subscribe(this.#handleVitalMetric)
firstContentfulPaint.subscribe(this.#handleVitalMetric)
firstInputDelay.subscribe(this.#handleVitalMetric)
largestContentfulPaint.subscribe(this.#handleVitalMetric)
firstInteraction.subscribe(this.#handleVitalMetric)
interactionToNextPaint.subscribe(this.#handleVitalMetric)
timeToFirstByte.subscribe(({ attrs }) => {
this.addTiming('load', Math.round(attrs.navigationEntry.loadEventEnd))
Expand Down
5 changes: 5 additions & 0 deletions src/features/session_trace/aggregate/trace/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ export class TraceStorage {

processPVT (name, value, attrs) {
this.storeTiming({ [name]: value })
if (hasFID(name, attrs)) this.storeEvent({ type: 'fid', target: 'document' }, 'document', value, value + attrs.fid)

function hasFID (name, attrs) {
return name === 'fi' && !!attrs && typeof attrs.fid === 'number'
}
}

storeTiming (timingEntry, isAbsoluteTimestamp = false) {
Expand Down
11 changes: 11 additions & 0 deletions tests/components/page_view_timing/aggregate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ jest.mock('web-vitals/attribution', () => ({
value: 1,
attribution: {}
})),
onFID: jest.fn(cb => cb({
value: 1234,
attribution: { eventTime: 5, eventType: 'pointerdown' }
})),
onINP: jest.fn((cb) => cb({
value: 8,
attribution: {}
Expand Down Expand Up @@ -81,6 +85,13 @@ test('LCP event with CLS attribute', () => {
}
})

test('sends expected FI attributes when available', () => {
expect(timingsAggregate.events.get()[0].data.length).toBeGreaterThanOrEqual(1)
const fiPayload = timingsAggregate.events.get()[0].data.find(x => x.name === 'fi')
expect(fiPayload.value).toEqual(5)
expect(fiPayload.attrs).toEqual(expect.objectContaining({ type: 'pointerdown', fid: 1234, cls: 0.1119, ...expectedNetworkInfo }))
})

test('sends CLS node with right val on vis change', () => {
let clsNode = timingsAggregate.events.get()[0].data.find(tn => tn.name === VITAL_NAMES.CUMULATIVE_LAYOUT_SHIFT)
expect(clsNode).toBeUndefined()
Expand Down
11 changes: 11 additions & 0 deletions tests/components/page_view_timing/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ jest.mock('web-vitals/attribution', () => ({
value: 1,
attribution: {}
})),
onFID: jest.fn(cb => cb({
value: 1234,
attribution: { eventTime: 5, eventType: 'pointerdown' }
})),
onINP: jest.fn((cb) => cb({
value: 8,
attribution: {}
Expand Down Expand Up @@ -83,6 +87,13 @@ describe('pvt aggregate tests', () => {
}
})

test('sends expected FI attributes when available', () => {
expect(pvtAgg.events.get()[0].data.length).toBeTruthy()
const fiPayload = pvtAgg.events.get()[0].data.find(x => x.name === 'fi')
expect(fiPayload.value).toEqual(5)
expect(fiPayload.attrs).toEqual(expect.objectContaining({ type: 'pointerdown', fid: 1234, cls: 0.1119, ...expectedNetworkInfo }))
})

test('sends CLS node with right val on vis change', () => {
let clsNode = pvtAgg.events.get()[0].data.find(tn => tn.name === VITAL_NAMES.CUMULATIVE_LAYOUT_SHIFT)
expect(clsNode).toBeUndefined()
Expand Down
7 changes: 6 additions & 1 deletion tests/components/session_trace/aggregate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ test('creates right nodes', async () => {
document.dispatchEvent(new CustomEvent('DOMContentLoaded')) // simulate natural browser event
window.dispatchEvent(new CustomEvent('load')) // load is actually ignored by Trace as it should be passed by the PVT feature, so it should not be in payload
sessionTraceAggregate.events.storeXhrAgg('xhr', '[200,null,null]', { method: 'GET', status: 200 }, { rxSize: 770, duration: 99, cbTime: 0, time: 217 }) // fake ajax data
sessionTraceAggregate.events.processPVT('fi', 30) // fake pvt data
sessionTraceAggregate.events.processPVT('fi', 30, { fid: 8 }) // fake pvt data

const payload = sessionTraceAggregate.makeHarvestPayload()[0].payload
let res = payload.body
Expand Down Expand Up @@ -58,6 +58,11 @@ test('creates right nodes', async () => {
expect(pvt.o).toEqual('document')
expect(pvt.s).toEqual(pvt.e) // that FI has no duration
expect(pvt.t).toEqual('timing')
pvt = res.filter(node => node.n === 'fid')[0]
expect(pvt.o).toEqual('document')
expect(pvt.s).toEqual(30) // that FID has a duration relative to FI'
expect(pvt.e).toEqual(30 + 8)
expect(pvt.t).toEqual('event')

let unknown = res.filter(n => n.o === 'unknown')
expect(unknown.length).toEqual(0) // no events with unknown origin
Expand Down
12 changes: 8 additions & 4 deletions tests/specs/pvt/timings.e2e.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { supportsCumulativeLayoutShift, supportsFirstPaint, supportsInteractionToNextPaint, supportsLargestContentfulPaint, supportsPerformanceEventTiming } from '../../../tools/browser-matcher/common-matchers.mjs'
import { supportsCumulativeLayoutShift, supportsFirstInputDelay, supportsFirstPaint, supportsInteractionToNextPaint, supportsLargestContentfulPaint } from '../../../tools/browser-matcher/common-matchers.mjs'
import { testTimingEventsRequest } from '../../../tools/testing-server/utils/expect-tests'

const isClickInteractionType = type => type === 'pointerdown' || type === 'mousedown' || type === 'click'
const loadersToTest = ['rum', 'spa']

describe('pvt timings tests', () => {
Expand Down Expand Up @@ -71,7 +72,7 @@ describe('pvt timings tests', () => {

describe('interaction related timings', () => {
loadersToTest.forEach(loader => {
it(`FI, INP & LCP for ${loader} agent`, async () => {
it(`FI, FID, INP & LCP for ${loader} agent`, async () => {
const start = Date.now()
await browser.url(
await browser.testHandle.assetURL('basic-click-tracking.html', { loader })
Expand All @@ -84,17 +85,20 @@ describe('pvt timings tests', () => {
.then(async () => browser.url(await browser.testHandle.assetURL('/')))
])

if (browserMatch(supportsPerformanceEventTiming)) {
if (browserMatch(supportsFirstInputDelay)) {
// FID is replaced by subscribing to 'first-input'
const fi = timingsHarvests.find(harvest => harvest.request.body.find(t => t.name === 'fi'))
?.request.body.find(timing => timing.name === 'fi')
expect(fi.value).toBeGreaterThanOrEqual(0)
expect(fi.value).toBeLessThan(Date.now() - start)

const isClickInteractionType = type => type === 'pointerdown' || type === 'mousedown' || type === 'click'
const fiType = fi.attributes.find(attr => attr.key === 'type')
expect(isClickInteractionType(fiType.value)).toEqual(true)
expect(fiType.type).toEqual('stringAttribute')

const fid = fi.attributes.find(attr => attr.key === 'fid')
expect(fid.value).toBeGreaterThanOrEqual(0)
expect(fid.type).toEqual('doubleAttribute')
}

if (browserMatch(supportsLargestContentfulPaint)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,43 +1,34 @@
import { PERFORMANCE_ENTRY_TYPE } from '../../../../src/common/vitals/constants'

beforeEach(() => {
jest.resetModules()
jest.resetAllMocks()
jest.clearAllMocks()

const mockPerformanceObserver = jest.fn(cb => ({
// Note: this is an imperfect mock, as observer.disconnect() is not functional
observe: () => {
const callCb = () => {
cb({
getEntries: () => [{
name: PERFORMANCE_ENTRY_TYPE.FIRST_INPUT,
startTime: 1
}]
})
setTimeout(callCb, 250)
}
setTimeout(callCb, 250)
},
disconnect: jest.fn()
}))
global.PerformanceObserver = mockPerformanceObserver
global.PerformanceObserver.supportedEntryTypes = [PERFORMANCE_ENTRY_TYPE.FIRST_INPUT]
})

const getFreshImport = async (codeToRun) => {
const { firstInteraction } = await import('../../../../src/common/vitals/first-interaction')
codeToRun(firstInteraction)
const fidAttribution = {
eventTarget: 'html>body',
eventType: 'pointerdown',
eventTime: 1,
loadState: 'loading'
}
let triggeronFIDCallback
const getFreshFIDImport = async (codeToRun) => {
jest.doMock('web-vitals/attribution', () => ({
onFID: jest.fn(cb => { triggeronFIDCallback = cb; cb({ value: 100, attribution: fidAttribution }) })
}))
const { firstInputDelay } = await import('../../../../src/common/vitals/first-input-delay')
codeToRun(firstInputDelay)
}

describe('fi (first interaction)', () => {
test('reports fi', (done) => {
getFreshImport(metric => {
metric.subscribe(({ value }) => {
expect(value).toEqual(1)
done()
})
})
describe('fid', () => {
test('reports fcp from web-vitals', (done) => {
getFreshFIDImport(metric => metric.subscribe(({ value, attrs }) => {
expect(value).toEqual(1)
expect(attrs.type).toEqual(fidAttribution.eventType)
expect(attrs.fid).toEqual(100)
expect(attrs.eventTarget).toEqual(fidAttribution.eventTarget)
expect(attrs.loadState).toEqual(fidAttribution.loadState)
done()
}))
})

test('Does NOT report values if initiallyHidden', (done) => {
Expand All @@ -47,7 +38,7 @@ describe('fi (first interaction)', () => {
isBrowserScope: true
}))

getFreshImport(metric => {
getFreshFIDImport(metric => {
metric.subscribe(() => {
console.log('should not have reported')
expect(1).toEqual(2)
Expand All @@ -63,7 +54,7 @@ describe('fi (first interaction)', () => {
isBrowserScope: false
}))

getFreshImport(metric => {
getFreshFIDImport(metric => {
metric.subscribe(({ value, attrs }) => {
console.log('should not have reported...')
expect(1).toEqual(2)
Expand All @@ -79,7 +70,7 @@ describe('fi (first interaction)', () => {
isBrowserScope: true
}))
let witness = 0
getFreshImport(metric => {
getFreshFIDImport(metric => {
metric.subscribe(({ value }) => {
expect(value).toEqual(1)
witness++
Expand All @@ -99,14 +90,15 @@ describe('fi (first interaction)', () => {
isBrowserScope: true
}))
let triggered = 0
getFreshImport(metric => metric.subscribe(({ value }) => {
triggered++
expect(value).toEqual(1)
expect(triggered).toEqual(1)
setTimeout(() => {
getFreshFIDImport(metric => {
metric.subscribe(({ value }) => {
triggered++
expect(value).toEqual(1)
expect(triggered).toEqual(1)
done()
}, 1000)
}))
})
triggeronFIDCallback({ value: 'notequal1' })
expect(triggered).toEqual(1)
done()
})
})
})
6 changes: 3 additions & 3 deletions tests/unit/features/page_view_timing/aggregate/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ function testCases () {
},
{
type: 'doubleAttribute',
key: 'foo',
key: 'fid',
value: 12.34
}
]
Expand Down Expand Up @@ -201,7 +201,7 @@ function testCases () {
},
{
type: 'doubleAttribute',
key: 'foo',
key: 'fid',
value: 12.34
}
]
Expand All @@ -218,7 +218,7 @@ function testCases () {
},
{
type: 'doubleAttribute',
key: 'foo',
key: 'fid',
value: 12.34
}
]
Expand Down
Loading
Loading