Remove SessionRecorder.jsm

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Dexter)

Tracking

unspecified
mozilla55
Points:
2

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 789945
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.
Priority: -- → P3
Whiteboard: [measurement:client]
Summary: SessionRecorder writes to prefs every 5 seconds and on shutdown → Remove SessionRecorder.jsm
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1356035
Priority: P3 → P2
Points: --- → 2
(Assignee)

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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.
Attachment #8861048 - Flags: review?(gfritzsche)
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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()`?
Attachment #8861048 - Flags: review?(gfritzsche) → review+
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7a15ded20926
Remove SessionRecorder.jsm. r=gfritzsche

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a15ded20926
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 11

2 years ago
Madalin, could you validate this?
Flags: needinfo?(madalin.cotetiu)

Comment 12

2 years ago
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
Flags: needinfo?(madalin.cotetiu)
You need to log in before you can comment on or make changes to this bug.