1346223 - Remove SessionRecorder.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[:Gijs (he/him)]

This is the shutdown call:

https://dxr.mozilla.org/mozilla-central/rev/34585620e529614c79ecc007705646de748e592d/toolkit/modules/SessionRecorder.jsm#303-313

but when I refresh about:config, I also see the current session's total time increasing every 5 seconds.

If we implemented a delayed omt pref save, it would be kept busy just by this. This feels very inefficient - can we find another way of keeping track of session length? I also doubt that after about 30 seconds, we really care about the session length with 5 second granularity, or that after 2 hours even a 5 minute difference would make a lot of difference. Maybe at a minimum we could have some incremental backoff here.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I wonder if we need this for anything right now, this looks like its mostly a left-over from FHR times.

Searching for uses of this:
https://dxr.mozilla.org/mozilla-central/search?q=regexp%3Asessionrecorder&redirect=false

... this is the only actual use i can find:
https://dxr.mozilla.org/mozilla-central/rev/34585620e529614c79ecc007705646de748e592d/toolkit/components/telemetry/TelemetrySession.jsm#826

I think we should just migrate the activeTicks behavior:
https://dxr.mozilla.org/mozilla-central/search?q=path%3ASessionRecorder.jsm+regexp%3Aactiveticks&redirect=false
... to TelemetrySession.jsm and store it in properties there, not in prefs.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1346223 - Remove SessionRecorder.jsm.

This patch also moves the activeTicks logic to TelemetrySession.jsm
along with the related test coverage.

Review commit: https://reviewboard.mozilla.org/r/133042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133042/

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8861048 [details]
Bug 1346223 - Remove SessionRecorder.jsm.

https://reviewboard.mozilla.org/r/133042/#review136330

Great to see this needless complexity go away!

::: toolkit/components/telemetry/TelemetrySession.jsm:659
(Diff revision 1)
> +  get testActiveTicks() {
> +    return Impl._sessionActiveTicks;
> +  }

Why not just use `getPayload()` to test this?

::: toolkit/components/telemetry/TelemetrySession.jsm:672
(Diff revision 1)
>    _logger: null,
>    _prevValues: {},
>    _slowSQLStartup: {},
>    _hasWindowRestoredObserver: false,
>    _hasXulWindowVisibleObserver: false,
> +  _hasActiveTicksObservers: false,

Can we file a (mentored?) follow-up to make this into a pattern?

- storing active observers in, say, `this._observedTopics`
- book-keeping hidden away through helpers `this.addObserver()`, `this.removeObserver()`

::: toolkit/components/telemetry/TelemetrySession.jsm:715
(Diff revision 1)
>    // Start time of the current subsession using a monotonic clock for the subsession
>    // length measurements.
>    _subsessionStartTimeMonotonic: 0,
>    // The active ticks counted when the subsession starts
>    _subsessionStartActiveTicks: 0,
> +  // Session active ticks.

This comment should add some info over the variable name or be removed.
"Active ticks in the whole session."?

::: toolkit/components/telemetry/TelemetrySession.jsm:1940
(Diff revision 1)
>  
>    /**
> +   * Tracks the number of "ticks" the user was active in.
> +   */
> +  _handleActiveTicks(aUserActive) {
> +    const needsUpdate = aUserActive && !this._lastActivityWasInactive;

While we move this code, lets try to improve the clarity around what `_lastActiveWasInactive` does.

How about renaming this to `_isUserActive`?
(... and put a proper comment on its semantics where it is initially defined)

::: toolkit/components/telemetry/TelemetrySession.jsm:1943
(Diff revision 1)
> +   */
> +  _handleActiveTicks(aUserActive) {
> +    const needsUpdate = aUserActive && !this._lastActivityWasInactive;
> +    this._lastActivityWasInactive = !aUserActive;
> +
> +    if (needsUpdate) {

Lets also put a comment somewhere that explains what this tries to achieve here.

Presumably this is to not count the first "user-interaction-active", because it is just the start of this active tick.

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8861048 [details]
Bug 1346223 - Remove SessionRecorder.jsm.

https://reviewboard.mozilla.org/r/133042/#review136330

> Can we file a (mentored?) follow-up to make this into a pattern?
> 
> - storing active observers in, say, `this._observedTopics`
> - book-keeping hidden away through helpers `this.addObserver()`, `this.removeObserver()`

Filed bug 1359801!

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8861048 [details]
Bug 1346223 - Remove SessionRecorder.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133042/diff/1-2/

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8861048 [details]
Bug 1346223 - Remove SessionRecorder.jsm.

https://reviewboard.mozilla.org/r/133042/#review137650

::: toolkit/components/telemetry/TelemetrySession.jsm:1935
(Diff revision 2)
>    },
>  
>    /**
> +   * Tracks the number of "ticks" the user was active in.
> +   */
> +  _handleActiveTicks(aUserActive) {

Nit: `_onActiveTick()`?

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8861048 [details]
Bug 1346223 - Remove SessionRecorder.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133042/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a15ded20926
Remove SessionRecorder.jsm. r=gfritzsche

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a15ded20926

Comment 11

[Alessio Placitelli [:Dexter]]

Madalin, could you validate this?

Comment 12

[Madalin Cotetiu]

The testing is completed. All the details can be found in here: https://docs.google.com/document/d/1BiQMi8jO-DXRJyoxbbjttR9ufEPgU176LDHsm6xiihg/edit#
The test results are marked in here: https://docs.google.com/spreadsheets/d/1hLEgAs4H4FFCqxtscRsyIQlxGxXYmfXU1jAeWb6RIm0/edit#gid=0