Closed
Bug 1186551
Opened 9 years ago
Closed 9 years ago
[mozlog] add structured action process_start/process_exit
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(firefox42 fixed)
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: parkouss, Assigned: parkouss)
References
Details
Attachments
(1 file)
As a replacement for printstatus function in automationutils.py, we want to add two structured logging actions. See bug 1091285 for discussions about this.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham
Attachment #8637372 -
Flags: review?(james)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8637372 -
Flags: review?(james)
Comment 2•9 years ago
|
||
Comment on attachment 8637372 [details] MozReview Request: Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham https://reviewboard.mozilla.org/r/13851/#review12503 ::: testing/mozbase/mozlog/mozlog/formatters/process.py:18 (Diff revision 1) > + and not k.startswith("SIG_") Indentation ::: testing/mozbase/mozlog/mozlog/formatters/process.py:15 (Diff revision 1) > + _sigtbl = [None]*signal.NSIG Can we make this variable name less obscure? ::: testing/mozbase/mozlog/mozlog/formatters/process.py:31 (Diff revision 1) > + return _sigtbl[n] Seems like this table should be cached or we should return when we find the number that we're looking for. ::: testing/mozbase/mozlog/mozlog/formatters/machformatter.py:330 (Diff revision 1) > + return "%s: %s" % (data['process'], strstatus(data['exitcode'])) Shouldn't this say that the process exited or something? Also I suppose it should only output anything if the process exited with a non-zero exit code.
Assignee | ||
Comment 3•9 years ago
|
||
https://reviewboard.mozilla.org/r/13851/#review12503 > Shouldn't this say that the process exited or something? > > Also I suppose it should only output anything if the process exited with a non-zero exit code. Status exited is printed by the strstatus function - I intend to reuse that function in reftest so there is not much copypaste. I can remove the output in case status is 0, but then it is strange to output something for process started. Maybe I can remove the mach formatter entirely ? > Can we make this variable name less obscure? Right, I'll rewrite this function. I fear that - good old come coming from automationutils.py - but you're right it should not go as this into mozlog. :)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8637372 [details] MozReview Request: Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham
Attachment #8637372 -
Flags: review?(james)
Assignee | ||
Comment 5•9 years ago
|
||
Thanks :jgraham for the previous review! I should have fixed everything with this patch.
Assignee | ||
Comment 6•9 years ago
|
||
https://reviewboard.mozilla.org/r/13851/#review12503 > Indentation Just for information, previous indentation was also pep8 compatible. https://www.python.org/dev/peps/pep-0008/#indentation This is the rule "Add some extra indentation on the conditional continuation line". Well not really important, I replaced that with a blank line for visibility.
Comment 7•9 years ago
|
||
Comment on attachment 8637372 [details] MozReview Request: Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham https://reviewboard.mozilla.org/r/13851/#review12765 ::: testing/mozbase/mozlog/mozlog/formatters/process.py:15 (Diff revisions 1 - 2) > - # Signal numbers run 0 through NSIG-1; an array with NSIG members > + import signal Why did you move this import?
Attachment #8637372 -
Flags: review?(james)
Comment 8•9 years ago
|
||
https://reviewboard.mozilla.org/r/13851/#review12503 > Status exited is printed by the strstatus function - I intend to reuse that function in reftest so there is not much copypaste. > > I can remove the output in case status is 0, but then it is strange to output something for process started. > > Maybe I can remove the mach formatter entirely ? I think I would prefer having some output in the mach formatter and if we find it's too verbose we can scale it back. So I would say revert to your original code.
Assignee | ||
Updated•9 years ago
|
Attachment #8637372 -
Flags: review?(james)
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8637372 [details] MozReview Request: Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham
Comment 11•9 years ago
|
||
Comment on attachment 8637372 [details] MozReview Request: Bug 1186551 - [mozlog] add structured action process_start/process_exit. r?jgraham https://reviewboard.mozilla.org/r/13851/#review12789 Ship It!
Attachment #8637372 -
Flags: review?(james) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a56aafbbd9
Assignee | ||
Comment 13•9 years ago
|
||
tree is closed, flagging checkin-needed. try is green.
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70a55f8feb8
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c70a55f8feb8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•