1024757 - Translation opportunity FHR should include languages not in the supported list

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

The proper definition of the translation opportunity metric that we want to measure is any language not in the user's target language, independent of us showing the infobar (because that language is supported) or not.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Comment 2

[Marco Mucci [:MarcoM]]

Hi Felipe, I didn't receive a notification if you would be taking Bug 1024757 this iteration?

Comment 3

[Mike de Boer [:mikedeboer]]

Marco, can you include this in the current iteration?

Thanks!

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8439650 [details] [diff] [review]
Patch

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

r=me _if_ the question I asked below can be answered with 'nay'.

Thanks for this one!

::: browser/components/translation/Translation.jsm
@@ +49,2 @@
>          return;
> +      }

nit: no need for braces for this single line block.

@@ +53,3 @@
>        TranslationHealthReport.recordTranslationOpportunity(aData.detectedLanguage);
> +
> +      if (this.supportedSourceLanguages.indexOf(aData.detectedLanguage) == -1) {

Question: do want to measure this separately, or can this information be filtered easily when analyzing the data?

It's just that an additional datapoint to filter the translation-opportunities with seems easier to me than to keep a list around and filter the numbers manually.

@@ +53,5 @@
>        TranslationHealthReport.recordTranslationOpportunity(aData.detectedLanguage);
> +
> +      if (this.supportedSourceLanguages.indexOf(aData.detectedLanguage) == -1) {
> +        return;
> +      }

nit: same.

Comment 5

[Mike de Boer [:mikedeboer]]

Oh, and don't forget a nice commit message! ;)

Comment 6

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Separate list of unsupported languages

I checked your suggestion with the Metrics people and they preferred a separate list for unsupported languages as you suggested.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8442857 [details] [diff] [review]
Separate list of unsupported languages

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

Yikes, looks like my suggestion blew up this patch! But, it's for the best... sorry for the inconvenience ;)

Thanks!

::: browser/components/translation/Translation.jsm
@@ +257,5 @@
> +   *        The language of the page.
> +   */
> +  recordMissedTranslationOpportunity: function (language) {
> +    this._withProvider(provider => provider.recordMissedTranslationOpportunity(language));
> +   },

nit: indentation looks wrong here.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Np, there was a lot of copy&pasting from the other measurement :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f480ffb3e0b3

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8442857 [details] [diff] [review]
Separate list of unsupported languages

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug is part of the automatic translation feature, which we want to A/B with a subset of Aurora 32 users.
User impact if declined: Page languages that are unsupported by the translation service won't be reported through FHR
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): small
String or IDL/UUID changes made by this patch: none

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

How to verify this bug: 

- make sure telemetry is enabled
- enable language detection by enabling these two prefs:
  browser.translation.detectLanguage
  browser.translation.ui.show

Visit a page in a language not on the supported list. e.g., italian, dutch, finnish..

Verify that the FHR payload contains measurements about these languages under the "missed opportunities" counter and list.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8442857 [details] [diff] [review]
Separate list of unsupported languages

Aurora approval granted.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/606653d5d01d

Comment 16

[Andrei Vaida [:avaida]]

I was able to confirm that the "missedTranslationOpportunityCount" and "missedTranslationOpportunityCountsByLanguage" metrics are working properly on:
 * Nightly 33 (2014-06-22),
 * Aurora 32 (2014-06-22),
using Windows 7 64-bit, Windows 8.1 64-bit, Mac OS X 10.9.2 and Ubuntu 14.04 LTS 32-bit.

Comment 17

[Asaf Romano (gone)]

Follow up (on behalf of Felipe): https://hg.mozilla.org/integration/fx-team/rev/577642baa9bf

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/577642baa9bf

Comment 19

[:Felipe Gomes (needinfo for replies!)]

I pushed the follow-up comment to aurora to keep the documentation fully correct there. This is just a documentation file, NPOTB
https://hg.mozilla.org/releases/mozilla-aurora/rev/93046aa1a6e3