Bug 204531 - Web Inspector: TabActivity diagnostic event should sample the active tab uniformly
Summary: Web Inspector: TabActivity diagnostic event should sample the active tab unif...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-22 12:13 PST by BJ Burg
Modified: 2019-12-06 11:34 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2019-11-22 15:07 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch (11.98 KB, patch)
2019-11-28 13:58 PST, BJ Burg
hi: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2019-11-22 12:13:08 PST
Currently, it will record an event every time a tab is switched (recording it as an interaction with the outgoing tab), which is wrong.
Comment 1 BJ Burg 2019-11-22 15:07:50 PST
Created attachment 384204 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-11-22 15:15:17 PST
<rdar://problem/57442539>
Comment 3 Devin Rousso 2019-11-22 17:31:46 PST
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".
Comment 4 BJ Burg 2019-11-27 10:40:35 PST
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.
Comment 5 BJ Burg 2019-11-28 13:58:44 PST
Created attachment 384473 [details]
Patch
Comment 6 Devin Rousso 2019-12-02 17:12:23 PST
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.
Comment 7 BJ Burg 2019-12-05 12:50:29 PST
(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.
Comment 8 BJ Burg 2019-12-06 11:34:46 PST
Committed r253211: <https://trac.webkit.org/changeset/253211>