1341996 - DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED records values using empty keys

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Alessio Placitelli [:Dexter]]

Hey Morgan, any chance you could take a look at this? Thanks!

Comment 2

[Mark Banner (:standard8)]

Promoting to blocker, as this is continuously spamming the browser console, which makes following the console a lot harder.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

While fixing this, can the 'empty key' error message start including which histogram this is related to? It would help for debugging purposes.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1341996 - Prevent using empty keys in DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED.

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

Comment 9

[Brian Grinstead [:bgrins]]

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

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857520 [details]
Bug 1341996 - Prevent using empty keys in DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129494/diff/1-2/

Comment 11

[Alessio Placitelli [:Dexter]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e16b6746b48f
Prevent using empty keys in DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED. r=bgrins

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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