1644311 - [Thunderbird Telemetry] collect account setup success rate with subtypes

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, task)

Description

[Ping Chen [:rnons] (away)]

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

It would be useful to know the subtypes of accout config source, and whether http or https was used. See https://bugzilla.mozilla.org/show_bug.cgi?id=1615987#c6

Comment 1

[Magnus Melin [:mkmelin]]

When referencing bugs, please just write "bug" and the bug number, like bug 1615987 comment 6

Comment 2

[Ping Chen [:rnons] (away)]

Attachment: 1644311.patch

Replace the ispdb key with more specific xml-from-disk, xml-from-db, xml-from-isp-http(s)-N keys.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9158784 [details] [diff] [review]
1644311.patch

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

At the same time we could set a subType "guess" here: https://searchfox.org/comm-central/rev/93db6b3c1403b18025e1ad81466979c5b4cd2772/mail/components/accountcreation/content/exchangeAutoDiscover.js#636 and if its url1, url2, or url3 we got the detection from: https://searchfox.org/comm-central/rev/93db6b3c1403b18025e1ad81466979c5b4cd2772/mail/components/accountcreation/content/exchangeAutoDiscover.js#63-69

Comment 4

[Ping Chen [:rnons] (away)]

Attachment: 1644311.patch

Added subtypes

• guess-exchange, guess-other
• exchange-url-N

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9159127 [details] [diff] [review]
1644311.patch

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

::: mail/components/accountcreation/content/accountConfig.js
@@ +54,5 @@
>    // who created the config.
>    // { one of kSource* }
>    source: null,
> +  /**
> +   * Used for telemetry purpose.

purposes.

::: mail/components/accountcreation/content/fetchConfig.js
@@ +113,3 @@
>    for (let url of urls) {
>      call = priority.addCall();
> +    call.foundMsg = i <= httpsCount ? `https-${i}` : `http-${i - httpsCount}`;

please drop the httpsCount and just check if the url starts with http or https

::: mail/components/accountcreation/content/guessConfig.js
@@ +61,5 @@
>    successCallback,
>    errorCallback,
>    resultConfig,
> +  which,
> +  source

instead of adding this parameter, can't you just set the source as needed here: https://searchfox.org/comm-central/rev/a8444d358c7abb921d81ee97d73b6f6ba26c7c8a/mail/components/accountcreation/content/exchangeAutoDiscover.js#637

::: mail/test/browser/account/browser_mailAccountSetupWizard.js
@@ +226,1 @@
>        awc.e("create_button").click();

why this change?

Comment 6

[Ping Chen [:rnons] (away)]

please drop the httpsCount and just check if the url starts with http or https

Same for the exchange?

   awc.e("create_button").click();

why this change?

I don't know what's the purpose of clicking create_button twice.

      awc.e("create_button").click();
      ...
      awc.e("create_button").click();
      awc.e("manual-edit_button").click();

https://searchfox.org/comm-central/source/mail/test/browser/account/browser_mailAccountSetupWizard.js#225-226
Without this change, the test will fail with scalars["tb.account.failed_email_account_setup"]["xml-from-db"] being 2. Don't know how it worked before.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Ping Chen (:rnons) from comment #6)

please drop the httpsCount and just check if the url starts with http or https

Same for the exchange?

Well for exchange which of the urls is what is known, so I'm not sure it's relevant there.

The second wait ("Timeout waiting for create button to be visible and active") does look pointless

Comment 8

[Ping Chen [:rnons] (away)]

Attachment: 1644311.patch

Fixed. I also changed AccountConfig.kSourceExchange to "exchange" since it's more specific to me. Let me know if "autodiscover" is better.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9159203 [details] [diff] [review]
1644311.patch

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

::: mail/components/accountcreation/content/exchangeAutoDiscover.js
@@ +635,5 @@
>        );
>      },
>      function(probedConfig) {
>        // Probing succeeded: found open protocols, yay!
> +      probedConfig.subSource = "exchange";

The ISPDB (xml) entries is a complete different mechanism from exchange autodiscover. Exchange is never a subSource for that. ISPDB xml is a Thunderbird defined format. Exchange uses it's own.

@@ +641,5 @@
>      },
>      function(e, probedConfig) {
>        // Probing didn't find any open protocols.
>        // Let's use the exchange (only) config that was listed then.
> +      config.subSource = "exchange";

This is the case that the main type was found through exchange autodiscover, but then we checked through guessing that there were open protocols available. 
So main source is exchange autodiscover, but subtype "guess"

@@ +646,5 @@
>        successCallback(config);
>      },
>      config2,
> +    "both",
> +    "exchange"

not needed right?

Comment 10

[Ping Chen [:rnons] (away)]

The ISPDB (xml) entries is a complete different mechanism from exchange autodiscover. Exchange is never a subSource for that. ISPDB xml is a Thunderbird defined format. Exchange uses it's own.

I'm a bit confused. This is not related to kSourceXML, but for kSourceGuess. Inside guessConfig, there is
resultConfig.source = AccountConfig.kSourceGuess;, so the telemetryKey will be "guess-from-exchange".

So main source is exchange autodiscover, but subtype "guess"

I don't know, since the main source was set in guessConfig, is it good that we change the main source only for telemetry purpose?

In the case of autodiscover works directly https://searchfox.org/comm-central/source/mail/components/accountcreation/content/exchangeAutoDiscover.js#595-601, the telemetryKey will be "exchange-from-urlN". After going through guessConfig, it will become "guess-from-exchange".

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9159203 [details] [diff] [review]
1644311.patch

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

::: mail/components/accountcreation/content/accountConfig.js
@@ +57,5 @@
> +  /**
> +   * Used for telemetry purposes.
> +   * - for kSourceXML, subSource can be one of {disk, db, isp-http, isp-http}.
> +   * - for kSourceExchange, subSource is urlN.
> +   * - for kSourceGuess, subSource can be one of {exchange, null}.

I guess I'd just add the -guess after, for exchange, so kSourceGuess don't have subSource

Comment 12

[Ping Chen [:rnons] (away)]

Attachment: 1644311.patch

Updated, please see if I understand correctly. Only kSourceXML and kSourceExchange have subSource, and the subSource is used as telemetry key. For other cases, source is used as telemetry key.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9159240 [details] [diff] [review]
1644311.patch

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

::: mail/components/accountcreation/content/accountConfig.js
@@ +56,5 @@
>    source: null,
> +  /**
> +   * Used for telemetry purposes.
> +   * - for kSourceXML, subSource is one of xml-from-{disk, db, isp-https, isp-http}.
> +   * - for kSourceExchange, subSource is one of exchange-from-{urlN, guess}.

Almost, I'd make it
exchange-from-{urlN} or exchange-from-{urlN}-guess

::: mail/components/accountcreation/content/exchangeAutoDiscover.js
@@ +635,5 @@
>        );
>      },
>      function(probedConfig) {
>        // Probing succeeded: found open protocols, yay!
> +      probedConfig.subSource = "exchange-from-guess";

probedConfig.subSource += "-guess", I think

@@ +641,5 @@
>      },
>      function(e, probedConfig) {
>        // Probing didn't find any open protocols.
>        // Let's use the exchange (only) config that was listed then.
> +      config.subSource = "exchange-from-guess";

This one is not a guess, it's the configuration the server listed.

::: mail/test/browser/account/browser_mailAccountSetupWizard.js
@@ +217,4 @@
>          function() {
>            return !this.disabled;
>          },
>          "Timeout waiting for create button to be visible and active",

can we remove this, since it's already checked before?

Comment 14

[Ping Chen [:rnons] (away)]

Attachment: 1644311.patch

probedConfig.subSource += "-guess", I think

Makes sense, updated.

This one is not a guess, it's the configuration the server listed.

I'm not sure about this, there are three successCallback, all of them don't count as guess? https://searchfox.org/comm-central/source/mail/components/accountcreation/content/guessConfig.js#70,117,160

I updated as you suggested anyway.

can we remove this, since it's already checked before?

Hmm, the test timed out when I removed this.

Comment 15

[Ping Chen [:rnons] (away)]

Attachment: 1644311.patch

Added a comment about why waitFor is needed.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9159574 [details] [diff] [review]
1644311.patch

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

Looks correct, thx. r=mkmelin

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/333e0623a505
Collect account setup success rate with subtypes. r=mkmelin

Comment 18

[Rob Lemley [:rjl]]

Comment on attachment 9159574 [details] [diff] [review]
1644311.patch

[Approval Request Comment]
Telemetry bug was not previously uplifted. Originally landed in 79 milestone.

Comment 19

[Rob Lemley [:rjl]]

Comment on attachment 9159574 [details] [diff] [review]
1644311.patch

[Triage Comment]
This patch is needed for full telemetry functionality on esr78. See bug 1597180 comment 50.

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/dc0366def1dd