Use only DOMHighResTimeStamp to calculate duration #143

asakusuma · 2021-06-17T04:24:01Z

Previously, we were combining Date.now() and DOMHighResTimeStamp values to calculate duration. These two value types use different clocks, so we should avoid combining the two value types where possible, since using two different clocks leaves us vulnerable to asymetrical issues. The native intersection observer uses DOMHighResTimeStamp, so we should use that type wherever possible in calculations.

This change also starts running the tests with and without native intersection observer. Previously we were not running tests with native intersection observer.

This change also removes the native-* files, which are superseded by USE_NATIVE_IO and never part of the public API.

Thanks to @xg-wang for pointing out some potential issues affecting one clock and not the other, which could in turn cause asymmetrical bugs:
w3c/hr-time#115
mdn/content#4713

xg-wang · 2021-06-17T18:01:32Z

src/interfaces.ts

  target: SpanielTrackedElement;
 }

-export interface SpanielObserverEntry extends IntersectionObserverEntryInit {
+export interface SpanielRect extends Partial<DOMRectReadOnly> {


Isn't this just Readonly<Required<DOMRectInit>>?
And why do we make this change for the duration fix?

The previous code was not accurately typing things. SpanielObserver operates of entries from the intersection observer, which can either be the native intersection observer or the spaniel intersection observer. But the entry was always being typed as the native IntersectionObserverEntry, whereas the entries from the spaniel intersection observer polyfill are not exactly the same because they are missing a few sub fields. When I addressed this issue, there was a cascading effect. Basically the typings have to respect the fact that the entries could be from native or polyfill IO.

xg-wang · 2021-06-17T18:08:49Z

src/spaniel-observer.ts

          payload: record.payload,
          label: state.threshold.label,
          entering: false,
          rootBounds: emptyRect,
          boundingClientRect: boundingClientRect || emptyRect,
          intersectionRect: emptyRect,
-          duration: time - state.lastVisible,
+          duration: state.lastVisible.highResTime - perfTime,


perfTime - state.lastVisible.highResTime? I guess test pass because we calculate opts.visibleTime = entry.unixTime - entry.duration; later.

oh ya. I'll add a test for the cases where this duration doesn't get overwritten

ok looks like this always gets overwritten anyways. really need to re-write this code.

xg-wang · 2021-06-17T18:26:36Z

src/intersection-observer.ts

+    x: interLeft,
+    y: interTop,


Are these new? I might have missed where are x and y read?

no, it's just that we previously were not actually returning a https://developer.mozilla.org/en-US/docs/Web/API/DOMRect
so adding it to be closer to the native behavior.

Looks like ClientRect is a subset of SpanielRect so there's no need to do ClientRect & SpanielRect. I also noticed there's built-in DOMRectReadOnly from https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts. So

spaniel/src/interfaces.ts

Line 83 in 6b74e4d

export interface DOMRectReadOnly extends DOMRectInit, DOMMargin {}

and #143 (comment) might not be needed

Ok I created export type DOMRectPojo = Omit<DOMRectReadOnly, 'toJSON'>;
Instead of ClientRect & SpanielRect

src/intersection-observer.ts

xg-wang · 2021-06-17T18:30:55Z

src/intersection-observer.ts

  if (el.style.display === 'none') {
    return {
+      unixTime: frame.dateNow,


Same above. Can we keep only one of time and unixTime

xg-wang · 2021-06-17T18:40:37Z

src/spaniel-observer.ts

    let { intersectionRatio, rootBounds, boundingClientRect, intersectionRect, isIntersecting, time, target } = entry;
    let record = this.recordStore[(<SpanielTrackedElement>target).__spanielId];
-    const timeOrigin = w.performance.timeOrigin || w.performance.timing.navigationStart;
-    const unixTime = this.usingNativeIo ? Math.floor(timeOrigin + time) : time;
+    const unixTime = this.usingNativeIo


This ultimately becomes visibleTime in the watcher callback right?

We should stick to Date.now for start time (visibleTime), and use hr time for calculating the duration. Mixing timeOrigin / timing.navigationStart and Date.now is not reliable.

Pausing the hr time clock during sleep is okay for us to calculate impression duration.

if we use Date.now when using native intersection observer, that's also lossy because the callback is async. So capturing a Date.now() in the callback is not necessarily when the event happened. do you have an opinion on which approach is less bad?

Also, I don't think we are ever mixing timeOrigin / timing.navigationStart and Date.now for any individual entry (as in, https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserverEntry). I would rather use the HR time all throughout the entire entry or Date throughout the entire entry.

The line below, Math.floor(w.performance.timeOrigin || w.performance.timing.navigationStart + time) is time unix time? The correct should be hrTime which is relative to timeOrigin. Logic looks incorrect to me.

Left another related comment at line 209.

If the logic is indeed incorrect, do we have a test to cover?

The correct should be hrTime which is relative to timeOrigin

Sort of, it's correct if we want to mirror native IO, but that is a breaking change.

This change just modifies how duration is calculated.

Added tests for visibleTime earlier today: https://github.com/linkedin/spaniel/blob/v2-release/test/playwright/modules/visible-time.ts

Ah wait I see what you mean, how using native IO previously meant using hr time. I will add another test for this behavior first.

wait no we always are using unix timestamp time for the time field, even for native IO: https://github.com/linkedin/spaniel/blob/v2-release/src/spaniel-observer.ts#L141
So we should keep that behavior for now until a breaking change. I'll add a test regardless

src/watcher.ts

src/interfaces.ts

xg-wang · 2021-08-11T17:11:10Z

src/spaniel-observer.ts

    record.thresholdStates.forEach((state: SpanielThresholdState) => {
      const boundingClientRect = record.lastSeenEntry && record.lastSeenEntry.boundingClientRect;
      this.handleThresholdExiting(
        {
          intersectionRatio: -1,
          isIntersecting: false,
+          unixTime: time,
          time,


Looks like this is not used anywhere anymore, can we remove?

While it's true that it's technically not needed anymore (because time is a unix timestamp), in next major version, I want to make time the high res timestamp to mirror the native observer API as closely as possible, get rid of the highRes property, and keep the unixTime property if you need unix time (which we do need a unix timestamp for visibleTime.

Added tests for visibleTime earlier today: https://github.com/linkedin/spaniel/blob/v2-release/test/playwright/modules/visible-time-tests.ts

xg-wang · 2021-08-11T17:21:29Z

src/spaniel-observer.ts

      spanielEntry.entering = false;
      state.visible = false;
      this.queuedEntries.push(spanielEntry);
    }

    clearTimeout(state.timeoutId);
  }
-  private handleObserverEntry(entry: IntersectionObserverEntry) {
+  private handleObserverEntry(entry: InternalIntersectionObserverEntry) {


I'm not sure about this.
handleObserverEntry is bound as IO callback, and receives hrTime, why switch to use InternalIntersectionObserverEntry which has extra fields? And it seems we generates unix time inside spaniel, but internalCallback will pass in hrTime.

xg-wang · 2021-08-11T17:22:49Z

src/spaniel-observer.ts

    let { intersectionRatio, rootBounds, boundingClientRect, intersectionRect, isIntersecting, time, target } = entry;
    let record = this.recordStore[(<SpanielTrackedElement>target).__spanielId];
-    const timeOrigin = w.performance.timeOrigin || w.performance.timing.navigationStart;
-    const unixTime = this.usingNativeIo ? Math.floor(timeOrigin + time) : time;
+    const unixTime = this.usingNativeIo


The line below, Math.floor(w.performance.timeOrigin || w.performance.timing.navigationStart + time) is time unix time? The correct should be hrTime which is relative to timeOrigin. Logic looks incorrect to me.

Left another related comment at line 209.

xg-wang · 2021-08-11T17:23:25Z

src/spaniel-observer.ts

    let { intersectionRatio, rootBounds, boundingClientRect, intersectionRect, isIntersecting, time, target } = entry;
    let record = this.recordStore[(<SpanielTrackedElement>target).__spanielId];
-    const timeOrigin = w.performance.timeOrigin || w.performance.timing.navigationStart;
-    const unixTime = this.usingNativeIo ? Math.floor(timeOrigin + time) : time;
+    const unixTime = this.usingNativeIo


If the logic is indeed incorrect, do we have a test to cover?

@xg-wang

Previously, we were combining Date.now() and DOMHighResTimeStamp values to calculate duration. These two value types use different clocks, so we should avoid combining the two value types where possible, since using two different clocks leaves us vulnerable to asymetrical issues. The native intersection observer uses DOMHighResTimeStamp, so we should use that type wherever possible in calculations. This change also removes the native-* files, which are superseded by USE_NATIVE_IO and never part of the public API. Thanks to @xg-wang for pointing out some potential issues affecting one clock and not the other, which could in turn cause asymmetrical bugs: w3c/hr-time#115 mdn/content#4713

asakusuma requested a review from seanjohnson08 June 17, 2021 04:24

xg-wang suggested changes Jun 17, 2021

View reviewed changes

asakusuma force-pushed the asa/no-cross-clock branch from fe2e64d to 20448a0 Compare June 18, 2021 08:12

xg-wang reviewed Jun 18, 2021

View reviewed changes

src/interfaces.ts Outdated Show resolved Hide resolved

xg-wang reviewed Jun 18, 2021

View reviewed changes

src/interfaces.ts Outdated Show resolved Hide resolved

asakusuma force-pushed the asa/no-cross-clock branch from 20448a0 to 3872c13 Compare August 10, 2021 18:28

xg-wang suggested changes Aug 11, 2021

View reviewed changes

asakusuma force-pushed the asa/no-cross-clock branch 5 times, most recently from 4f4c383 to 23a148b Compare August 18, 2021 18:43

xg-wang approved these changes Aug 18, 2021

View reviewed changes

asakusuma force-pushed the asa/no-cross-clock branch from 23a148b to ab5182c Compare August 18, 2021 19:16

asakusuma force-pushed the asa/no-cross-clock branch from ab5182c to 40c9d9e Compare August 18, 2021 19:22

asakusuma merged commit c8d1c9c into linkedin:v2-release Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only DOMHighResTimeStamp to calculate duration #143

Use only DOMHighResTimeStamp to calculate duration #143

asakusuma commented Jun 17, 2021 •

edited

Loading

xg-wang Jun 17, 2021

asakusuma Jun 18, 2021

xg-wang Jun 17, 2021

asakusuma Jun 18, 2021

asakusuma Jun 18, 2021

xg-wang Jun 17, 2021

asakusuma Jun 18, 2021

xg-wang Jun 18, 2021

asakusuma Aug 10, 2021

xg-wang Jun 17, 2021

asakusuma Jun 18, 2021

xg-wang Jun 17, 2021

asakusuma Jun 18, 2021

asakusuma Aug 10, 2021

xg-wang Aug 11, 2021

xg-wang Aug 11, 2021

asakusuma Aug 16, 2021 •

edited

Loading

asakusuma Aug 16, 2021

asakusuma Aug 17, 2021

xg-wang Aug 11, 2021

asakusuma Aug 13, 2021

asakusuma Aug 16, 2021 •

edited

Loading

xg-wang Aug 11, 2021

xg-wang Aug 11, 2021

xg-wang Aug 11, 2021

Use only DOMHighResTimeStamp to calculate duration #143

Use only DOMHighResTimeStamp to calculate duration #143

Conversation

asakusuma commented Jun 17, 2021 • edited Loading

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

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

asakusuma Aug 16, 2021 • edited Loading

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

asakusuma Aug 16, 2021 • edited Loading

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

asakusuma commented Jun 17, 2021 •

edited

Loading

asakusuma Aug 16, 2021 •

edited

Loading

asakusuma Aug 16, 2021 •

edited

Loading