1120363 - Break up Telemetry sessions on environment changes

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Bug 1069880 redesigns the environment data in telemetry pings.
As we always want to identify a specific session with one specific environment, we will need to break up sessions on environment changes (e.g. addon installation).

Bug 1120362 deals with breaking up telemetry sessions already, so doing this based on the work there should be easiest.

Comment 1

[Georg Fritzsche [:gfritzsche]]

> If possible, when the ping environment changes, the change-triggered ping
> should list the old value(s) to simplify some measurements.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Split subsessions on environment changes

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Split subsessions on environment changes

Rebase.

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 8566044 [details] [diff] [review]
Split subsessions on environment changes

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1318,5 @@
> +    this._log.trace("_onEnvironmentChange");
> +    let payload = this.getSessionPayload(REASON_ENVIRONMENT_CHANGE);
> +
> +    let options = {
> +      retentionDays: RETENTION_DAYS,

how is retentionDays going to be used?

@@ +1322,5 @@
> +      retentionDays: RETENTION_DAYS,
> +      addClientId: true,
> +      addEnvironment: true,
> +    };
> +    let promise = TelemetryPing.send(PING_TYPE_MAIN, payload, options);

- i'm not crazy about sending a ping on every environment change.. i'd rather batch pings up until an idle event. 
- also shouldn't this reschedule the daily timer?

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #4)
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1318,5 @@
> > +    this._log.trace("_onEnvironmentChange");
> > +    let payload = this.getSessionPayload(REASON_ENVIRONMENT_CHANGE);
> > +
> > +    let options = {
> > +      retentionDays: RETENTION_DAYS,
> 
> how is retentionDays going to be used?

This will be coming later (bug 1122481) and determines how long pings are kept locally.
We will increase the retention for the telemetry pings to 180 days (independent of submission state), because we need that data locally.
All pings will be deleted after RETENTION_DAYS if they were not submitted.

> @@ +1322,5 @@
> > +      retentionDays: RETENTION_DAYS,
> > +      addClientId: true,
> > +      addEnvironment: true,
> > +    };
> > +    let promise = TelemetryPing.send(PING_TYPE_MAIN, payload, options);
> 
> - i'm not crazy about sending a ping on every environment change.. i'd
> rather batch pings up until an idle event. 

Ok, as on bug 1120362 - could we move that to a follow-up for next week?

> - also shouldn't this reschedule the daily timer?

Fixing.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> > @@ +1322,5 @@
> > > +      retentionDays: RETENTION_DAYS,
> > > +      addClientId: true,
> > > +      addEnvironment: true,
> > > +    };
> > > +    let promise = TelemetryPing.send(PING_TYPE_MAIN, payload, options);
> > 
> > - i'm not crazy about sending a ping on every environment change.. i'd
> > rather batch pings up until an idle event. 
> 
> Ok, as on bug 1120362 - could we move that to a follow-up for next week?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Cleared up the above in a chat - we will go forward with this and consider better-performing approaches later after the first landing.

Comment 8

[Georg Fritzsche [:gfritzsche]]

> > > +      retentionDays: RETENTION_DAYS,
> > > +      addClientId: true,
> > > +      addEnvironment: true,
> > > +    };
> > > +    let promise = TelemetryPing.send(PING_TYPE_MAIN, payload, options);
> > 
...
> > - also shouldn't this reschedule the daily timer?

This already happens in getSessionPayload().

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Split subsessions on environment changes

Comment 10

[Vladan Djeric (:vladan)]

> > how is retentionDays going to be used?
> 
> This will be coming later (bug 1122481) and determines how long pings are
> kept locally.
> We will increase the retention for the telemetry pings to 180 days
> (independent of submission state), because we need that data locally.
> All pings will be deleted after RETENTION_DAYS if they were not submitted.

Ok. We should archive the old pings in a separate directory, so that Telemetry doesn't have to sift through 100+ pings on startup to find the few pings that haven't been uploaded.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #10)
> > > how is retentionDays going to be used?
> > 
> > This will be coming later (bug 1122481) and determines how long pings are
> > kept locally.
> > We will increase the retention for the telemetry pings to 180 days
> > (independent of submission state), because we need that data locally.
> > All pings will be deleted after RETENTION_DAYS if they were not submitted.
> 
> Ok. We should archive the old pings in a separate directory, so that
> Telemetry doesn't have to sift through 100+ pings on startup to find the few
> pings that haven't been uploaded.

Yes, definitely.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Pushed in: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=b83c3fa8c80f

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d06825c3cfc