1442700 - Add a label to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Chris H-C :chutten]

bug 1400748 added a new ErrorType "eTerminated" for XMLHttpRequests in C++[1], but we didn't mirror it in TelemetrySend's XHR_ERROR_TYPE enum or TELEMETRY_SEND_FAILURE_TYPE's labels.

This bug is to add it in. Specifically, find TelemetrySend's XHR_ERROR_TYPE and add in an entry for eTerminated and add a label to TELEMETRY_SEND_FAILURE_TYPE for eTerminated.

For bonus points, add a comment in XMLHttpRequestMainThread.h on the ErrorType enum to ask that future additions to ErrorType be mirrored in TelemetrySend, and a comment in TelemetrySend that future additions to XHR_ERROR_TYPE be mirrored in TELEMETRY_SEND_FAILURE_TYPE.

[1]: https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/dom/xhr/XMLHttpRequestMainThread.h#194

Comment 1

[sangeetha PH]

HI Chris,
     I would like to take care of this enum issue. would you assign it to me ?

Comment 2

[Chris H-C :chutten]

Certainly. Please let me know if you have questions.

Note: This isn't as easy as some of the other good first bugs we have in our component, so if you are having trouble you might find one of them a better place to start.

Comment 3

[Chris H-C :chutten]

Unassigning for now.

Comment 4

[akriti verma]

Can i upload a patch for this?

Comment 5

[akriti verma]

Attachment: Bug 1442700 : Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated ,

Review commit: https://reviewboard.mozilla.org/r/230992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230992/

Comment 6

[Chris H-C :chutten]

Comment on attachment 8962142 [details]
Bug 1442700 : Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated ,

https://reviewboard.mozilla.org/r/230992/#review236600

This is exactly what we need, with just a few things that need cleaning up.

::: dom/xhr/XMLHttpRequestMainThread.h:188
(Diff revision 1)
>      load,
>      loadend,
>      ENUM_MAX
>    };
>  
> +  // Make sure that any additions done to ErrorType enum are also mirrored in 

There's one space at the end of the line here that should be removed.

::: toolkit/components/telemetry/Histograms.json:7244
(Diff revision 1)
>      "labels": [
>        "eOK",
>        "eRequest",
>        "eUnreachable",
>        "eChannelOpen",
> +      "eTerminated",

This has to be added at the end of the list, as categorical histograms can only expand from the end.

Comment 7

[akriti verma]

Attachment: Bug 1442700 : Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated V2 ,

Review commit: https://reviewboard.mozilla.org/r/231496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231496/

Comment 8

[Chris H-C :chutten]

Comment on attachment 8962636 [details]
Bug 1442700 : Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated V2 ,

https://reviewboard.mozilla.org/r/231496/#review237186

::: toolkit/components/telemetry/Histograms.json:7248
(Diff revision 1)
>        "eChannelOpen",
>        "eRedirect",
>        "abort",
>        "timeout",
>        "eTooLate"
> +      "eTerminated"

This won't build, as it is missing a comma on the preceding line. Be sure to build and test your changes before submitting a patch.

(I forget sometimes, too)

Comment 9

[akriti verma]

Attachment: Bug 1442700 : Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated ,

Review commit: https://reviewboard.mozilla.org/r/231870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231870/

Comment 10

[Chris H-C :chutten]

Comment on attachment 8963035 [details]
Bug 1442700 : Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated ,

https://reviewboard.mozilla.org/r/231870/#review237666

This looks good, thank you! How did you test this?

Comment 11

[akriti verma]

Thanks Chris.
For testing this i ran the `mach test toolkit/components/telemetry/tests/..` & `mach lint commands`.

Comment 12

[Chris H-C :chutten]

Excellent, thank you for this. Let's mark this for checkin, shall we?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1aa3f6a5a23
Label added to TELEMETRY_SEND_FAILURE_TYPE and TelemetrySend's XHR_ERROR_TYPE for eTerminated , r=chutten

Comment 14

[Noemi Erli[:noemi_erli]]

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