Closed Bug 1143796 Opened 5 years ago Closed 5 years ago

Increasing TelemetryScheduler ticking interval when user is not active

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- unaffected
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

Attachments

(2 files, 1 obsolete file)

TelemetryScheduler currently ticks every 5 minutes: this interval must be bumped to an hour when the user is not active.
Blocks: 1120356
Blocks: 1069869
No longer blocks: 1120356
Blocks: 1120356
No longer blocks: 1069869
Blocks: 1069869
No longer blocks: 1120356
Depends on: 1133536
Depends on: 1139751
No longer depends on: 1133536
This patch allows TelemetryScheduler to understand when user is idle and changes the tick time accordingly.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8581766 - Flags: review?(gfritzsche)
Attached patch part 2 - Add test coverage. (obsolete) — Splinter Review
Adds test coverage.
Attachment #8581767 - Flags: review?(gfritzsche)
Comment on attachment 8581766 [details] [diff] [review]
part 1 - Enable idle detection in TelemetryScheduler.

Review of attachment 8581766 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +418,5 @@
>  
>    // The timer which drives the scheduler.
>    _schedulerTimer: null,
> +  // The interval used by the scheduler timer.
> +  _schedulerInterval: 0,

Lets just track the idle status in, say, _userIsIdle, and pass the right interval below accordingly.
Attachment #8581766 - Flags: review?(gfritzsche) → feedback+
Comment on attachment 8581767 [details] [diff] [review]
part 2 - Add test coverage.

Review of attachment 8581767 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +1526,5 @@
> +  // When not idle, the scheduler should have a 5 minutes tick interval.
> +  Assert.equal(schedulerTimeout, SCHEDULER_TICK_INTERVAL_MS);
> +
> +  // Send an "idle" notification to the scheduler.
> +  fakeIdleNotification("idle");

This can probably use a little less documenting:
// When idle, ...
fakeIdle...
Assert.equal(...

Dito below.
Attachment #8581767 - Flags: review?(gfritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Comment on attachment 8581766 [details] [diff] [review]
> part 1 - Enable idle detection in TelemetryScheduler.
> 
> Review of attachment 8581766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +418,5 @@
> >  
> >    // The timer which drives the scheduler.
> >    _schedulerTimer: null,
> > +  // The interval used by the scheduler timer.
> > +  _schedulerInterval: 0,
> 
> Lets just track the idle status in, say, _userIsIdle, and pass the right
> interval below accordingly.

Nevermind, i'll do that in bug 1139751.
Attachment #8581766 - Flags: feedback+ → review+
Attachment #8581767 - Attachment is obsolete: true
Attachment #8582247 - Flags: review+
Blocks: 1139751
No longer depends on: 1139751
Depends on: 1133536
Comment on attachment 8581766 [details] [diff] [review]
part 1 - Enable idle detection in TelemetryScheduler.

Review of attachment 8581766 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +512,5 @@
> +    switch(aTopic) {
> +      case "idle":
> +        // If the user is idle, increase the tick interval.
> +        this._schedulerInterval = SCHEDULER_TICK_IDLE_INTERVAL_MS;
> +        this._rescheduleTimeout();

you should take into account midnight for daily pings.. you don't want to miss midnight by 1hr+
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #7)
> you should take into account midnight for daily pings.. you don't want to
> miss midnight by 1hr+

This work is done in bug 1139751
Clearing the ni?, as bug 1139460 is the root cause for the shutdown crashes.
Flags: needinfo?(alessio.placitelli)
This and everything else from that push is backed out in https://hg.mozilla.org/integration/fx-team/rev/6901a267f856 for Windows PGO XPCShell failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=2520751&repo=fx-team
Flags: needinfo?(alessio.placitelli)
Clearing n?, as bug 1140558 is the root cause for this issue (see bug 1140558 comment 65).
Flags: needinfo?(alessio.placitelli)
https://hg.mozilla.org/mozilla-central/rev/ad335ce95140
https://hg.mozilla.org/mozilla-central/rev/c1a2d8973085
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8581766 [details] [diff] [review]
part 1 - Enable idle detection in TelemetryScheduler.

Approved for Aurora. For approval request see bug 1139460 comment 42. For approval comments see bug 1139460 comment 43.
Attachment #8581766 - Flags: approval-mozilla-aurora+
Attachment #8582247 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.