1147522 - Move SessionRecorder out of datareporting to a more general shared location

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Note that we also have to take care of the |"@mozilla.org/datareporting/service;1" in Cc| checks guarding this.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Move session.jsm to toolkit/modules

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Move SessionRecorder management to TelemetryPing

Comment 4

[Georg Fritzsche [:gfritzsche]]

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.

Comment 5

[Georg Fritzsche [: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.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Move SessionRecorder management to TelemetryPing - v2

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

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Move SessionRecorder management to TelemetryPing - v2

Whoops. Forgot to qref.

Comment 8

[Georg Fritzsche [: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.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Move SessionRecorder management to TelemetryPing - v3

Rebased and addressed the comments.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Move SessionRecorder management to TelemetryPing - v3

Comment 11

[Alessio Placitelli [:Dexter]]

(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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ff5fea9f5332
https://hg.mozilla.org/integration/fx-team/rev/b0a2cf75403c

Comment 13

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/ff5fea9f5332
https://hg.mozilla.org/mozilla-central/rev/b0a2cf75403c

Comment 14

[Philip Chee]

Protip: Use |hg move| instead of deleting a file and then recreating it elsewhere. Doing this will preserve history/blame.

Comment 15

[Alessio Placitelli [:Dexter]]

(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.