1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Sam Penrose]

histogram_tools.py is copied from toolkit/ [1] to our analytics toolchain [2]. In the latter, STDOUT goes directly to documents such as Jupyter notebooks that are intended to be read by analysts' clients. In that context, it is not appropriate to dump IOError diagnostics to STDOUT.

I have been told that histogram_tools.py in python_moztelemetry is intentionally written from mozilla-central. Assuming this decision remains in force, the fix will be to remove the error message from STDOUT according to whatever log stream the m-c maintainers feel are best for that message in a gecko context.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/histogram_tools.py#89
[2] https://github.com/mozilla/python_moztelemetry/blob/master/moztelemetry/histogram_tools.py#L89

Comment 1

[Georg Fritzsche [:gfritzsche]]

I think this is not the best fix.
We only care about the whitelist when doing client builds, so we don't need to print anything for data jobs.

Chris mentioned that we can do this using `strict_type_checks`.
I think this involves:
- moving the whitelist loading code [1] to a function
- only trigger loading the whitelist if `strict_type_checks` is `True` in [2]
- changing the warning printing code at [3] into raising a `ParserError`

We can test this e.g. by running the normal build:
> ./mach build toolkit/components/telemetry
with histogram-whitelists.json present (should succeed) & deleted (should fail).

To test the server-side scenario we can explicitly pass `strict_type_checks=False` at [4] and test that the build with and without histogram-whitelists.json present.

1: https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/toolkit/components/telemetry/histogram_tools.py#77
2: https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/toolkit/components/telemetry/histogram_tools.py#593
3: https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/toolkit/components/telemetry/histogram_tools.py#88-90
4: https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/toolkit/components/telemetry/gen-histogram-enum.py#49

Comment 2

[Raajit Raj(:raajitr)]

Attachment: Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

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

Comment 3

[Raajit Raj(:raajitr)]

Hi,

Not sure if its correct way or not. 
I have tried a patch with the instructions given in comment 1.
I also followed the instruction on how to test it. For me, the build is running normally whether the whitelist.json is present or not. Is it because I have implemented incorrectly or not following the instructions on how to run correctly.

Can you please explain this more.
Also, can the mentors give me their IRC rooms where I can contact them as I'm new contributor and this is literally my second bug.

Thanks

Comment 4

[Chris H-C :chutten]

Comment on attachment 8871143 [details]
Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

https://reviewboard.mozilla.org/r/142666/#review146500

Very close!

Your comment is of concern. Your local build _should_ fail if histogram-whitelists.json is not present locally, unless you pass strict_type_checks=False. Did you mean it succeeded only when you passed False?

As for IRC channels, you can find us (and others) on irc.mozilla.org's #telemetry

::: toolkit/components/telemetry/gen-histogram-enum.py:49
(Diff revision 1)
>      print(banner, file=output)
>      print(header, file=output)
>  
>      # Load the histograms.
>      try:
> -        all_histograms = list(histogram_tools.from_files(filenames))
> +        all_histograms = list(histogram_tools.from_files(filenames, strict_type_checks=True))

from_files will default to true, so you should be fine without this.

Comment 5

[Raajit Raj(:raajitr)]

Comment on attachment 8871143 [details]
Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

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

Comment 6

[Raajit Raj(:raajitr)]

Hi,

So I was testing this incorrectly now I'm getting the error message on removing the histogram json.
I have updated the patch but forgot to add `r?`. Tried it in moz review but I can't get it worked.
Anyways, needinfo - 'ing' :chutten here. 

Please let me know if any more changes are required.
Thanks

Comment 7

[Chris H-C :chutten]

Comment on attachment 8871143 [details]
Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

https://reviewboard.mozilla.org/r/142666/#review146580

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 90241b2c4f30 -d 4639f041d809: rebasing 398335:90241b2c4f30 "Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds r=chutten" (tip)
merging toolkit/components/telemetry/histogram_tools.py
warning: conflicts while merging toolkit/components/telemetry/histogram_tools.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

[Chris H-C :chutten]

Okay, so mozreview _was_ telling me the truth.

It appears as though the commit you gave to mozreview doesn't apply. It looks as though it's missing the first half of your changes, somehow. Can you get this squashed together so that it's all in one commit and can apply cleanly?

Comment 10

[Raajit Raj(:raajitr)]

Comment on attachment 8871143 [details]
Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

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

Comment 11

[Raajit Raj(:raajitr)]

Not sure what happened but I have resubmitted the patch. And again I forgot to added `r?` (facepalm).

Hope it works this time. 

Thanks

Comment 12

[Chris H-C :chutten]

Comment on attachment 8871143 [details]
Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

https://reviewboard.mozilla.org/r/142666/#review146594

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66138e53fec2
Don't print whitelist.json warning from histogram_tools.py outside of client builds r=chutten

Comment 14

[Raajit Raj(:raajitr)]

Comment on attachment 8871143 [details]
Bug 1365577 - Don't print whitelist.json warning from histogram_tools.py outside of client builds

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

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1342f98acb1
Backed out changeset 66138e53fec2 for flake8 failures.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db097ec51f62
Don't print whitelist.json warning from histogram_tools.py outside of client builds r=chutten

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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