1356223 - Support scalar telemetry probe types in Telemetry.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

I recently tried to add 3 new "count" probes to Histograms.json and got backed out because probes "kind": "count" aren't allowed anymore for Firefox Desktop!

See bug 1352115 for context.

Here's the failure: https://treeherder.mozilla.org/logviewer.html#?job_id=90949702&repo=autoland&lineNumber=1125
And the error message from there:

New "count" histograms are not supported on Desktop, you should use scalars instead: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html

So, going forward we need to use "uint" probes for these things, but we have a custom wrapper class for telemetry in DevTools, and this wrapper won't work with these new types, because the JS API to set values is different.

So, this bug is about changing Telemetry.js to support scalar types.

Note that in this bug, we should also update the documentation in http://searchfox.org/mozilla-central/source/devtools/docs/frontend/telemetry.md to mention that count kind isn't supported anymore on Desktop.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We should use Services.telemetry.scalarSet(histId, true); inside of Telemetry.js

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1356223 - Support scalar telemetry probe types in Telemetry.js

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

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8859562 [details]
Bug 1356223 - Support scalar telemetry probe types in Telemetry.js

https://reviewboard.mozilla.org/r/131546/#review134776

::: devtools/client/shared/telemetry.js:286
(Diff revision 1)
> +   *         Scalar in which the data is to be stored.
> +   * @param  value
> +   *         Value to store.
> +   */
> +  logScalar: function (scalarId, value) {
> +    if (scalarId) {

nit: Maybe invert this condition to make the function easier to read:

```
if (!scalarId) {
  return;
}

try {
  ...
```

::: devtools/client/shared/telemetry.js:298
(Diff revision 1)
> +          return;
> +        }
> +        Services.telemetry.scalarSet(scalarId, value);
> +      } catch (e) {
> +        dump(`Warning: An attempt was made to write to the ${scalarId} ` +
> +             `histogram, which is not defined in Scalars.yaml\n`);

s/histogram/scalar

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8859562 [details]
Bug 1356223 - Support scalar telemetry probe types in Telemetry.js

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91c19f2d71f3
Support scalar telemetry probe types in Telemetry.js r=pbro

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/91c19f2d71f3