Add telemetry for SSL Error Reports

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

unspecified
mozilla41
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
It  would be useful to have telemetry on:
1) about:netError being seen for pin violations
2) users choosing to submit
3) users choosing to send automatically
4) errors encountered during send
(Assignee)

Comment 1

3 years ago
Created attachment 8609315 [details] [diff] [review]
telemetry_for_error_report.patch

This adds telemetry for the TLS error reporting code.

NEEDINFOing bsmedberg as per https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe since this adds a telemetry probe - Given https://bugzilla.mozilla.org/show_bug.cgi?id=846489#c74, I assume this will be fine.
Flags: needinfo?(benjamin)
Attachment #8609315 - Flags: review?(felipc)

Updated

3 years ago
Flags: needinfo?(benjamin)
Attachment #8609315 - Flags: feedback+
(Assignee)

Comment 2

3 years ago
Are you a good reviewer for this sort of thing or should I bug someone else?
Flags: needinfo?(felipc)
Comment on attachment 8609315 [details] [diff] [review]
telemetry_for_error_report.patch

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

The names around the UI_SHOWN and expanded could be improved a bit. The standard pageload triggers the TLS_ERROR_REPORT_TELEMETRY_UI_SHOWN metric, while there's an event called AboutNetErrorUIShown that triggers the TLS_ERROR_REPORT_TELEMETRY_UI_EXPANDED metric. Confusing..
I think the simplest would be to change your event name to AboutNetErrorUIExpanded.

::: browser/base/content/aboutNetError.xhtml
@@ +112,5 @@
>                  .addEventListener('click', function togglePanelVisibility() {
>            var panel = document.getElementById('certificateErrorReportingPanel');
>            toggleDisplay(panel);
> +
> +          if (panel.style.display === 'block') {

nit: our code style is to use " and ==, although I see some ' have already creeped in this file.
Attachment #8609315 - Flags: review?(felipc) → review+
Flags: needinfo?(felipc)
(Assignee)

Comment 4

3 years ago
Created attachment 8617306 [details] [diff] [review]
telemetry_for_error_report.patch

Feedback addressed, specifically:
* replaced ' with " and === with == in the changes to aboutNetError.xhtml as per coding style.
* Renamed the AboutNetErrorUIShown event to AboutNetErrorUIExpanded for greater clarity, as suggested.
Attachment #8609315 - Attachment is obsolete: true
Attachment #8617306 - Flags: review+
Attachment #8617306 - Flags: feedback+
(Assignee)

Comment 5

3 years ago
(In reply to :Felipe Gomes from comment #3)
> I see some ' have already
> creeped in this file.

Would you like me to do some cleanup in a followup bug for the other instances related to error reports?
Flags: needinfo?(felipc)
No, I don't think it's necessary.
Flags: needinfo?(felipc)
https://hg.mozilla.org/mozilla-central/rev/9925f173e5ad
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.