Translation opportunity FHR should include languages not in the supported list

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

Trunk
Firefox 33
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox32 verified, firefox33 verified)

Details

(Whiteboard: p=1 s=33.1 [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Attachment #8439650 - Flags: review?(mdeboer)
Flags: firefox-backlog+
Whiteboard: p=1 [qa+]
Hi Felipe, I didn't receive a notification if you would be taking Bug 1024757 this iteration?
Flags: needinfo?(felipc)
Marco, can you include this in the current iteration?

Thanks!
Flags: needinfo?(felipc) → needinfo?(mmucci)
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.
Attachment #8439650 - Flags: review?(mdeboer) → review+
Oh, and don't forget a nice commit message! ;)
Added to Iteration 33.1
Flags: needinfo?(mmucci)
Whiteboard: p=1 [qa+] → p=1 s=33.1 [qa+]
I checked your suggestion with the Metrics people and they preferred a separate list for unsupported languages as you suggested.
Attachment #8439650 - Attachment is obsolete: true
Attachment #8442857 - Flags: review?(mdeboer)
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.
Attachment #8442857 - Flags: review?(mdeboer) → review+
Np, there was a lot of copy&pasting from the other measurement :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f480ffb3e0b3
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
Attachment #8442857 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/f480ffb3e0b3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
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.
Flags: needinfo?(florin.mezei)
QA Contact: bogdan.maris
Comment on attachment 8442857 [details] [diff] [review]
Separate list of unsupported languages

Aurora approval granted.
Attachment #8442857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Whiteboard: p=1 s=33.1 [qa+] → p=1 s=33.1 [qa!]
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
You need to log in before you can comment on or make changes to this bug.