In bug 1219733 we are allowing a "bug_numbers" field for Histograms.json that allows us to link histogram definitions to related bugs. We should make that field mandatory so no-one forgets to fill that out. However, that requires to backfill it first for the already existing entries in Histograms.json.

This would involve: For all entries in Histograms.json that don't have a "bug_numbers" field yet: * look at the hg history for that entry, e.g. by looking at annotate: https://hg.mozilla.org/mozilla-central/annotate/tip/toolkit/components/telemetry/Histograms.json * find out the bug number(s) from that and add it as the "bug_numbers" field Once we have done that, we can start making the "bug_numbers" field mandatory in the histogram python script here: https://dxr.mozilla.org/mozilla-central/rev/0711218a018d912036f7d3be2ae2649e213cfb85/toolkit/components/telemetry/histogram_tools.py#253 ... by raising a KeyError that states, descriptively that "bug_numbers" is required. The functionality can be tested by removing the "bug_numbers" property from one entry and doing "mach build" - this should trigger the error above.

Looking at e.g. A11Y_INSTANTIATED_FLAG, global changes like the expires_in_version change should be ignored. Instead we should look at the change that actually added the histogram entry.

I realize that part of the Histograms were originally defined in TelemetryHistograms.h: https://hg.mozilla.org/mozilla-central/annotate/63d6c36e40b8/toolkit/components/telemetry/TelemetryHistograms.h (With bug 781531 Nathan moved them to generating them with python scripts: https://hg.mozilla.org/mozilla-central/rev/b4316e1c474d) So we should: * list all the bug_numbers from the TelemetryHistograms.h history first: https://hg.mozilla.org/mozilla-central/annotate/63d6c36e40b8/toolkit/components/telemetry/TelemetryHistograms.h * then fill in the rest from the Histograms.json history: https://hg.mozilla.org/mozilla-central/annotate/tip/toolkit/components/telemetry/Histograms.json

(In reply to Georg Fritzsche [:gfritzsche] [away Dec 19 - Jan 3] from comment #3) > I realize that part of the Histograms were originally defined in > TelemetryHistograms.h: > https://hg.mozilla.org/mozilla-central/annotate/63d6c36e40b8/toolkit/ > components/telemetry/TelemetryHistograms.h > > (With bug 781531 Nathan moved them to generating them with python scripts: > https://hg.mozilla.org/mozilla-central/rev/b4316e1c474d) > > So we should: > * list all the bug_numbers from the TelemetryHistograms.h history first: > > https://hg.mozilla.org/mozilla-central/annotate/63d6c36e40b8/toolkit/ > components/telemetry/TelemetryHistograms.h > * then fill in the rest from the Histograms.json history: > > https://hg.mozilla.org/mozilla-central/annotate/tip/toolkit/components/ > telemetry/Histograms.json This can be done slowly, and lets hope no one comes and make some changes in the Histograms.json in the meanwhile. I will take this bug and will start working on it.

It would be better to wait until the bug 1201492 gets merged into the tree and then make the entries for bug_numbers, because in that bug we are removing the extended_statistics_ok fields from Histograms.json file.

I think this is not a good bug to get started on working on Firefox :)

I think the better approach here is to: (1) make alert_emails & bug_numbers required (2) make exceptions for a whitelist of existing histograms We can morph bucket-whitelist.json to a more general histogram-whitelists.json, e.g.: > { > "n_buckets": [ > // ... names of histograms excerpt of n_buckets limits > ], > "alert_emails": [ > // ... names of histograms excerpt of alert_emails limits > ], > ... We can fill up the "alert_emails" & "bug_numbers" whitelists here with a simple script that extracts the names of the histograms that don't have those fields now. This means updating how we handle n_buckets_whitelist: https://dxr.mozilla.org/mozilla-central/rev/e1cf617a1f2813b6cd66f460313a61c223406c9b/toolkit/components/telemetry/histogram_tools.py#71 Then we can update verify_attributes() to make "alert_emails" & "bug_numbers" mandatory except for the whitelisted histograms: https://dxr.mozilla.org/mozilla-central/rev/e1cf617a1f2813b6cd66f460313a61c223406c9b/toolkit/components/telemetry/histogram_tools.py#192

I heartily endorse this event or product. Since I wrote the n_buckets whitelist, I guess I can generalize it. Does this mean we're going to abandon the archaeological dig for ancient bug numbers?

(In reply to Chris H-C :chutten from comment #8) > Does this mean we're going to abandon the archaeological dig for ancient bug > numbers? Yes - if anyone wants to add this later, that seems fine, but we shouldn't block the field enforcement on this. Thanks for taking this :)

There are 2983 histograms (quite a few of them use counters not specified in Histograms.json) that do not have bug numbers. These are going to be largish whitelists.

This just means 2983 strings to load and map, right? Unless we have any build I/O issues with this etc. i don't think we need to worry about this.

Future-proofing. Review commit: https://reviewboard.mozilla.org/r/36069/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36069/

bug_numbers is now mandatory, because we really want to have some explanation about where the probe came from. We have a lot of non-bug-numbered probes (including non-Histograms.json- resident probes like the use counters) that are being "grandfathered in" via a whitelist in whitelists.json. Review commit: https://reviewboard.mozilla.org/r/36071/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36071/

alert_emails tell us who is interested in a particular histogram. If there's no one interested, then the histogram shouldn't exist. Review commit: https://reviewboard.mozilla.org/r/36073/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36073/

Comment on attachment 8722526 [details] MozReview Request: bug 1219768 - make histogram_tools' whitelist file generic r?gfritzsche https://reviewboard.mozilla.org/r/36069/#review32691 ::: toolkit/components/telemetry/histogram_tools.py:73 (Diff revision 1) > - whitelist_path = os.path.join(os.path.abspath(os.path.realpath(os.path.dirname(__file__))), 'bucket-whitelist.json') > + whitelist_path = os.path.join(os.path.abspath(os.path.realpath(os.path.dirname(__file__))), 'whitelists.json') `whitelists.json` is very generic, lets call it `histogram-whitelists.json`.

Comment on attachment 8722526 [details] MozReview Request: bug 1219768 - make histogram_tools' whitelist file generic r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36069/diff/1-2/

Comment on attachment 8722527 [details] MozReview Request: bug 1219768 - make Histograms.json's bug_numbers field mandatory r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36071/diff/1-2/

Comment on attachment 8722528 [details] MozReview Request: bug 1219768 - make alert_emails field mandatory for new histograms r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36073/diff/1-2/

Comment on attachment 8722527 [details] MozReview Request: bug 1219768 - make Histograms.json's bug_numbers field mandatory r?gfritzsche https://reviewboard.mozilla.org/r/36071/#review32705 ::: toolkit/components/telemetry/histogram_tools.py:241 (Diff revision 2) > + if whitelists is None or name in whitelists['bug_numbers']: Is there a legit scenario where the whitelist is not available? Otherwise we could just make that file mandatory and drop the repeated `whitelists = None` checks. ::: toolkit/components/telemetry/histogram_tools.py:244 (Diff revision 2) > + raise KeyError, 'New histogram %s must have a bug_numbers field.' % name Nit: ... for "%s" ...

Comment on attachment 8722528 [details] MozReview Request: bug 1219768 - make alert_emails field mandatory for new histograms r?gfritzsche https://reviewboard.mozilla.org/r/36073/#review32707 ::: toolkit/components/telemetry/histogram_tools.py:211 (Diff revision 2) > - raise KeyError, 'alert_emails must be an array if present (in Histogram %s)' % name > + raise KeyError, 'New histogram %s must have an alert_emails field.' % name Nit: ... histogram "%s" ::: toolkit/components/telemetry/histogram_tools.py:213 (Diff revision 2) > + raise KeyError, 'alert_emails must be an array (in Histogram %s)' % name Ditto.

Comment on attachment 8722526 [details] MozReview Request: bug 1219768 - make histogram_tools' whitelist file generic r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36069/diff/2-3/

Comment on attachment 8722527 [details] MozReview Request: bug 1219768 - make Histograms.json's bug_numbers field mandatory r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36071/diff/2-3/

Comment on attachment 8722528 [details] MozReview Request: bug 1219768 - make alert_emails field mandatory for new histograms r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36073/diff/2-3/

https://reviewboard.mozilla.org/r/36073/#review32707 > Ditto. I've gone ahead and just quoted them all for consistency. Hopefully that helps.

https://reviewboard.mozilla.org/r/36071/#review32705 > Is there a legit scenario where the whitelist is not available? > Otherwise we could just make that file mandatory and drop the repeated `whitelists = None` checks. There is. histogram_tools.py is used in spark clusters to provide Histogram descriptions for parsing pings. This is why you get the "assuming all histograms are acceptable" message on ad-hoc clusters when importing from moztelemetry. There may or may not be other cases where histogram_tools.py is being used outside of in-tree, but I only know of that one.

https://hg.mozilla.org/integration/fx-team/rev/89aa2714a7ab https://hg.mozilla.org/integration/fx-team/rev/60ab240e4631 https://hg.mozilla.org/integration/fx-team/rev/2b277249b355

https://hg.mozilla.org/mozilla-central/rev/89aa2714a7ab https://hg.mozilla.org/mozilla-central/rev/60ab240e4631 https://hg.mozilla.org/mozilla-central/rev/2b277249b355