Closed Bug 1147522 Opened 5 years ago Closed 5 years ago

Move SessionRecorder out of datareporting to a more general shared location

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

Attachments

(2 files, 4 obsolete files)

We started to use services/datareporting/sessions.jsm in Telemetry.
We should move it and associated tests to a proper shared location, toolkit/modules, so we can use it even when turning off FHR/DRS.
Note that we also have to take care of the |"@mozilla.org/datareporting/service;1" in Cc| checks guarding this.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8583969 - Flags: review?(gfritzsche)
Attachment #8583970 - Flags: review?(gfritzsche)
Attachment #8583969 - Flags: review?(gfritzsche) → review+
Comment on attachment 8583970 [details] [diff] [review]
part 2 - Move SessionRecorder management to TelemetryPing

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

::: services/healthreport/healthreporter.jsm
@@ +1188,5 @@
>   *
>   * @param policy
>   *        (HealthReportPolicy) Policy driving execution of HealthReporter.
>   */
> +this.HealthReporter = function (branch, policy, stateLeaf=null) {

Did you do a try push already?
Who knows what test or other code this (and other changes) might affect.

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +262,5 @@
>      return Impl._shutdownBarrier.client;
>     },
> +
> +   /**
> +    * The session recorder object.

"The session recorder instance managed by Telemetry"?

@@ +263,5 @@
>     },
> +
> +   /**
> +    * The session recorder object.
> +    * @return {Object} The session recorder or null if not available.

Point out that this is an instance of SessionRecorder.

@@ +706,5 @@
>  
> +    // Only initialize the session recorder if FHR is enabled.
> +    if (!this._sessionRecorder && Preferences.get(PREF_FHR_ENABLED, true)) {
> +      this._sessionRecorder = new SessionRecorder(PREF_SESSIONS_BRANCH);
> +      this._sessionRecorder.onStartup();

Move this initialization down to after the enableTelemetryRecording() check, then you can just skip the new check here.
Attachment #8583970 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> @@ +706,5 @@
> >  
> > +    // Only initialize the session recorder if FHR is enabled.
> > +    if (!this._sessionRecorder && Preferences.get(PREF_FHR_ENABLED, true)) {
> > +      this._sessionRecorder = new SessionRecorder(PREF_SESSIONS_BRANCH);
> > +      this._sessionRecorder.onStartup();
> 
> Move this initialization down to after the enableTelemetryRecording() check,
> then you can just skip the new check here.

Per IRC:
> right now |enableTelemetryRecording| returns false also if toolkit.telemetry.enabled is false
> so we can't move the session record init after that point
> not until the [bug 1137252] lands

Given that, we can just keep it there for now with a TODO comment so this bug is not blocked.
Thanks for the review, I've addressed your comments and rebased the patch.

Yes, I did a Linux-only try push during the development, but here's a full one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e9ec63ce2bb
Attachment #8583970 - Attachment is obsolete: true
Attachment #8587928 - Flags: review?(gfritzsche)
Whoops. Forgot to qref.
Attachment #8587928 - Attachment is obsolete: true
Attachment #8587928 - Flags: review?(gfritzsche)
Attachment #8587930 - Flags: review?(gfritzsche)
Comment on attachment 8587930 [details] [diff] [review]
part 2 - Move SessionRecorder management to TelemetryPing - v2

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

Please also manually confirm that FHR & Telemetry data dependent on this keeps working.

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +266,5 @@
> +   /**
> +    * The session recorder instance managed by Telemetry.
> +    * @return {Object} The active SessionRecorder instance or null if not available.
> +    */
> +   get sessionRecorder() {

After thinking again, let's keep this as getSessionRecorder() - that may make people more aware that this could fail / return null.

@@ +752,5 @@
>  
> +    // Only initialize the session recorder if FHR is enabled.
> +    // TODO: move this after the |enableTelemetryRecording| block and drop the
> +    // PREF_FHR_ENABLED check after bug 1137252 lands.
> +    if (!this._sessionRecorder && Preferences.get(PREF_FHR_ENABLED, true)) {

You should still move this down to after the initialization checks for now.
Attachment #8587930 - Flags: review?(gfritzsche) → review+
Rebased and addressed the comments.
Attachment #8587930 - Attachment is obsolete: true
Attachment #8588596 - Flags: review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Comment on attachment 8587930 [details] [diff] [review]
> part 2 - Move SessionRecorder management to TelemetryPing - v2
> 
> Review of attachment 8587930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please also manually confirm that FHR & Telemetry data dependent on this
> keeps working.

I've just double checked the following things:

- about:healthreport reports a coherent value for activeTicks (monotonically increasing integer).
- about:telemetry reports a coherent value for activeTicks (monotonically increasing integer, matching with the one reported by about:healthreport for the current session).
- activeTicks correctly breaks when starting a new subsession by inspecting the ping content.

It looks like this is ready to land.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff5fea9f5332
https://hg.mozilla.org/mozilla-central/rev/b0a2cf75403c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
Protip: Use |hg move| instead of deleting a file and then recreating it elsewhere. Doing this will preserve history/blame.
(In reply to Philip Chee from comment #14)
> Protip: Use |hg move| instead of deleting a file and then recreating it
> elsewhere. Doing this will preserve history/blame.

Thanks for the tip.
You need to log in before you can comment on or make changes to this bug.