-
Notifications
You must be signed in to change notification settings - Fork 40
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
Changes from all commits
c31d1ad
c6d577d
17a0811
90cc7a0
49fa39a
cd6730a
31888aa
d6f9fd4
549a139
f1f22dc
1344f3c
7e37dbd
2a678cb
16ec88c
ec0949f
998404d
4aff4db
e63f05f
694040c
73b7895
3744536
66072f0
cfb0239
4682da1
7aede1e
9dc2294
8e7de0e
f414835
b3d01b0
4ce4409
ac12f93
00b28f3
3467200
b18910d
9f6b02f
f9c82e2
ea10a49
60d22f4
87be003
5928384
45f8401
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 { | ||||||
|
@@ -82,12 +84,36 @@ | |||||
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)) | ||||||
} | ||||||
|
||||||
getValue (key, value, type) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// 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 }) || {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this |
||||||
if (shouldHold) return true | ||||||
let browserInteractionId = interactions?.[0]?.id.replace('\'', '') | ||||||
if (browserInteractionId) value.params.browserInteractionId = browserInteractionId | ||||||
return false | ||||||
} | ||||||
Comment on lines
+104
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||
|
@@ -159,13 +185,3 @@ | |||||
c: 1 | ||||||
} | ||||||
} | ||||||
|
||||||
function toArray (obj) { | ||||||
if (typeof obj !== 'object') return [] | ||||||
|
||||||
return mapOwn(obj, getValue) | ||||||
} | ||||||
|
||||||
function getValue (key, value) { | ||||||
return value | ||||||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,38 @@ | ||
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 () { | ||
clearTimeout(this.#ltTimer) | ||
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 } }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. --Actually see related notes in initial page load class. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} |
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 {} | ||
} |
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 () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
|
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(';') | ||
} | ||
} |
There was a problem hiding this comment.
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 ofmapOwn