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

chore: "Soft Nav" POC #626

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
c31d1ad
bones in place
metal-messiah Jul 21, 2023
c6d577d
progress decorating other events
metal-messiah Jul 24, 2023
17a0811
remove unused bit
metal-messiah Jul 25, 2023
90cc7a0
minor cleanup
metal-messiah Jul 25, 2023
49fa39a
add interaction id to errors in aggregator
metal-messiah Jul 25, 2023
cd6730a
hold from sending if interaction is in progress
metal-messiah Jul 25, 2023
31888aa
minor clean up
metal-messiah Jul 25, 2023
d6f9fd4
route changes create ixns
metal-messiah Jul 25, 2023
549a139
add harvesting and minor cleanup
metal-messiah Jul 26, 2023
f1f22dc
add better handling
metal-messiah Jul 26, 2023
1344f3c
use dom update and paint cycle to determine ixn end
metal-messiah Aug 1, 2023
7e37dbd
merge with main
metal-messiah Aug 1, 2023
2a678cb
set querypack back to main branch
metal-messiah Aug 1, 2023
16ec88c
compare side-by-side
metal-messiah Aug 7, 2023
ec0949f
merge with main
metal-messiah Sep 7, 2023
998404d
fix merge conflict
metal-messiah Sep 7, 2023
4aff4db
fix merge conflicts
metal-messiah Sep 7, 2023
e63f05f
remove console logs before publishing experiment
metal-messiah Sep 7, 2023
694040c
add console logging
metal-messiah Sep 7, 2023
73b7895
temporarily place in spa loader for experiment, revert when CI change…
metal-messiah Sep 7, 2023
3744536
try setting callbackEnd to end
metal-messiah Sep 8, 2023
66072f0
fix drain typo
metal-messiah Sep 8, 2023
cfb0239
try adding ajax children
metal-messiah Sep 8, 2023
4682da1
add logging
metal-messiah Sep 8, 2023
7aede1e
allow interaction to end itself after 30s
metal-messiah Sep 8, 2023
9dc2294
refining behavior -- adjusting timestamps
metal-messiah Sep 8, 2023
8e7de0e
try to fix metadata initial page load
metal-messiah Sep 8, 2023
f414835
try fixing timestamps
metal-messiah Sep 9, 2023
b3d01b0
add metadata on when needed
metal-messiah Sep 9, 2023
4ce4409
isolate metadata
metal-messiah Sep 9, 2023
ac12f93
messing with ajax passing
metal-messiah Sep 11, 2023
00b28f3
basic cleanup work
metal-messiah Sep 11, 2023
3467200
capture interactions after evaluating tti
metal-messiah Sep 11, 2023
b18910d
save
metal-messiah Sep 12, 2023
9f6b02f
timeToInteractive module
metal-messiah Sep 12, 2023
f9c82e2
cbduration
metal-messiah Sep 12, 2023
ea10a49
clean up invalid children
metal-messiah Sep 13, 2023
60d22f4
refinements
metal-messiah Sep 13, 2023
87be003
merge main
metal-messiah Sep 13, 2023
5928384
move promise out of constructor and cancel tti if node cancels
metal-messiah Sep 13, 2023
45f8401
also clear tti timeout
metal-messiah Sep 13, 2023
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: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions src/cdn/experimental.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import { Instrument as InstrumentErrors } from '../features/jserrors/instrument'
import { Instrument as InstrumentXhr } from '../features/ajax/instrument'
import { Instrument as InstrumentSessionTrace } from '../features/session_trace/instrument'
import { Instrument as InstrumentSessionReplay } from '../features/session_replay/instrument'
import { Instrument as InstrumentSpa } from '../features/spa/instrument'
// import { Instrument as InstrumentSpa } from '../features/spa/instrument'
import { Instrument as InstrumentBasicSpa } from '../features/basic_spa/instrument'
import { Instrument as InstrumentPageAction } from '../features/page_action/instrument'

new Agent({
Expand All @@ -30,7 +31,8 @@ new Agent({
InstrumentMetrics,
InstrumentPageAction,
InstrumentErrors,
InstrumentSpa
// InstrumentSpa,
InstrumentBasicSpa
],
loaderType: 'experimental'
})
4 changes: 2 additions & 2 deletions src/cdn/spa.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Instrument as InstrumentErrors } from '../features/jserrors/instrument'
import { Instrument as InstrumentXhr } from '../features/ajax/instrument'
import { Instrument as InstrumentSessionTrace } from '../features/session_trace/instrument'
import { Instrument as InstrumentSessionReplay } from '../features/session_replay/instrument'
import { Instrument as InstrumentSpa } from '../features/spa/instrument'
import { Instrument as InstrumentBasicSpa } from '../features/basic_spa/instrument'
import { Instrument as InstrumentPageAction } from '../features/page_action/instrument'

new Agent({
Expand All @@ -24,7 +24,7 @@ new Agent({
InstrumentMetrics,
InstrumentPageAction,
InstrumentErrors,
InstrumentSpa
InstrumentBasicSpa
],
loaderType: 'spa'
})
38 changes: 27 additions & 11 deletions src/common/aggregate/aggregator.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { FEATURE_NAMES } from '../../loaders/features/features'
import { SharedContext } from '../context/shared-context'
import { getFeatureState } from '../util/feature-state'
import { mapOwn } from '../util/map-own'

export class Aggregator extends SharedContext {
Expand Down Expand Up @@ -82,12 +84,36 @@ export class Aggregator extends SharedContext {
var hasData = false
for (var i = 0; i < types.length; i++) {
type = types[i]
results[type] = toArray(this.aggregatedData[type])
results[type] = this.toArray(type, this.aggregatedData[type])

if (results[type].length) hasData = true
delete this.aggregatedData[type]
}
return hasData ? results : null
}

toArray (type, obj) {
if (typeof obj !== 'object') return []

return mapOwn(obj, (key, value) => this.getValue(key, value, type))
Copy link
Contributor

Choose a reason for hiding this comment

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

The key parameter is never used and I would like to continue removing uses of mapOwn

Suggested change
return mapOwn(obj, (key, value) => this.getValue(key, value, type))
return Object.values(obj).map(value => this.getValue(value, type))

}

getValue (key, value, type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
getValue (key, value, type) {
getValue (value, type) {

// err type can be decorated with an interaction ID if present
if (type !== 'err') return value
const shouldHold = this.checkInteractions(value) // add an interaction ID if exists
if (shouldHold) return
return value
}

checkInteractions (value) {
const spaFeature = getFeatureState({ agentIdentifier: this.sharedContext.agentIdentifier, featureName: FEATURE_NAMES.basicSpa })
const { shouldHold, interactions } = spaFeature?.hasInteraction?.({ timestamp: value?.metrics?.time?.t }) || {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this shouldHold? the .hasInteraction only ever returns an object with just a single interactions property.

if (shouldHold) return true
let browserInteractionId = interactions?.[0]?.id.replace('\'', '')
if (browserInteractionId) value.params.browserInteractionId = browserInteractionId
return false
}
Comment on lines +104 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

A styling remark but there's something--or a combination of things--that makes this not obvious as to what's happening.

}

function aggregateMetrics (newMetrics, oldMetrics) {
Expand Down Expand Up @@ -159,13 +185,3 @@ function createMetricObject (value) {
c: 1
}
}

function toArray (obj) {
if (typeof obj !== 'object') return []

return mapOwn(obj, getValue)
}

function getValue (key, value) {
return value
}
4 changes: 2 additions & 2 deletions src/common/serialize/bel-serializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function getAddStringContext (agentIdentifier) {
}
}

export function addCustomAttributes (attrs, addString) {
export function addCustomAttributes (attrs, addString, shouldSerialize) {
var attrParts = []

mapOwn(attrs, function (key, val) {
Expand Down Expand Up @@ -83,7 +83,7 @@ export function addCustomAttributes (attrs, addString) {
attrParts.push([type, key + (serializedValue ? ',' + serializedValue : '')])
})

return attrParts
return shouldSerialize ? attrParts.map(v => ({ serialize: () => v.join(',') })) : attrParts
}

var escapable = /([,\\;])/g
Expand Down
2 changes: 1 addition & 1 deletion src/common/timing/now.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@

// This is our own layer around performance.now. It's not strictly necessary, but we keep it in case of future mod-ing of the value for refactor purpose.
export function now () {
return Math.round(performance.now())
return Math.floor(performance.now())
cwli24 marked this conversation as resolved.
Show resolved Hide resolved
}
37 changes: 37 additions & 0 deletions src/common/timing/time-to-interactive.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put comments in here about how this is being calculated, e.g. citing whatever source?

Also, assuming this is following webdev, it's missing both the max 2 in-flight network GET factor and the FCP should be gotten within this module so that this is a self-contained metric.

Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { longTask } from '../vitals/long-task'

export class TimeToInteractive {
#ltTimer
#latestLT
#ltUnsub
#startTimestamp
#resolver
#rejector
#longTasks = 0

start ({ startTimestamp, buffered = false }) {
this.#startTimestamp = startTimestamp
this.#ltUnsub = longTask.subscribe(({ name, value, attrs }) => {
this.#latestLT = attrs.ltStart + value
this.#longTasks++
this.#setLtTimer()
}, buffered)
return new Promise((resolve, reject) => {
metal-messiah marked this conversation as resolved.
Show resolved Hide resolved
this.#resolver = resolve
this.#rejector = reject
})
}

cancel () {
this.#ltUnsub()
// this.#rejector()
}

#setLtTimer () {
clearTimeout(this.#ltTimer)
this.#ltTimer = setTimeout(() => {
this.#resolver({ value: Math.round(Math.max(this.#startTimestamp, this.#latestLT)), attrs: { startTimestamp: this.#startTimestamp, longTasks: this.#longTasks } })
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is #startTimestamp (passed in as) and why is it some external value?

Copy link
Contributor

Choose a reason for hiding this comment

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

--Actually see related notes in initial page load class.

Copy link
Member Author

Choose a reason for hiding this comment

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

essentially to accommodate for either being started on the fly, or representing a past time, ie from the page load origin time

this.#ltUnsub()
}, 5000)
}
}
2 changes: 1 addition & 1 deletion src/common/util/feature-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const bucketMap = {
stn: [FEATURE_NAMES.sessionTrace],
err: [FEATURE_NAMES.jserrors, FEATURE_NAMES.metrics],
ins: [FEATURE_NAMES.pageAction],
spa: [FEATURE_NAMES.spa],
spa: [FEATURE_NAMES.spa, FEATURE_NAMES.basicSpa],
cwli24 marked this conversation as resolved.
Show resolved Hide resolved
sr: [FEATURE_NAMES.sessionReplay, FEATURE_NAMES.sessionTrace]
}

Expand Down
8 changes: 4 additions & 4 deletions src/common/util/feature-flags.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ test('emits the right events when feature flag = 1', () => {

const sharedEE = jest.mocked(eventEmitterModule.ee.get).mock.results[0].value

// each flag gets emitted to each of its mapped features, and a feat- AND a rumresp- for every emit, so (1+2+1+1+2)*2 = 14
expect(handleModule.handle).toHaveBeenCalledTimes(14)
// each flag gets emitted to each of its mapped features, and a feat- AND a rumresp- for every emit, so (1+2+1+1+2+2)*2 = 16
expect(handleModule.handle).toHaveBeenCalledTimes(16)
expect(handleModule.handle).toHaveBeenNthCalledWith(1, 'feat-stn', [], undefined, FEATURE_NAMES.sessionTrace, sharedEE)
expect(handleModule.handle).toHaveBeenLastCalledWith('rumresp-sr', [true], undefined, FEATURE_NAMES.sessionTrace, sharedEE)

Expand All @@ -69,8 +69,8 @@ test('emits the right events when feature flag = 0', () => {

const sharedEE = jest.mocked(eventEmitterModule.ee.get).mock.results[0].value

// each flag gets emitted to each of its mapped features, and a block- AND a rumresp- for every emit, so (1+2+1+1+2)*2 = 14
expect(handleModule.handle).toHaveBeenCalledTimes(14)
// each flag gets emitted to each of its mapped features, and a block- AND a rumresp- for every emit, so (1+2+1+1+2+2)*2 = 16
expect(handleModule.handle).toHaveBeenCalledTimes(16)
expect(handleModule.handle).toHaveBeenNthCalledWith(1, 'block-stn', [], undefined, FEATURE_NAMES.sessionTrace, sharedEE)
expect(handleModule.handle).toHaveBeenLastCalledWith('rumresp-sr', [false], undefined, FEATURE_NAMES.sessionTrace, sharedEE)

Expand Down
13 changes: 13 additions & 0 deletions src/common/util/feature-state.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { gosNREUM } from '../window/nreum'

export const FEATURE_TYPE = {
AGGREGATE: 0,
INSTRUMENT: 1
}

export function getFeatureState ({ agentIdentifier, featureName, featureType = FEATURE_TYPE.AGGREGATE }) {
const nr = gosNREUM()
if (featureType === FEATURE_TYPE.AGGREGATE) return nr.initializedAgents[agentIdentifier]?.features?.[featureName]?.featAggregate || {}
if (featureType === FEATURE_TYPE.INSTRUMENT) return nr.initializedAgents[agentIdentifier]?.features?.[featureName] || {}
return {}
}
3 changes: 2 additions & 1 deletion src/common/vitals/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ export const VITAL_NAMES = {
CUMULATIVE_LAYOUT_SHIFT: 'cls',
INTERACTION_TO_NEXT_PAINT: 'inp',
LONG_TASK: 'lt',
TIME_TO_FIRST_BYTE: 'ttfb'
TIME_TO_FIRST_BYTE: 'ttfb',
TIME_TO_INTERACTIVE: 'tti'
}
2 changes: 1 addition & 1 deletion src/common/window/load.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { windowAddEventListener, documentAddEventListener } from '../event-listener/event-listener-opts'

function checkState () {
export function checkState () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't being used anywhere though?

return (typeof document === 'undefined' || document.readyState === 'complete')
}

Expand Down
38 changes: 18 additions & 20 deletions src/features/ajax/aggregate/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { FEATURE_NAME } from '../constants'
import { FEATURE_NAMES } from '../../../loaders/features/features'
import { SUPPORTABILITY_METRIC_CHANNEL } from '../../metrics/constants'
import { AggregateBase } from '../../utils/aggregate-base'
import { getFeatureState } from '../../../common/util/feature-state'
import { AjaxNode } from '../../basic_spa/aggregate/ajax-node'

export class Aggregate extends AggregateBase {
static featureName = FEATURE_NAME
Expand Down Expand Up @@ -43,21 +45,6 @@ export class Aggregate extends AggregateBase {
this.prepareHarvest = prepareHarvest
this.getStoredEvents = function () { return { ajaxEvents, spaAjaxEvents } }

ee.on('interactionSaved', (interaction) => {
if (!spaAjaxEvents[interaction.id]) return
// remove from the spaAjaxEvents buffer, and let spa harvest it
delete spaAjaxEvents[interaction.id]
})
ee.on('interactionDiscarded', (interaction) => {
if (!spaAjaxEvents[interaction.id]) return

spaAjaxEvents[interaction.id].forEach(function (item) {
// move it from the spaAjaxEvents buffer to the ajaxEvents buffer for harvesting here
ajaxEvents.push(item)
})
delete spaAjaxEvents[interaction.id]
})

const scheduler = new HarvestScheduler('events', {
onFinished: onEventsHarvestFinished,
getPayload: prepareHarvest
Expand Down Expand Up @@ -119,11 +106,19 @@ export class Aggregate extends AggregateBase {
event.spanTimestamp = xhrContext.dt.timestamp
}

// if the ajax happened inside an interaction, hold it until the interaction finishes
if (this.spaNode) {
var interactionId = this.spaNode.interaction.id
spaAjaxEvents[interactionId] = spaAjaxEvents[interactionId] || []
spaAjaxEvents[interactionId].push(event)
const spaFeature = getFeatureState({ agentIdentifier, featureName: FEATURE_NAMES.basicSpa })
const {
interactions
} = spaFeature?.hasInteraction?.({ timestamp: event.startTime }) || {}

// if the ajax happened inside an ixn window (found a match), add it to the ixn
if (interactions?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there is more than 1 interaction that's returned? How can it make sense that there's more than 1 interaction at a time and that this ajax is a child of every one (multiple)?

Copy link
Member Author

Choose a reason for hiding this comment

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

it could be a child of both a page load interaction and a click interaction if both are happening simultaneously while the ajax occurs under this model, since its timestamp-based alone.

interactions.forEach(interaction => {
const node = new AjaxNode(agentIdentifier, event)
interaction.addChild(node)
// add the ajax event back to the ajax feature queue if the ixn cancels
node.on('cancelled', () => { ajaxEvents.push(event) }) // this needs to be fixed later to avoid duplicates
})
} else {
ajaxEvents.push(event)
}
Expand Down Expand Up @@ -200,6 +195,9 @@ export class Aggregate extends AggregateBase {

for (var i = 0; i < events.length; i++) {
var event = events[i]

// if (interaction) console.log('ajax', event, 'has FOUND AJAX INTERACTION!', interaction)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to delete


var fields = [
numeric(event.startTime),
numeric(event.endTime - event.startTime),
Expand Down
64 changes: 64 additions & 0 deletions src/features/basic_spa/aggregate/ajax-node.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { getAddStringContext, nullable, numeric } from '../../../common/serialize/bel-serializer'
import { TYPE_IDS } from '../constants'
import { BelNode } from './bel-node'

export class AjaxNode extends BelNode {
method
status
domain
path
txSize // request body size
rxSize // response body size
requestedWith // isFetch (1) | isJSONP (2) | XMLHttpRequest ('')
spanId
traceId
spanTimestamp

constructor (agentIdentifier, ajaxEvent) {
super(agentIdentifier)
this.belType = TYPE_IDS.AJAX
this.method = ajaxEvent.method
this.status = ajaxEvent.status
this.domain = ajaxEvent.domain
this.path = ajaxEvent.path
this.txSize = ajaxEvent.requestSize
this.rxSize = ajaxEvent.responseSize
this.requestedWith = ajaxEvent.type === 'fetch' ? 1 : ajaxEvent.type === 'jsonp' ? 2 : '' // does this actually work?
this.spanId = ajaxEvent.spanId
this.traceId = ajaxEvent.traceId
this.spanTimestamp = ajaxEvent.spanTimestamp

this.start = ajaxEvent.startTime
this.end = ajaxEvent.endTime
this.callbackDuration = ajaxEvent.callbackDuration
this.callbackEnd = this.callbackDuration
}

serialize (startTimestamp) {
const addString = getAddStringContext(this.agentIdentifier)
const nodeList = []
const fields = [
numeric(this.belType),
this.children.length,
numeric(this.start - startTimestamp), // start relative to first seen (parent interaction)
numeric(this.end - this.start), // end is relative to start
numeric(this.callbackEnd),
numeric(this.callbackDuration),
addString(this.method),
numeric(this.status),
addString(this.domain),
addString(this.path),
numeric(this.txSize),
numeric(this.rxSize),
this.requestedWith,
addString(this.nodeId),
nullable(this.spanId, addString, true),
nullable(this.traceId, addString, true),
nullable(this.spanTimestamp, numeric, true)
]

nodeList.push(fields)

return nodeList.join(';')
}
}
Loading