Expose XMLHttpRequest error cases to chrome JS

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: chutten, Assigned: chutten)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(3 attachments)

Assignee

Description

2 years ago
When Telemetry upload fails, we get a generic "error" message in TelemetrySend's XHR error handler[1].

The "error" comes from XMLHttpRequestMainThread's mErrorLoad which is set in four places: OnStartRequest, OnStopRequest, InitiateFetch, OnRedirectVerifyCallback.[2]

If we replace the bool with an enum and then expose it to chrome JS we can be more descriptive in reporting errors in cases like TelemetrySend's.

[1]: http://reports.telemetry.mozilla.org/post/projects/telemetry_send_failures.kp
[2]: http://searchfox.org/mozilla-central/search?q=mErrorLoad&path=
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8870958 [details]
bug 1367110 - Collect Telemetry for different TelemetrySend failures  data-r=bsmedberg

https://reviewboard.mozilla.org/r/142518/#review146242

data-r=me
Attachment #8870958 - Flags: review?(benjamin) → review+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8870958 [details]
bug 1367110 - Collect Telemetry for different TelemetrySend failures  data-r=bsmedberg

https://reviewboard.mozilla.org/r/142518/#review146336

This looks good, thanks!

Do you think it would be useful to add Telemetry test coverage (xpcshell) for this probe?
Attachment #8870958 - Flags: review?(alessio.placitelli) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8870956 [details]
Bug 1367110 - Make XHRMainThread's mErrorLoad more descriptive.

https://reviewboard.mozilla.org/r/142514/#review146348
Attachment #8870956 - Flags: review?(amarchesini) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8870957 [details]
Bug 1367110 - Expose XHRMT's ErrorCode to chrome JS.

https://reviewboard.mozilla.org/r/142516/#review146350

::: dom/webidl/XMLHttpRequest.webidl:159
(Diff revision 1)
>  
>    [ChromeOnly, Exposed=Window]
>    void setOriginAttributes(optional OriginAttributesDictionary originAttributes);
>  
> +  [ChromeOnly]
> +  readonly attribute unsigned short errorCode;

Write a comment saying:

1. this is for testing only.
2. this just works on main-thread.
3. when this will be removed.
4. file a bug for removing this in 1 cycle.
5. write the bug ID here.
Attachment #8870957 - Flags: review?(amarchesini) → review+
I don't think we want this in release, am I wrong? Can nightly be enough for the detection of this issue?
Flags: needinfo?(chutten)
Assignee

Updated

2 years ago
Priority: -- → P1
Assignee

Comment 9

2 years ago
It's for somewhat more than testing. It's for measuring the health of TelemetrySend over (hopefully) the long term.

I haven't yet filed the bug for 60 to examine whether to remove or extend the lifetime of the TELEMETRY_SEND_FAILURE_TYPE probe (and revert the errorCode changes), but when I do I can certainly include it in the comment. I expect it will prove to be sufficiently useful to warrant upgrading it to permanent.

Having this in release might be a useful addition at a later time, but for now the data will not be recorded there. (Though the attribute will be read)

I worry maybe my intentions were misunderstood when I approached you about this issue. I'm hoping to use this to identify the causes of telemetry ping send failures over time: to analyze trends and identify problems as they occur. If this is the wrong way to go about it, then I'll happily rework the patch to use a better long-term mechanism.
Flags: needinfo?(chutten) → needinfo?(amarchesini)
Assignee

Updated

2 years ago
Blocks: 1368540
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9983ac05d7d1
Make XHRMainThread's mErrorLoad more descriptive. r=baku
https://hg.mozilla.org/integration/autoland/rev/1b93ec532890
Expose XHRMT's ErrorCode to chrome JS. r=baku
https://hg.mozilla.org/integration/autoland/rev/244e7cfea731
Collect Telemetry for different TelemetrySend failures r=bsmedberg,Dexter data-r=bsmedberg
Backed out for eslint failures in TelemetrySend.jsm (must use doublequotes):

https://hg.mozilla.org/integration/autoland/rev/1941e33040491f2ba9c435cf10747e9200901083
https://hg.mozilla.org/integration/autoland/rev/e30c63dcb366d2e8f89751d3962a9cbe50e58a2e
https://hg.mozilla.org/integration/autoland/rev/83f02ddde71f96da055af41fd10c945be89dde67

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=244e7cfea7317c67c322a2b4fba6c2b2e64feb34&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=103473826&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:98:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:99:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:100:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:101:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:102:3 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/telemetry/TelemetrySend.jsm:1121:23 | Strings must use doublequote. (quotes)
Flags: needinfo?(chutten)
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d632ed767b41
Make XHRMainThread's mErrorLoad more descriptive. r=baku
https://hg.mozilla.org/integration/autoland/rev/35d360dce251
Expose XHRMT's ErrorCode to chrome JS. r=baku
https://hg.mozilla.org/integration/autoland/rev/91a308b8cbbe
Collect Telemetry for different TelemetrySend failures r=bsmedberg,Dexter data-r=bsmedberg
Assignee

Updated

2 years ago
Flags: needinfo?(chutten)
Assignee

Updated

2 years ago
Blocks: 1371312
Depends on: 1432489
Assignee

Comment 18

Last year
I will assume that all is well. We have now used these error codes to diagnose and fix issues in Telemetry.
Flags: needinfo?(amarchesini)
Depends on: 1501303
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.