Closed Bug 1442700 Opened 2 years ago Closed Last year

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

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: chutten, Assigned: akriti.v10, Mentored)

References

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(1 file, 2 obsolete files)

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
HI Chris,
     I would like to take care of this enum issue. would you assign it to me ?
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.
Assignee: nobody → phsangeetha1
Status: NEW → ASSIGNED
Unassigning for now.
Assignee: phsangeetha1 → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
Can i upload a patch for this?
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.
Attachment #8962142 - Flags: review?(chutten)
Assignee: nobody → akriti.v10
Status: NEW → ASSIGNED
Attachment #8962142 - Attachment is obsolete: true
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)
Attachment #8962636 - Flags: review?(chutten) → review-
Attachment #8962636 - Attachment is obsolete: true
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?
Attachment #8963035 - Flags: review?(chutten) → review+
Thanks Chris.
For testing this i ran the `mach test toolkit/components/telemetry/tests/..` & `mach lint commands`.
Excellent, thank you for this. Let's mark this for checkin, shall we?
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c1aa3f6a5a23
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.