1278531 - Disallow adding new "scalar" histograms to Histograms.json

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

After we have feature parity with the "flag" and "count" histogram types (including client implementation, validation & dashboarding), we should forbid adding new histograms of that type to encourage using scalars instead.

We can add an entry for the existing histograms in histogram-whitelists.json.

Comment 1

[Georg Fritzsche [:gfritzsche]]

We need to at least make scalars available in re:dash before we can do this.
t.m.o dashboarding would be great but might not be blocking (this depends on the bigger work of bug 1286868).

Comment 2

[Georg Fritzsche [:gfritzsche]]

Are there any blockers remaining for this?

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Are there any blockers remaining for this?

None that I can thing of. Right now, we have:

- Scalars on TMO
- Scalars on re:dash
- Expiration notification for them

So I think we should be covered.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> After we have feature parity with the "flag" and "count" histogram types
> (including client implementation, validation & dashboarding), we should
> forbid adding new histograms of that type to encourage using scalars instead.
> 
> We can add an entry for the existing histograms in histogram-whitelists.json.

The list of histograms to whitelist can be obtained using the "jq" utility:

> jq 'with_entries(select(.value.kind | contains("flag", "count"))) | keys' toolkit/components/telemetry/Histograms.json

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1278531 - Disallow adding new "scalar" histograms to Histograms.json.

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

Comment 6

[Alessio Placitelli [:Dexter]]

This is the error that gets printed:

> '"flag" histograms are deprecated, check "ASD_LOL_ROTFL". Use scalars instead on Desktop. See https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html Trying to do things for Android? Add "cpp_guard": "ANDROID" to your histogram definition.'

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8855778 [details]
Bug 1278531 - Disallow adding new "scalar" histograms to Histograms.json.

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8855778 [details]
Bug 1278531 - Disallow adding new "scalar" histograms to Histograms.json.

https://reviewboard.mozilla.org/r/127672/#review130424

::: toolkit/components/telemetry/histogram-whitelists.json:1862
(Diff revision 2)
>      "PAGE_FAULTS_HARD",
>      "BROWSERPROVIDER_XUL_IMPORT_BOOKMARKS",
>      "PDF_VIEWER_USED",
>      "NETWORK_DISK_CACHE_OPEN",
>      "GEOLOCATION_WIN8_SOURCE_IS_MLS"
> +    ],

Can we fix the indentation here while we add this?

::: toolkit/components/telemetry/histogram-whitelists.json:2152
(Diff revision 2)
> +    "WEB_NOTIFICATION_SENDERS",
> +    "WEB_NOTIFICATION_SHOWN",
> +    "XUL_CACHE_DISABLED",
> +    "YOUTUBE_NONREWRITABLE_EMBED_SEEN",
> +    "YOUTUBE_REWRITABLE_EMBED_SEEN"
>      ]

Nit: indentation.

::: toolkit/components/telemetry/histogram_tools.py:316
(Diff revision 2)
> +        # Disallow "flag" and "count" histograms on desktop.
> +        # Suggest to use scalars instead.

Comment on why we can't do this on Android.

::: toolkit/components/telemetry/histogram_tools.py:319
(Diff revision 2)
> +                   "components/telemetry/telemetry/collection/scalars.html")
> +
> +        # Disallow "flag" and "count" histograms on desktop.
> +        # Suggest to use scalars instead.
> +        hist_kind = definition.get("kind")
> +        androd_cpp_guard =\

`android_...`

::: toolkit/components/telemetry/histogram_tools.py:325
(Diff revision 2)
> +            raise KeyError(('"%s" histograms are deprecated, check "%s".'
> +                            ' Use scalars instead on Desktop. See %s'
> +                            ' Trying to do things for Android? Add "cpp_guard": "ANDROID"'
> +                            ' to your histogram definition.') % (hist_kind, name, DOC_URL))

No linebreaks?
"New %s histograms are not supported on Desktop, you should use scalars instead:\n
%s\n
Are you trying to add a histogram on Android?\n
Add ..."

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8855778 [details]
Bug 1278531 - Disallow adding new "scalar" histograms to Histograms.json.

https://reviewboard.mozilla.org/r/127672/#review130428

r+wc, lets talk if anything is unclear.

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8855778 [details]
Bug 1278531 - Disallow adding new "scalar" histograms to Histograms.json.

https://reviewboard.mozilla.org/r/127672/#review130424

> No linebreaks?
> "New %s histograms are not supported on Desktop, you should use scalars instead:\n
> %s\n
> Are you trying to add a histogram on Android?\n
> Add ..."

Nope, unfortunately linebreaks are not getting rendered when the exception is raised and the text looks weird with the "\n" in it. I've changed the text according to what you suggested, but I've dropped the linebreaks.

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8855778 [details]
Bug 1278531 - Disallow adding new "scalar" histograms to Histograms.json.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fd4b08368ad
Disallow adding new "scalar" histograms to Histograms.json. r=gfritzsche

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/4fd4b08368ad