Closed Bug 1120363 Opened 5 years ago Closed 5 years ago

Break up Telemetry sessions on environment changes

Categories

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

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Whiteboard: [ready])

Attachments

(1 file, 2 obsolete files)

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.
> If possible, when the ping environment changes, the change-triggered ping
> should list the old value(s) to simplify some measurements.
Assignee: nobody → gfritzsche
Attachment #8565720 - Flags: review?(vdjeric)
Status: NEW → ASSIGNED
Rebase.
Attachment #8565720 - Attachment is obsolete: true
Attachment #8565720 - Flags: review?(vdjeric)
Attachment #8566044 - Flags: review?(vdjeric)
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?
Attachment #8566044 - Flags: review?(vdjeric)
(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.
(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?
Flags: needinfo?(vdjeric)
Cleared up the above in a chat - we will go forward with this and consider better-performing approaches later after the first landing.
Flags: needinfo?(vdjeric)
> > > +      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().
Attachment #8566044 - Attachment is obsolete: true
Attachment #8567242 - Flags: review?(vdjeric)
> > 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.
Attachment #8567242 - Flags: review?(vdjeric) → review+
(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.
Whiteboard: [ready]
https://hg.mozilla.org/mozilla-central/rev/8d06825c3cfc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.