Bug 204531 – Web Inspector: TabActivity diagnostic event should sample the active tab uniformly

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

Currently, it will record an event every time a tab is switched (recording it as an interaction with the outgoing tab), which is wrong.

[Blaze Burg]

Created attachment 384204 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/57442539>

[Devin Rousso]

Comment on attachment 384204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384204&action=review

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:36
> +        this._eventSamplingTimerIdentifier = 0;
> +        this._initialDelayBeforeSamplingTimerIdentifier = 0;

Given that we reset to `undefined`, we should start out with `undefined`.

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:51
> +        this._startInitialDelayBeforeSamplingTimer();

NIT: shouldn't this use `_startEventSamplingTimer` if `setup` is called way after Web Inspector is opened?

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:98
> +        this._initialDelayBeforeSamplingTimerIdentifier = setTimeout(this._sampleCurrentTabActivity.bind(this), WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval);

NIT: you can drop the `WI`

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:116
> +        this._eventSamplingTimerIdentifier = setTimeout(this._sampleCurrentTabActivity.bind(this), WI.TabActivityDiagnosticEventRecorder.EventSamplingInterval);

Ditto (98)

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:134
> +        let intervalSinceLastUserInteraction = Date.now() - this._lastUserInteractionTimestamp;

Should we use `performance.now()` instead?

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:135
> +        if (intervalSinceLastUserInteraction > WI.TabActivityDiagnosticEventRecorder.EventSamplingInterval) {

Ditto (98)

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:136
> +            if (WI.settings.debugAutoLogDiagnosticEvents.valueRespectingDebugUIAvailability)

Please add another check for `WI.isDebugUIEnabled()`.

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:156
> +        this._lastUserInteractionTimestamp = Date.now();

Ditto (134)

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:187
> +WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval = 10 * 1000;

Rather than add logic to wait 10s, could we just move the `setup()` call to right after `InspectorFrontendAPI.loadCompleted();` in `WI.contentLoaded`?  That's when we start receiving messages anyways, so it's when Web Inspector could be considered "usable".

[Blaze Burg]

Comment on attachment 384204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384204&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:36
>> +        this._initialDelayBeforeSamplingTimerIdentifier = 0;
> 
> Given that we reset to `undefined`, we should start out with `undefined`.

I find 0 to be a more useful sentinel value when debugging, as it is not a real timer identifier.

>> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:51
>> +        this._startInitialDelayBeforeSamplingTimer();
> 
> NIT: shouldn't this use `_startEventSamplingTimer` if `setup` is called way after Web Inspector is opened?

Yes. The trick is how to tell whether it's been a long time or not.

>> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:134
>> +        let intervalSinceLastUserInteraction = Date.now() - this._lastUserInteractionTimestamp;
> 
> Should we use `performance.now()` instead?

Sure ¯\_(ツ)_/¯

>> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:136
>> +            if (WI.settings.debugAutoLogDiagnosticEvents.valueRespectingDebugUIAvailability)
> 
> Please add another check for `WI.isDebugUIEnabled()`.

This check is rolled into the property getter.

>> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:187
>> +WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval = 10 * 1000;
> 
> Rather than add logic to wait 10s, could we just move the `setup()` call to right after `InspectorFrontendAPI.loadCompleted();` in `WI.contentLoaded`?  That's when we start receiving messages anyways, so it's when Web Inspector could be considered "usable".

Yes, it should use that to account for frontend loading delays. However, the initial delay (10 seconds) is not just to account for the delay while the frontend loads. It also accounts for the user needing a moment to look at the UI and make their initial action before we sample. I hypothesize that tab switching is often performed in the first few seconds of a user's session. If we sample immediately after the frontend is done loading, the sample will count the initial tab (which did not receive meaningful user interaction). 

TL;DR I worry the samples will be heavily biased towards whatever the default selected tab is.

One way to examine this more analytically would be to somehow log or compute a sampleIteration with each diagnostic event. If the distribution of active tabs looks way different when sampleIteration=1 than when sampleIteration>1, then this would indicate the first sample is being influenced by something.

[Blaze Burg]

Created attachment 384473 [details]
Patch

[Devin Rousso]

Comment on attachment 384473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384473&action=review

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:106
> +        let initialDelay = remainingTime > 0 ? remainingTime : maximumInitialDelay;

`Math.max(remainingTime, maximumInitialDelay)`

NIT: you could inline also this

> Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:196
> +WI.TabActivityDiagnosticEventRecorder.EventSamplingInterval = 60 * 1000;
> +WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval = 10 * 1000;

NIT: I've started using `static get` instead of creating these as a named property after the class' structure is defined.

[Blaze Burg]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 384473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384473&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:106
> > +        let initialDelay = remainingTime > 0 ? remainingTime : maximumInitialDelay;
> 
> `Math.max(remainingTime, maximumInitialDelay)`

This will always choose maximumInitialDelay. Instead it should be Math.min(Math.max(0, remainingTime), maximumInitialDelay). I suppose it could also be Number.constrain(remainingTime, 0, maximumInitialDelay).

> 
> NIT: you could inline also this
> 
> > Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:196
> > +WI.TabActivityDiagnosticEventRecorder.EventSamplingInterval = 60 * 1000;
> > +WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval = 10 * 1000;
> 
> NIT: I've started using `static get` instead of creating these as a named
> property after the class' structure is defined.

[Blaze Burg]

Committed r253211: <https://trac.webkit.org/changeset/253211>