1149537 - Daily subsession splits should not occur before midnight

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

(Benjamin Smedberg  [:bsmedberg] on bug 1139751, comment #18)
> I just want to check something here. It's important that no matter whether
> the ping is started before or after midnight, that the subsessionStartDate
> is for the correct day, so e.g.:
> 
> Monday       Midnight      Tuesday
> A          *                   B
> A                     *        B
> 
> no matter exactly when we split the subsession, "A" should have
> subsessionStartDate=Monday and "B" should have subsessionStartDate=Tuesday.

Comment 1

[Georg Fritzsche [:gfritzsche]]

* if the user is active, we shouldnt miss midnight by more then 5 min on either side
* if the user is idle, we would be ok with missing midnight by 30 min

Comment 2

[Georg Fritzsche [:gfritzsche]]

Per discussing things, we are best off if we simply don't submit daily pings before midnight.
That way we avoid data association issues in the first place and can use subsessionStartDate for identification.

A remaining concern for associating data with a specific day is a computer sleeping past midnight.
I will file a separate bug for triggering a session split on wakeup.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Only send daily pings after midnight

This patch changes the daily ping policy to generate a new daily ping only after midnight. Daily pings are no longer generated when the user is idle before midnight.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Only send daily pings after midnight

Forgot to qref.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Remove the daily on idle test

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8613935 [details] [diff] [review]
part 1 - Only send daily pings after midnight

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +438,5 @@
>    _sentDailyPingToday: function(nowDate) {
>      // This is today's date and also the previous midnight (0:00).
>      const todayDate = Utils.truncateToDays(nowDate);
> +    // We consider a ping sent for today if it occured after midnight.
> +    return (this._lastDailyPingTime >= (todayDate.getTime() - SCHEDULER_MIDNIGHT_TOLERANCE_MS));

If we always create the daily ping after midnight, we don't need any tolerance in the calculation.

@@ +458,5 @@
>  
> +    const lastMidnight = Utils.truncateToDays(nowDate);
> +    const isOverdue =
> +      (nowDate.getTime() - lastMidnight.getTime()) > SCHEDULER_MIDNIGHT_TOLERANCE_MS;
> +    if (isOverdue) {

I don't think we need the overdue logic anymore.
Do you see a scenario where it is needed?

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Only send daily pings after midnight - v2

Thanks Georg. I was thinking about a computer waking up from the sleep for the overdue but, as discussed over IRC, that case is already covered by the past midnight check.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8614075 [details] [diff] [review]
part 1 - Only send daily pings after midnight - v2

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

One thing i missed before: as we don't happen to trigger the daily pings before midnight anymore, we should adjust the midnight fuzzing accordingly.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +437,5 @@
>  
>    _sentDailyPingToday: function(nowDate) {
>      // This is today's date and also the previous midnight (0:00).
>      const todayDate = Utils.truncateToDays(nowDate);
> +    // We consider a ping sent for today if it occured after midnight.

"... after or at 00:00 today" or "... 12AM today".
Days start at 00:00 and run up to, exclusive, 24:00 (start of next day).

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Only send daily pings after midnight - v3

This patch changes the fuzzing function to delay pings, within the fuzzing range, after midnight.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Remove the daily on idle test - v2

Updated the patch to fix the broken midnight fuzzing test.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Only send daily pings after midnight - v3

Sorry, I attached the wrong patch.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Remove the daily on idle test - v2

Same here.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8614551 [details] [diff] [review]
part 1 - Only send daily pings after midnight - v3

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +502,5 @@
>    },
>  
>    /**
>     * This helper calculates the next time that we can send pings at.
> +   * Currently this mostly redistributes ping sends after midnight to avoid submission

It should not be "after", the spike starts at 00:00.
"... from midnight until one hour after ..."

@@ +520,1 @@
>      // Don't delay ping if we are not close to midnight.

This comment needs updating.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +447,5 @@
>     * @param {Object} nowDate A date object.
>     * @return {Boolean} True if we can send the daily ping, false otherwise.
>     */
>    _isDailyPingDue: function(nowDate) {
>      const sentPingToday = this._sentDailyPingToday(nowDate);

Nit: we don't use sentPingToday anywhere else here, we can just move it down below the comment on line 453 to make that clearer or just move the call into the condition.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Only send daily pings after midnight - v4

Thanks Georg. As discussed, this will land after the sending logic refactoring lands.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8614552 [details] [diff] [review]
part 2 - Remove the daily on idle test - v2

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

r=me with the below addressed.

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +281,5 @@
>  
>    gRequestIterator = Iterator(new Request());
>    yield TelemetryController.reset();
>  
>    // A ping submitted shortly before midnight should not get sent yet.

This comment is a left-over now, you want to move the ping handler down to where it's used too.

@@ -283,5 @@
>    yield TelemetryController.reset();
>  
>    // A ping submitted shortly before midnight should not get sent yet.
> -  now = new Date(2030, 5, 1, 23, 55, 0);
> -  fakeNow(now);

Let's instead do a check that we *do* send shortly before midnight now (23:59).

@@ +289,3 @@
>  
>    // A ping after midnight within the fuzzing delay should also not get sent.
>    now = new Date(2030, 5, 2, 0, 40, 0);

Let's add another check for not sending right before the end of the fuzzing delay too.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Remove the daily on idle test - v3

Comment 17

[Alessio Placitelli [:Dexter]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39ebcb0a8e18

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1fe7dce1c609
https://hg.mozilla.org/integration/fx-team/rev/ea849f73f3c9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1fe7dce1c609
https://hg.mozilla.org/mozilla-central/rev/ea849f73f3c9