Closed Bug 1219768 Opened 4 years ago Closed 4 years ago

Make the "alert_emails" and "bug_numbers" fields mandatory in Histograms.json

Categories

(Toolkit :: Telemetry, defect, P4)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- affected
firefox47 --- fixed

People

(Reporter: gfritzsche, Assigned: chutten, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=python])

Attachments

(3 files)

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.
No longer blocks: 1219733
Depends on: 1219733
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.
Mentor: alessio.placitelli, gfritzsche
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.
Assignee: nobody → allamsetty.anup
Status: NEW → ASSIGNED
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.
Assignee: allamsetty.anup → sakshivaid95
I think this is not a good bug to get started on working on Firefox :)
Assignee: sakshivaid95 → nobody
Status: ASSIGNED → NEW
Priority: P3 → P4
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
Summary: Make the "bug_numbers" field mandatory in Histograms.json → Make the "alert_emails" and "bug_numbers" fields mandatory in Histograms.json
Whiteboard: [measurement:client] → [measurement:client] [lang=python]
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?
Assignee: nobody → chutten
Status: NEW → ASSIGNED
(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.
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/
Attachment #8722527 - Flags: review?(gfritzsche)
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/
Attachment #8722528 - Flags: review?(gfritzsche)
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`.
Attachment #8722526 - Flags: review?(gfritzsche) → review+
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" ...
Attachment #8722527 - Flags: review?(gfritzsche) → review+
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.
Attachment #8722528 - Flags: review?(gfritzsche) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/89aa2714a7ab
https://hg.mozilla.org/mozilla-central/rev/60ab240e4631
https://hg.mozilla.org/mozilla-central/rev/2b277249b355
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.