Closed Bug 1012535 Opened 8 years ago Closed 8 years ago

Handle "service unavailable" errors

Categories

(Firefox :: Translation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox32 --- verified
firefox33 --- verified

People

(Reporter: Felipe, Assigned: florian)

References

Details

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

Attachments

(2 files, 1 obsolete file)

The "service unavailable" condition is tricky because if we know that the translation service being used is unavailable, we should stop detecting language to offer the translation opportunity for users.

However, if this is done, should there be a timer or a deadline to recheck if the service is back up and re-start language detection? Or turn it off until Firefox is restarted? but that has its own set of problems.

Also, the server response might inform us of unavailability, but not with enough information to understand the reason or the duration of the downtime.

We need to investigate this problem space, both from an UX and a technical point of view.
Flags: firefox-backlog+
Whiteboard: p=0 [qa?]
Assignee: nobody → mano
Status: NEW → ASSIGNED
Whiteboard: p=0 [qa?] → p=5 s=it-32c-31a-30b.3 [qa+]
Un-taking. This is not ready for work. A UX bug needs to be opened first, to figure out how to indicate that translation is disabled temporarily. I think it'd seem odd that there's no feedback at all.

And I believe that before we address any bug like this we need to finalize the providers code (as of now, Bing is hardcoded). This bug tells me the provider "object" should have some isAvailable API, for starters.
Status: ASSIGNED → NEW
QA Contact: nobody
Removed from Iteration 32.3
Whiteboard: p=5 s=it-32c-31a-30b.3 [qa+] → p=5 [qa+]
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #2)
> Un-taking. This is not ready for work. A UX bug needs to be opened first, to
> figure out how to indicate that translation is disabled temporarily. I think
> it'd seem odd that there's no feedback at all.
> 
> And I believe that before we address any bug like this we need to finalize
> the providers code (as of now, Bing is hardcoded). This bug tells me the
> provider "object" should have some isAvailable API, for starters.

We have an error translating infobar already: https://bugzilla.mozilla.org/show_bug.cgi?id=974538
I thought this was handled with this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=974538 but now I understand the difference...if the service is down completely, or if we run out of chars, we shouldn't display the bar. The problem here is how to detect if a service is down...I like the timer idea, but what's a realistic amount of time between checks? We could arbitrarily pick a time…like disable in-browser translation for 20 minutes then display it again?

Suggested flow:

- Translation infobar appears when user visits page written in a different language to their locale
- User translated page
- "Translation in progress..." infobar appears
- IF TRANSLATION DATA RECEIVED, THEN DISPLAY TRANSLATED PAGE
- IF NO DATA RECEIVED, then display "There has been an error translating this page" infobar http://cl.ly/image/123i0n2d0j0L
- User has opportunity to try again. If user tries three times with no results, we should remove the "Try again" button and replace it with a "Close" button.
- Translation infobar is no longer displayed for X amount of minutes.
- After X amount of minutes, the translation infobar will appear again for users.

Note: The translation icon should always appear on pages where page content is written in a foreign language…so a user can always click the icon to display the info bar to try and translate a page at any time.
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #2)
> Un-taking. This is not ready for work. A UX bug needs to be opened first, to
> figure out how to indicate that translation is disabled temporarily. I think
> it'd seem odd that there's no feedback at all.
> 
> And I believe that before we address any bug like this we need to finalize
> the providers code (as of now, Bing is hardcoded). This bug tells me the
> provider "object" should have some isAvailable API, for starters.

There's a lot that can be investigated in the meantime from the engineering point of view, like querying the service status using the API linked in comment 1, and also watching the response from the translation API calls (I have an API key with expired quota that can be used to test that).

I don't plan to refactor the provider code into a multi-provider story while Bing is our only provider in the foreseeable future.
Talked with Sevaan, here's the proposed UX for the "service temporarily unavailable" case:

- When service is determined to be temporarily unavailable:
  - do not disable language detection
  - on language detection, display the URL bar icon (indication that the page is in a different language), but not the infobar
  - if the user clicks the icon, display the infobar in the error state, with a message "The translation service is temporarily unavailable, try again later".
Felipe, per my discussion with Florian, we need multi-provider pseudo API at least for the purpose of testing (we don't want to use Bing when testing the translation component). It doesn't need to be a nice API, or even reusable in any meaningful way.
Taking back (but not for this iteration).
Assignee: nobody → mano
Added to Iteration 32.3
Assignee: mano → florian
Status: NEW → ASSIGNED
Whiteboard: p=5 [qa+] → p=5 s=it-32c-31a-30b.3 [qa+]
Final language for the infobar should read:

Translation in not available at the moment. Please try again later.
did you mean "Translation is* ..."?
Flags: needinfo?(sfranks)
Lol, yes. final final version:

Translation is not available at the moment. Please try again later.

Thanks!
Flags: needinfo?(sfranks)
Whiteboard: p=5 s=it-32c-31a-30b.3 [qa+] → p=5 s=33.1 [qa+]
Attached patch Patch v1 (obsolete) — Splinter Review
I don't have an expired key to test with (the 2 keys I thought were expired actually work and return translations), so I tested with a bogus client id or a bogus key.

The error returned by the server when attempting to get a token is:
{"error":"invalid_client","error_description":"ACS50012: Authentication failed.\r\nTrace ID: dfa9a67b-3e7c-4cc5-a0b2-fd536ccb6239\r\nCorrelation ID: 5b77673f-5094-4a4f-90c3-46f5abeb5a04\r\nTimestamp: 2014-06-11 12:54:52Z"}

And then I discovered that we don't handle errors at all on that code path (we just return undefined as the token).

I'm not sure if an expired key will fail when attempting to get a token, or fail when attempting to get a translation.

I'm not fully confident in my changes in BingTranslator.jsm, more specifically I'm not sure bingRequest.fireRequest being rejected is the only way to have a 'service unavailable' situation.
Attachment #8438431 - Flags: feedback?(felipc)
Attachment #8438431 - Flags: feedback?(felipc) → feedback+
Attached patch Patch v2Splinter Review
Now showing the "service unavailable" state only when the server returns an error 400 with a body containing the words "TranslateApiException" and "balance", as discussed on IRC.
Attachment #8438431 - Attachment is obsolete: true
Attachment #8439955 - Flags: review?(felipc)
Comment on attachment 8439955 [details] [diff] [review]
Patch v2

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

::: browser/components/translation/BingTranslator.jsm
@@ +103,5 @@
>     * @param   request   The BingRequest sent to the server.
>     */
>    _chunkCompleted: function(bingRequest) {
> +    if (this._parseChunkResult(bingRequest)) {
> +      // error on request

This comment "error on request" was leftover in the wrong place from another patch.. since you're touching this code can you remove it?

@@ +112,5 @@
>  
> +    this._checkIfFinished();
> +  },
> +
> +  _chunkFailed: function(aError) {

need to add javadoc comments for the new functions _chunkFailed and _chunkFinished
Attachment #8439955 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/fx-team/rev/1200d0aa0fc3
QA Contact: nobody → bogdan.maris
Summary: Investigate "service unavailable" error handling → Handle "service unavailable" errors
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8439955 [details] [diff] [review]
Patch v2

[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: Our Bing key going over-quota would show an error message with a "Try Again" button.
Testing completed (on m-c, etc.): will be verified on m-c by QA
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: the string landed before the previous central->aurora merge, and is already present in current aurora.
Attachment #8439955 - Flags: approval-mozilla-aurora?
Attachment #8439955 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8439955 [details] [diff] [review]
Patch v2

This currently doesn't apply cleanly on aurora, we need to land first:
Bug 1015527 - Back/Forward navigation shouldn't break the Translation UI
Bug 1017560 - Choosing the same language will modify the state of Show Translation button

Both are currently waiting for approval.
Verified as fixed on Windows 7 64bit, Windows 8.1 64bit, Mac OS X 10.9.3 and Ubuntu 14.04 32bit using latest Nightly. Please flip back the [qa+] keyword once the fix arrives in Aurora.
Status: RESOLVED → VERIFIED
Whiteboard: p=5 s=33.1 [qa+] → p=5 s=33.1 [qa!]
Thanks for the approvals in the other 2 bugs!

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee3af593dcb8
Whiteboard: p=5 s=33.1 [qa!] → p=5 s=33.1 [qa+]
Depends on: 1027024
I was able to confirm the fix for this issue on Aurora 32 (2014-06-19) as well, using: 
 * Windows 7 64-bit [1], 
 * Mac OS X 10.9.2 [2],
 * Windows 8.1 64-bit [3],
 * Ubuntu 12.04 32-bit [4].

[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
[2] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
[3] Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
[4] Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0
Whiteboard: p=5 s=33.1 [qa+] → p=5 s=33.1 [qa!]
You need to log in before you can comment on or make changes to this bug.