[mozlog] add structured action process_start/process_exit

RESOLVED FIXED in Firefox 42

Status

Testing
Mozbase
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

Trunk
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

2 years ago
Created 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)

Updated

2 years ago
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8637372 - Flags: review?(james)
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

2 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

2 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

2 years ago
Thanks :jgraham for the previous review! I should have fixed everything with this patch.
(Assignee)

Comment 6

2 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 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)
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.
https://reviewboard.mozilla.org/r/13851/#review12503
(Assignee)

Updated

2 years ago
Attachment #8637372 - Flags: review?(james)
(Assignee)

Comment 10

2 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 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

2 years ago
Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a56aafbbd9
(Assignee)

Comment 13

2 years ago
tree is closed, flagging checkin-needed. try is green.
Keywords: checkin-needed

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70a55f8feb8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c70a55f8feb8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.