1626590 - histograms not needed anymore in histogram-allowlists.json should be called out more explicitly

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

In bug 1625409 tracebacks had been noticed which got printed because a histogram should have been removed from histogram-allowlists.json.

Because these messages are printed for a green build task, they are missed by people who are not explicitly checking for them. On the other hand, letting the build task fail for them but prevent any of the dependent test tasks from running.

Can these checks be moved to a test task and fail in that one?

Comment 1

[Chris H-C :chutten]

The allowlists are checked as part of the code generation step. We intend the build to fail on these because we can't be certain what code we're supposed to generate. Has it always resulted in a successful build? I could've sworn it failed for me locally when I implemented it all those years ago.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Yes, always passed for almost a day until it got detected by an out-of-tree script scanning for failure lines in tasks.

Comment 3

[Chris H-C :chutten]

Can we turn it more directly into a build failure? The generated code is not guaranteed to be correct.

We're creating a ParserError here which is printed and then exit(1)'d here... is that not enough to fail the build?

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Chris H-C :chutten from comment #3)

Can we turn it more directly into a build failure? The generated code is not guaranteed to be correct.

OK.

We're creating a ParserError here which is printed and then exit(1)'d here... is that not enough to fail the build?

Should the sys.exit(1) be a raise ParserError instead? https://hg.mozilla.org/mozilla-central/rev/92dbf32cc3526258daebc6281204224b7ca49dc4 dropped that.

Comment 5

[Chris H-C :chutten]

Raising an error doesn't seem to fail the build either. Maybe GeneratedFile doesn't fail on failures? Do you know anyone who'd know something about generated files in the build system?

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Can you answer Chris' equestion about generated files?

Comment 7

[Mike Hommey [:glandium]]

I bet this is the problem:

$ cat > foo.py <<EOF
import atexit
import sys

def foo():
    sys.exit(1)

atexit.register(foo)
EOF
$ python foo.py
$ echo $?
1
$ python3 foo.py
$ echo $?
0

GeneratedFiles have switched to python 3 by default in bug 1611326.

Comment 8

[Chris H-C :chutten]

https://bugs.python.org/issue27035

Looks like the new behaviour is being defended and there isn't a plan for a designed solution as of yet. Lemme try the _exit workaround mentioned in the comments.

Comment 9

[Chris H-C :chutten]

Attachment: Bug 1626590 Use os._exit not sys.exit in atexit in Python3 r?glandium!,janerik!

Also: os._exit is much faster than sys.exit, so remember to flush stderr or
we won't see the errors we just printed.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e83007a7ebe
Use os._exit not sys.exit in atexit in Python3 r=glandium,janerik

Comment 11

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 3e83007a7ebe (Bug 1626590) for build bustage at test_parse_scalars.py.
https://hg.mozilla.org/integration/autoland/rev/1e11d85ed0d493b1ed25e4adde8ccbabd7bcfbe2

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=3e83007a7ebe3b28c8027e9f679e01f9374f57a4&selectedJob=296823809

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296823809&repo=autoland&lineNumber=66858

Comment 12

[Chris H-C :chutten]

[task 2020-04-08T18:52:10.278Z] 18:52:10 ERROR - check> E TypeError: 'flush' is an invalid keyword argument for this function

It is?

[task 2020-04-08T18:52:10.271Z] 18:52:10 INFO - check> platform linux2 -- Python 2.7.9, pytest-3.6.2, py-1.5.4, pluggy-0.6.0 -- /builds/worker/workspace/obj-build/_virtualenvs/obj-build-1hbI4qbY-2.7/bin/python

Ohhh. I thought we were Python3 through and through? Apparently not. I guess I'll need to flush stderr itself instead of telling print to do the right thing for me. (flush is on io.IOBase in both 2 and 3)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a66e5e777cd1
Use os._exit not sys.exit in atexit in Python3 r=glandium,janerik

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

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