1281602 - Redo automigration telemetry to use an enumerated histogram

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P2)

Description

[:Gijs (he/him)]

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

See bug 1271775 comment 15 and later.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

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

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Justin Dolske [: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 4

[Justin Dolske [:Dolske]]

Comment on attachment 8764510 [details]
Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

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

Comment 5

[Justin Dolske [:Dolske]]

(Sorry, thought I marked that r+ earlier but found it in my queue again.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e1af8a633527
rework automigration telemetry to use an enumerated histogram, r=bsmedberg,dolske

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1af8a633527

Comment 8

[:Gijs (he/him)]

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.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8764510 [details]
Bug 1281602 - rework automigration telemetry to use an enumerated histogram,

More telemetry changes for automigration.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/757e355a8efa