DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED records values using empty keys

RESOLVED FIXED in Firefox 55

Status

defect
P1
blocker
RESOLVED FIXED
2 years ago
Last year

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)

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
Whiteboard: [measurement:client:tracking]
Blocks: 1334469
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: nobody → alessio.placitelli
Priority: -- → P1
Whiteboard: [measurement:client:tracking] → [measurement:client]
Status: NEW → ASSIGNED
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 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!
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
https://hg.mozilla.org/mozilla-central/rev/e16b6746b48f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: needinfo?(winter2718)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.