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

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: spenrose, Assigned: raajitr, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=python][good next bug])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
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
Mentor: gfritzsche, chutten
Whiteboard: [lang=python]
Summary: histogram_tools.py: move whitelist.json complaint off STDOUT when used outside toolkit/ → Don't print whitelist.json from histogram_tools.py outside of client builds
Summary: Don't print whitelist.json from histogram_tools.py outside of client builds → Don't print whitelist.json warning from histogram_tools.py outside of client builds
Priority: -- → P3

Updated

2 years ago
Whiteboard: [lang=python] → [lang=python][good next bug]
(Assignee)

Comment 3

2 years ago
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
Flags: needinfo?(chutten)

Comment 4

2 years ago
mozreview-review
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.
Attachment #8871143 - Flags: review-

Updated

2 years ago
Flags: needinfo?(chutten)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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
Flags: needinfo?(chutten)

Comment 7

2 years ago
mozreview-review
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
Attachment #8871143 - Flags: review+

Updated

2 years ago
Flags: needinfo?(chutten)

Comment 8

2 years ago
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)
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 hidden (mozreview-request)
(Assignee)

Comment 11

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

Hope it works this time. 

Thanks
Flags: needinfo?(chutten)

Comment 12

2 years ago
mozreview-review
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

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66138e53fec2
Don't print whitelist.json warning from histogram_tools.py outside of client builds r=chutten

Updated

2 years ago
Assignee: nobody → raajit.raj
Flags: needinfo?(chutten)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(chutten)

Comment 15

2 years ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f1342f98acb1
Backed out changeset 66138e53fec2 for flake8 failures.

Updated

2 years ago
Flags: needinfo?(chutten)

Comment 16

2 years ago
Pushed by chutten@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db097ec51f62
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1382556
You need to log in before you can comment on or make changes to this bug.