chore: "Soft Nav" POC #626

metal-messiah · 2023-07-25T19:47:06Z

Please add a one-paragraph summary here, suitable for a release notes description. This will help with documentation.

Overview

Related Issue(s)

Testing

codecov · 2023-07-25T19:49:29Z

Codecov Report

Merging #626 (45f8401) into main (51bfee8) will decrease coverage by 0.30%.
Report is 1 commits behind head on main.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
- Coverage   86.74%   86.45%   -0.30%     
==========================================
  Files         136      137       +1     
  Lines        5273     5278       +5     
  Branches      724      734      +10     
==========================================
- Hits         4574     4563      -11     
- Misses        636      645       +9     
- Partials       63       70       +7

Flag	Coverage Δ
unit-tests	`78.62% <39.28%> (-0.43%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/cdn/experimental.js	`100.00% <ø> (ø)`
src/cdn/spa.js	`100.00% <ø> (ø)`
src/common/serialize/bel-serializer.js	`48.48% <0.00%> (-1.52%)`	⬇️
src/common/util/feature-flags.js	`96.42% <ø> (ø)`
src/common/vitals/constants.js	`100.00% <ø> (ø)`
src/features/ajax/aggregate/index.js	`91.39% <ø> (-1.06%)`	⬇️
src/features/page_view_event/aggregate/index.js	`87.75% <ø> (ø)`
src/features/spa/aggregate/index.js	`78.44% <ø> (ø)`
src/loaders/features/features.js	`100.00% <ø> (ø)`
src/common/util/feature-state.js	`16.66% <16.66%> (ø)`
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2023-07-25T19:49:40Z

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent	Asset	Previous Size	New Size	Diff
lite	loader	28.54 kB / 9.89 kB (gzip)	29.26 kB / 10.18 kB (gzip)	2.5% / 2.85% (gzip)
lite	async-chunk	45.53 kB / 15.1 kB (gzip)	45.78 kB / 15.14 kB (gzip)	0.55% / 0.3% (gzip)

pro	loader	45.95 kB / 15.38 kB (gzip)	46.72 kB / 15.68 kB (gzip)	1.69% / 1.93% (gzip)
pro	async-chunk	166.12 kB / 52.32 kB (gzip)	166.12 kB / 52.32 kB (gzip)	0% / 0% (gzip)

spa	loader	52.36 kB / 17.35 kB (gzip)	48.69 kB / 16.25 kB (gzip)	-7.02% / -6.34% (gzip)
spa	async-chunk	166.12 kB / 52.32 kB (gzip)	166.12 kB / 52.32 kB (gzip)	0% / 0% (gzip)

lite-polyfills	loader	99.04 kB / 31.55 kB (gzip)	99.95 kB / 31.9 kB (gzip)	0.92% / 1.11% (gzip)
lite-polyfills	async-chunk	57.95 kB / 17.32 kB (gzip)	58.23 kB / 17.36 kB (gzip)	0.48% / 0.27% (gzip)

pro-polyfills	loader	119.02 kB / 37.53 kB (gzip)	120.06 kB / 37.89 kB (gzip)	0.88% / 0.96% (gzip)
pro-polyfills	async-chunk	99.08 kB / 26.77 kB (gzip)	103.4 kB / 27.7 kB (gzip)	4.36% / 3.47% (gzip)

spa-polyfills	loader	126.99 kB / 39.67 kB (gzip)	122.84 kB / 38.66 kB (gzip)	-3.26% / -2.56% (gzip)
spa-polyfills	async-chunk	113.96 kB / 31.16 kB (gzip)	129.28 kB / 34.14 kB (gzip)	13.44% / 9.57% (gzip)

worker	loader	40.65 kB / 13.78 kB (gzip)	41.41 kB / 14.08 kB (gzip)	1.89% / 2.16% (gzip)
worker	async-chunk	45.51 kB / 15.44 kB (gzip)	47.56 kB / 16.1 kB (gzip)	4.51% / 4.26% (gzip)

… is merged

patrickhousley · 2023-09-13T19:38:23Z

src/common/aggregate/aggregator.js

+  toArray (type, obj) {
+    if (typeof obj !== 'object') return []
+
+    return mapOwn(obj, (key, value) => this.getValue(key, value, type))


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))

patrickhousley · 2023-09-13T19:38:36Z

src/common/aggregate/aggregator.js

+    return mapOwn(obj, (key, value) => this.getValue(key, value, type))
+  }
+
+  getValue (key, value, type) {


Suggested change

getValue (key, value, type) {

getValue (value, type) {

patrickhousley · 2023-09-13T20:44:47Z

src/features/basic_spa/instrument/index.js

+
+    windowAddEventListener('hashchange', trackURLChange, true, this.removeOnAbort?.signal)
+    windowAddEventListener('load', trackURLChange, true, this.removeOnAbort?.signal)
+    windowAddEventListener('popstate', trackURLChange, true, this.removeOnAbort?.signal)


Do we need both the history wrappers on lined 25-26 and this event listener?

These are ports over from the original spa feature, needs eval as to what is actually needed there

src/common/timing/time-to-interactive.js

src/features/basic_spa/aggregate/index.js

cwli24

So while this is less problem prone than the existing spa, it could use a big sweeping.

Overall, a lack of jsdoc, documentation behind the process, and unclear semantics make this hard to trace through. For example, on one line, I'm reading about "interactions" -> "ixn" to have it suddenly use a function that's within the BelNode class -- is an interaction simply a bel-serializer node or is it made up of nodes? It's confusing and unclear.
The way things are passed around is also reminiscent of a top issue with writing/reading/debugging javascript code with dynamic types: I'm getting lost on what a function should pass back and both the type and available properties of objects.

The other part of the confusion-complication is the way things are structured. For example, the belnode is responsible for cancel()-ing itself. Something that should be a simple data structure is sharing the logic/responsibility of its handler.

cwli24 · 2023-09-14T15:15:43Z

src/common/aggregate/aggregator.js

+    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 }) || {}
+    if (shouldHold) return true
+    let browserInteractionId = interactions?.[0]?.id.replace('\'', '')
+    if (browserInteractionId) value.params.browserInteractionId = browserInteractionId
+    return false
+  }


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

src/common/timing/now.js

src/common/util/feature-flags.js

cwli24 · 2023-09-14T15:52:01Z

src/common/timing/time-to-interactive.js

+  #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 } })


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

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

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

cwli24 · 2023-09-14T15:53:38Z

src/common/timing/time-to-interactive.js

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.

cwli24 · 2023-09-15T15:09:21Z

src/features/spa/aggregate/index.js

@@ -720,6 +720,8 @@ export class Aggregate extends AggregateBase {
      }
      baseEE.emit('interactionSaved', [interaction])
      state.interactionsToHarvest.push(interaction)
+


Reminder to delete

cwli24 · 2023-09-15T15:09:26Z

src/features/page_view_event/aggregate/index.js

@@ -109,6 +109,7 @@ export class Aggregate extends AggregateBase {
          activateFeatures(JSON.parse(responseText), this.agentIdentifier)
          this.drain()
        } catch (err) {
+          console.error(err)


Reminder to delete

cwli24 · 2023-09-15T15:09:44Z

src/features/utils/lazy-feature-loader.js

@@ -32,6 +32,8 @@ export function lazyFeatureLoader (featureName, featurePart) {
        return import(/* webpackChunkName: "session_trace-aggregate" */ '../session_trace/aggregate')
      case FEATURE_NAMES.spa:


Likewise can take old spa out

cwli24 · 2023-09-15T15:10:55Z

src/features/basic_spa/aggregate/index.js

+  }
+
+  hasInteraction ({ timestamp }) {
+    if (!timestamp) return { interaction: undefined }


I think this is meant to return interactions? Can you align function name & property singular or plural?

cwli24 · 2023-09-15T15:19:26Z

src/features/ajax/aggregate/index.js

+      } = spaFeature?.hasInteraction?.({ timestamp: event.startTime }) || {}
+
+      // if the ajax happened inside an ixn window (found a match), add it to the ixn
+      if (interactions?.length) {


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)?

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.

metal-messiah added 6 commits July 21, 2023 16:57

bones in place

c31d1ad

progress decorating other events

c6d577d

remove unused bit

17a0811

minor cleanup

90cc7a0

add interaction id to errors in aggregator

49fa39a

hold from sending if interaction is in progress

cd6730a

metal-messiah added the POC A proof of concept label Jul 25, 2023

minor clean up

31888aa

metal-messiah added 6 commits July 25, 2023 14:38

route changes create ixns

d6f9fd4

add harvesting and minor cleanup

549a139

add better handling

f1f22dc

use dom update and paint cycle to determine ixn end

1344f3c

merge with main

7e37dbd

set querypack back to main branch

2a678cb

metal-messiah changed the title ~~[WIP] Basic spa poc~~ chore: Basic spa POC Aug 1, 2023

compare side-by-side

16ec88c

metal-messiah changed the title ~~chore: Basic spa POC~~ chore: "Soft Nav" POC Sep 6, 2023

metal-messiah added 11 commits September 7, 2023 09:57

merge with main

ec0949f

fix merge conflict

998404d

fix merge conflicts

4aff4db

remove console logs before publishing experiment

e63f05f

add console logging

694040c

temporarily place in spa loader for experiment, revert when CI change…

73b7895

… is merged

try setting callbackEnd to end

3744536

fix drain typo

66072f0

try adding ajax children

cfb0239

add logging

4682da1

allow interaction to end itself after 30s

7aede1e

metal-messiah added 14 commits September 8, 2023 16:29

refining behavior -- adjusting timestamps

9dc2294

try to fix metadata initial page load

8e7de0e

try fixing timestamps

f414835

add metadata on when needed

b3d01b0

isolate metadata

4ce4409

messing with ajax passing

ac12f93

basic cleanup work

00b28f3

capture interactions after evaluating tti

3467200

save

b18910d

timeToInteractive module

9f6b02f

cbduration

f9c82e2

clean up invalid children

ea10a49

refinements

60d22f4

merge main

87be003

patrickhousley reviewed Sep 13, 2023

View reviewed changes

src/common/timing/time-to-interactive.js Show resolved Hide resolved

patrickhousley reviewed Sep 13, 2023

View reviewed changes

src/features/basic_spa/aggregate/index.js Show resolved Hide resolved

metal-messiah added 2 commits September 13, 2023 15:40

move promise out of constructor and cancel tti if node cancels

5928384

also clear tti timeout

45f8401

cwli24 reviewed Sep 15, 2023

View reviewed changes

cwli24 mentioned this pull request Oct 23, 2023

chore: Expose base agent instance under initializedAgents property #785

Merged

cwli24 mentioned this pull request Nov 9, 2023

feat: Soft Navigations trial #808

Merged

cwli24 closed this Dec 8, 2023

cwli24 deleted the basic-spa-poc branch January 29, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: "Soft Nav" POC #626

chore: "Soft Nav" POC #626

metal-messiah commented Jul 25, 2023

codecov bot commented Jul 25, 2023 •

edited

Loading

github-actions bot commented Jul 25, 2023 •

edited

Loading

patrickhousley Sep 13, 2023

patrickhousley Sep 13, 2023

patrickhousley Sep 13, 2023

metal-messiah Sep 13, 2023

cwli24 left a comment

cwli24 Sep 14, 2023

cwli24 Sep 14, 2023

cwli24 Sep 15, 2023

metal-messiah Sep 15, 2023

cwli24 Sep 14, 2023

cwli24 Sep 15, 2023

cwli24 Sep 15, 2023

cwli24 Sep 15, 2023

cwli24 Sep 15, 2023

cwli24 Sep 15, 2023

metal-messiah Sep 15, 2023

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

		@@ -32,6 +32,8 @@ export function lazyFeatureLoader (featureName, featurePart) {
		return import(/* webpackChunkName: "session_trace-aggregate" */ '../session_trace/aggregate')
		case FEATURE_NAMES.spa:

chore: "Soft Nav" POC #626

chore: "Soft Nav" POC #626

Conversation

metal-messiah commented Jul 25, 2023

Please add a one-paragraph summary here, suitable for a release notes description. This will help with documentation.

Overview

Related Issue(s)

Testing

codecov bot commented Jul 25, 2023 • edited Loading

Codecov Report

github-actions bot commented Jul 25, 2023 • edited Loading

Asset Size Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwli24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2023 •

edited

Loading

github-actions bot commented Jul 25, 2023 •

edited

Loading