Closed Bug 1161032 Opened 5 years ago Closed 5 years ago

TelemetrySession shouldn't listen to "cycle-collector-begin" when extended recording is disabled

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

(Whiteboard: [uplift])

Attachments

(1 file, 2 obsolete files)

We actually bail out of gatherMemory() early when extended recording is off, so this doesn't have much impact.

However, when extended recording is off, we don't need to listen to "cycle-collector-begin" at all.
Let's also remove the "cycle-collector-begin notified" log statement while we're here.
It's bloating the log up without adding real information right now.
Attached patch bug1161032.patch (obsolete) — Splinter Review
This patch prevents TelemetrySession from subscribing to cycle-collector-begin if extended telemetry recording is disabled.

It also removes the log statement from |TelemetrySession.observe|.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8604071 - Flags: review?(gfritzsche)
Comment on attachment 8604071 [details] [diff] [review]
bug1161032.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1410,5 @@
>        return;
>      Services.obs.removeObserver(this, "idle-daily");
> +    if (Telemetry.canRecordExtended) {
> +      Services.obs.removeObserver(this, "cycle-collector-begin");
> +    }

What if canRecordExtended changes during the browser session?

@@ -1722,5 @@
>      if (!this._log) {
>        this._log = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, LOGGER_PREFIX);
>      }
>  
> -    this._log.trace("observe - " + aTopic + " notified.");

Tracing the other topcs can actually be useful.
Let's just log this when the topic is not "cycle-collector-begin".
Attachment #8604071 - Flags: review?(gfritzsche)
Attached patch bug1161032.patch -v2 (obsolete) — Splinter Review
Attachment #8604071 - Attachment is obsolete: true
Attachment #8605786 - Flags: review?(gfritzsche)
Comment on attachment 8605786 [details] [diff] [review]
bug1161032.patch -v2

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1410,5 @@
>        return;
>      Services.obs.removeObserver(this, "idle-daily");
> +    try {
> +      // Tests may flip Telemetry.canRecordExtended on and off. Just try to remove this
> +      // observer and catch if it fails because it's not available.

"if it fails because the observer was not added"

@@ +1412,5 @@
> +    try {
> +      // Tests may flip Telemetry.canRecordExtended on and off. Just try to remove this
> +      // observer and catch if it fails because it's not available.
> +      Services.obs.removeObserver(this, "cycle-collector-begin");
> +    } catch (e if e.result === Cr.NS_ERROR_FAILURE) {}

I think a generic |catch (e) { this._log.warn(...,e); }| is fine here.

@@ +1729,5 @@
>        this._log = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, LOGGER_PREFIX);
>      }
>  
> +    // Prevent the cycle collector begin topic from cluttering the log.
> +    if (aTopic != "cycle-collector-begin") {

That's a good time to make this a constant TOPIC_CYCLE_COLLECTOR_BEGIN.
Attachment #8605786 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f32ea4fc71d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [uplift]
Comment on attachment 8605814 [details] [diff] [review]
bug1161032.patch -v3

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8605814 - Flags: approval-mozilla-aurora?
Comment on attachment 8605814 [details] [diff] [review]
bug1161032.patch -v3

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.
Attachment #8605814 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.