Closed Bug 1281602 Opened 3 years ago Closed 3 years ago

Redo automigration telemetry to use an enumerated histogram

Categories

(Firefox :: Migration, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [migration-needs-uplift])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1271775 +++

See bug 1271775 comment 15 and later.
Comment on attachment 8764510 [details]
Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

https://reviewboard.mozilla.org/r/60348/#review57450

data-review=me I did not review the code changes.
Attachment #8764510 - Flags: review?(benjamin) → review+
Attachment #8764510 - Flags: review?(dolske)
https://reviewboard.mozilla.org/r/60348/#review57900

::: browser/components/migration/AutoMigrate.jsm:31
(Diff revision 1)
>     */
>    migrate(profileStartup, migratorKey, profileToMigrate) {
> -    let histogram = Services.telemetry.getKeyedHistogramById("FX_STARTUP_MIGRATION_AUTOMATED_IMPORT_SUCCEEDED");
> -    histogram.add("initialized");
> +    let histogram = Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_AUTOMATED_IMPORT_PROCESS_SUCCESS");
> +    histogram.add(0);
>      let migrator = this.pickMigrator(migratorKey);
> -    histogram.add("got-browser");
> +    histogram.add(4);

I was confused by the choice of numbers in the probe, especially because of the strange spacing. (0,4,8,10,15,18) AIUI it's basically just a counter, with some extra space to allow for growth.

Can we just make the (initial) spacing consistent so that's clearer? Say, increments of 5 or 10?

::: browser/components/migration/AutoMigrate.jsm:47
(Diff revision 1)
>      let sawErrors = false;
>      let migrationObserver = function(subject, topic, data) {
>        if (topic == "Migration:ItemError") {
>          sawErrors = true;
>        } else if (topic == "Migration:Ended") {
> -        histogram.add(sawErrors ? "finished-with-errors" : "finished");
> +        histogram.add(sawErrors ? 18 : 19);

Do we need a separate error state? We already have FX_MIGRATION_ERRORS.

Althoguh I suppose this might make simple monitoring of the probe easier, because you don't need an actual analysis to see how many automigrations failed.

Would it make sense to do it like this, so that it's clearer how many total completions there were?

if (topic == "Migration:Ended") {
  histogram.add(20);
  if (sawErrors)
    histogram.add(21);
}
Comment on attachment 8764510 [details]
Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

https://reviewboard.mozilla.org/r/60348/#review57964
Attachment #8764510 - Flags: review?(dolske) → review+
(Sorry, thought I marked that r+ earlier but found it in my queue again.)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/e1af8a633527
rework automigration telemetry to use an enumerated histogram, r=bsmedberg,dolske
https://hg.mozilla.org/mozilla-central/rev/e1af8a633527
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1283542
Whiteboard: [migration-needs-uplift]
Comment on attachment 8764510 [details]
Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

Approval Request Comment
[Feature/regressing bug #]: automigration, used in funnelcake builds 88 (bug 1295873)
[User impact if declined]: no funnelcake. :-(
[Describe test coverage new/current, TreeHerder]: telemetry-only change, the module as a whole is tested with the same xpcshell test that was introduced in bug 1271775.
[Risks and why]: no risk. The only change this change makes is to the histogram name and kind, so that the telemetry we collect (only if the feature is preffed on) is easier to understand. This code will not run if the pref is off.
[String/UUID change made/needed]: no.
Attachment #8764510 - Flags: approval-mozilla-beta?
Comment on attachment 8764510 [details]
Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

More telemetry changes for automigration.
Attachment #8764510 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.