1153987 - TelemetryPing should fully initialize whenever canRecordBase=true

Status: RESOLVED WORKSFORME

(Toolkit :: Telemetry, defect)

Description

[Vladan Djeric (:vladan)]

We don't initialize TelemetryPing if canRecordExtended is false. This is incorrect, the Telemetry service (including TelemetryPing) needs to fully initialize if canRecordBase = true, independent of canRecordExtended.

Offending code from enableTelemetryRecording:

let enabled = Preferences.get(PREF_ENABLED, false);
if (!enabled || !Telemetry.canRecordBase) {
  // Turn off extended telemetry recording if disabled by preferences or if base/telemetry
  // telemetry recording is off.
  Telemetry.canRecordExtended = false;
  this._log.config("enableTelemetryRecording - Telemetry is disabled, turning off Telemetry recording.");
  return false;
}

I think this was an oversight from bug 1120369, so it will need to be uplifted to 38 & 39

Comment 1

[Vladan Djeric (:vladan)]

Attachment: fixTelemetryPingInit.patch

Comment 2

[Vladan Djeric (:vladan)]

Attachment: fixTelemetryPingInit2.patch

The patch in bug 1137252 fixes the issue, but we need to uplift a minimal fix to aurora & beta. I think this patch might be sufficient

Comment 3

[Vladan Djeric (:vladan)]

This was actually a regression from bug 1134279. We actually don't need this on beta, beta doesn't have canRecordBase

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8591837 [details] [diff] [review]
fixTelemetryPingInit2.patch

I think this should also carry the changes from  runreftest.py, all.js and profile.py from bug 1137252 part 1, otherwise tests may fail/crash (tests would try to access real telemetry servers in tests).

Comment 5

[Georg Fritzsche [:gfritzsche]]

Do we need to fix this on Aurora right now or should we just focus on a batched uplift that includes bug 1137252?

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Do we need to fix this on Aurora right now or should we just focus on a
> batched uplift that includes bug 1137252?

Per todays meeting: Yes, we should, to fix things until bug 1137252 uplifts.

Comment 7

[Georg Fritzsche [:gfritzsche]]

At this point we should not uplift this so turning the unified behavior off works properly on 39.

Comment 8

[Vladan Djeric (:vladan)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> At this point we should not uplift this so turning the unified behavior off
> works properly on 39.

Sorry, can you restate this?

Comment 9

[Georg Fritzsche [:gfritzsche]]

Sorry - i was checking whether we can turn off the "unified" behavior properly on 39.
Currently this is mostly fine, but if we land this patch on 39 we would need to do further work on that train.

Comment 10

[Georg Fritzsche [:gfritzsche]]

As discussed above and in todays meeting, the behavior on 39 right does not need to be fixed right now.
Proper initialization should be covered by bug 1148500.