DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED records values using empty keys

RESOLVED FIXED in Firefox 55

Status

P1
blocker
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
With bug 1334469 landed, we're disallowing using empty keys to record data in Telemetry keyed histograms.

It turns out that most of the data recorded in the DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED histogram are recorded in an empty key [1].

For some reason, |scriptError.errorMessageName| is empty [2]. We should fix the problem by using a non-empty key here and investigate why and empty key is used in the first place.

[1] - https://mzl.la/2mf8zaJ
[2] - http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/devtools/client/webconsole/webconsole.js#1533
(Assignee)

Updated

2 years ago
Whiteboard: [measurement:client:tracking]
(Assignee)

Updated

2 years ago
Blocks: 1334469
(Assignee)

Comment 1

2 years ago
Hey Morgan, any chance you could take a look at this? Thanks!
Flags: needinfo?(winter2718)
Promoting to blocker, as this is continuously spamming the browser console, which makes following the console a lot harder.
Severity: normal → blocker
Duplicate of this bug: 1341877
Blocks: 1342064
Duplicate of this bug: 1342064
I'm guessing the issue is that `errorMessageName` is only supplied for some subset of JS errors (instead of all possible script errors), so DevTools should check for a value first before logging a probe?
While fixing this, can the 'empty key' error message start including which histogram this is related to? It would help for debugging purposes.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> While fixing this, can the 'empty key' error message start including which
> histogram this is related to? It would help for debugging purposes.

This bug is only about the DevTools probe that triggered most of this errors, not the actual verification code that logged the errors.  Anyway, your suggestion makes sense to me, so I copied it to bug 1334469.
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
Priority: -- → P1
(Assignee)

Updated

2 years ago
Whiteboard: [measurement:client:tracking] → [measurement:client]
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 9

2 years ago
mozreview-review
Comment on attachment 8857520 [details]
Bug 1341996 - Prevent using empty keys in DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED.

https://reviewboard.mozilla.org/r/129494/#review132076

Looks fine to me.  Alternatively we could not log at all if there is no errorMessageName.

::: devtools/client/webconsole/webconsole.js:1538
(Diff revision 1)
>  
>      // Collect telemetry data regarding JavaScript errors
> +    const errorMessageName =
> +      scriptError.errorMessageName ? scriptError.errorMessageName : "Unknown";
>      this._telemetry.logKeyed("DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED",
> -                             scriptError.errorMessageName,
> +                             errorMessageName,

Nit: please use `scriptError.errorMessageName || "Unknown"` here instead of assigning a new variable
Attachment #8857520 - Flags: review?(bgrinstead) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8857520 [details]
Bug 1341996 - Prevent using empty keys in DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED.

https://reviewboard.mozilla.org/r/129494/#review132076

Thanks for the review! I guess that having the number of time we have no |errorMessageName| could have some value, but I don't know too much about this code :-) I'll leave things as they are for now, but if you have a strong opinion on this, we could always change that later!

Comment 12

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e16b6746b48f
Prevent using empty keys in DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED. r=bgrins

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e16b6746b48f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
status-firefox54: affected → wontfix
(Assignee)

Updated

9 months ago
Flags: needinfo?(winter2718)

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.