Closed
Bug 973292
Opened 11 years ago
Closed 10 years ago
Firefox counts the number of characters its translates
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(firefox32 verified, firefox33 verified)
VERIFIED
FIXED
Firefox 33
People
(Reporter: MarcoM, Assigned: ttaubert)
References
Details
(Whiteboard: [translation] p=5 s=33.1 [qa!])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Felipe
:
review+
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1. Goal
* As a product owner I want to know how many characters Firefox translates so that I can assess the cost of the service
2. Acceptance Criteria
* I can view a report that tells me how many characters have been translated by our users
* This report will be updated weekly
Comment 1•11 years ago
|
||
Does translation really cost in characters? Would it be sufficient to ask the service provider to provide us with these metrics? Or are we trying to proof their numbers against our own internal metrics?
Flags: needinfo?(cweiner)
Comment 2•11 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Does translation really cost in characters?
Yup, for many of the leading providers :(
Would it be sufficient to ask
> the service provider to provide us with these metrics?
Or are we trying to
> proof their numbers against our own internal metrics?
It's always safer to know ourselves, especially with the potential size of the cost involved here.
Flags: needinfo?(cweiner)
Updated•11 years ago
|
Component: Firefox Operations → Client: Desktop
Product: Tracking → Firefox Health Report
Version: --- → Trunk
Reporter | ||
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Assignee | ||
Comment 3•10 years ago
|
||
This isn't prioritized yet but bug 978158 introduces a "recordTranslation()" method which can be used to record this data. Fixing this patch will require actually getting a character count of what's being translated and then passing it to this method along with the languages. Should be quite simple to fix.
Assignee | ||
Updated•10 years ago
|
Hardware: x86_64 → All
Assignee | ||
Comment 4•10 years ago
|
||
Marco, can you please add this to the current iteration? p=5 because it needs a test.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Whiteboard: [story] [translation] → [story] [translation] p=5
Reporter | ||
Comment 5•10 years ago
|
||
Added to Iteration 32.3
Flags: needinfo?(mmucci)
Whiteboard: [story] [translation] p=5 → [story] [translation] p=5 s=it-32c-31a-30b.3 [qa?]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [story] [translation] p=5 s=it-32c-31a-30b.3 [qa?] → [translation] p=5 s=it-32c-31a-30b.3 [qa?]
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8434190 -
Flags: review?(felipc)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8434190 [details] [diff] [review]
0001-Bug-973292-Record-the-number-of-characters-that-are-.patch
Review of attachment 8434190 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/translation/TranslationContentHandler.jsm
@@ +79,5 @@
>
> + // Get the total text length of all roots.
> + let totalChars = translationDocument.roots.reduce((acc, root) => {
> + return acc + translationDocument.generateTextForItem(root).length;
> + }, 0);
I wasn't sure about this part. On the one hand we could let BingTranslation count characters and return them but I don't really like shifting that responsibility to the translation provider. OTOH generateTextForItem() might have a non-negligible cost? It didn't look too costly to me.
Comment 8•10 years ago
|
||
Yeah, I was pondering about that too. I think in the end it's better to let the provider count the chars by itself, because that would be more accurate for two reasons:
- the provider splits the translation in various requests, and if one of that request fails, we should not count those chars as translated
- the provider has its own max data limit which is independent from the TranslationDocument. So it might translate less data than what's all stored in the TranslationDocument
For those reasons, I believe the provider knows the info for the most accurate counting.
_generateNextTranslationRequest already has the value calculated, so it can store it in the object it returns, to be stored in the BingRequest. Then on _chunkCompleted we can accumulate the count if .requestSucceeded, and pass the value back through the _onFinishDeferred promise.
Updated•10 years ago
|
Attachment #8434190 -
Flags: review?(felipc)
Assignee | ||
Comment 9•10 years ago
|
||
Chad, we're not sure about the use case here. So should we:
1) Count the characters that the user *wants* to translate? And also count them if some or all of the sub-requests for a single page translation fail? After all this would allow us to estimate the costs for a future translation provider even if we should start with a bad one.
2) Count only the number of characters that were successfully translated? That would allow us keep record of our translation volume and check with what the provider wants to charge us.
What exactly is the use case here?
Flags: needinfo?(cweiner)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [translation] p=5 s=it-32c-31a-30b.3 [qa?] → [translation] p=5 s=33.1 [qa?]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [translation] p=5 s=33.1 [qa?] → [translation] p=5 s=33.1 [qa+]
Assignee | ||
Comment 11•10 years ago
|
||
I ended up implementing it a little differently because it felt unnecessary to pass counts from _generateNextTranslationRequest() to _chunkCompleted() if we can just count in the request itself and use tack it onto that as a property.
The test works locally with the bing key file you gave me.
Attachment #8434190 -
Attachment is obsolete: true
Attachment #8438550 -
Flags: review?(felipc)
Assignee | ||
Comment 12•10 years ago
|
||
Rebased.
Attachment #8438550 -
Attachment is obsolete: true
Attachment #8438550 -
Flags: review?(felipc)
Attachment #8438554 -
Flags: review?(felipc)
Comment 13•10 years ago
|
||
Comment on attachment 8438554 [details] [diff] [review]
0001-Bug-973292-Record-the-number-of-characters-that-are-.patch, v3
Review of attachment 8438554 [details] [diff] [review]:
-----------------------------------------------------------------
Great, thanks! To land the test we need to wait for bug 1022725, because once we have a key built-in, we don't want our infra to be hitting Bing's server and using the key.
So I'll add a note on bug 1022725 to land the test part of this patch there once the mock server lands.
::: browser/components/translation/BingTranslator.jsm
@@ +122,2 @@
> } else {
> + this._onFinishedDeferred.reject();
promise.reject() requires at least one param to exist
::: browser/components/translation/TranslationContentHandler.jsm
@@ +141,3 @@
> translationDocument.showTranslation();
> },
> + () => {
the named param "error" is unused but I like the way it makes it clear that this is the function handling the promise rejection
maybe the characterCount param for the success case should be called result, and be an object where you get result.characterCount
Attachment #8438554 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Addressed all feedback and landed the test with skip-if=true.
https://hg.mozilla.org/integration/fx-team/rev/40856b1a0320
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment 16•10 years ago
|
||
Comment on attachment 8438554 [details] [diff] [review]
0001-Bug-973292-Record-the-number-of-characters-that-are-.patch, v3
[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug is part of the automatic translation feature, which we want to A/B test with a subset of Aurora 32 users.
User impact if declined: The feature won't count the number of chars to translate sent to the provider
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): minimal
String or IDL/UUID changes made by this patch: none
Attachment #8438554 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
QA Contact: bogdan.maris
Comment 17•10 years ago
|
||
I tested on Windows 7 64bit, Mac OS X 10.9.3 and Ubuntu 13.04 64bit using latest Nightly and verified that "charactersTranslatedCount" works as expected. I did some exploratory around "pageTranslatedCount", "translationOpportunityCount", "pageTranslatedCountsByLanguage" and "translationOpportunityCountsByLanguage", all seem to work just fine.
Status: RESOLVED → VERIFIED
Whiteboard: [translation] p=5 s=33.1 [qa+] → [translation] p=5 s=33.1 [qa!]
Updated•10 years ago
|
Attachment #8438554 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•10 years ago
|
||
status-firefox32:
--- → fixed
Updated•10 years ago
|
status-firefox33:
--- → fixed
Comment 19•10 years ago
|
||
Marking as Verified on Firefox 33, based on comment 17.
Comment 20•10 years ago
|
||
Also verified on Windows 7 64bit, Mac OS X 10.9.3 and Ubuntu 13.04 64bit using latest Aurora.
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•