1186551 - [mozlog] add structured action process_start/process_exit

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Julien Pagès (:parkouss)]

As a replacement for printstatus function in automationutils.py, we want to add two structured logging actions. See bug 1091285 for discussions about this.

Comment 1

[Julien Pagès (:parkouss)]

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

[James Graham [: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/#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.

Comment 3

[Julien Pagès (:parkouss)]

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. :)

Comment 4

[Julien Pagès (:parkouss)]

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 5

[Julien Pagès (:parkouss)]

Thanks :jgraham for the previous review! I should have fixed everything with this patch.

Comment 6

[Julien Pagès (:parkouss)]

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

[James Graham [: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/#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?

Comment 8

[James Graham [:jgraham]]

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.

Comment 9

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/13851/#review12503

Comment 10

[Julien Pagès (:parkouss)]

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

[James Graham [: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!

Comment 12

[Julien Pagès (:parkouss)]

Pushed to try:

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

Comment 13

[Julien Pagès (:parkouss)]

tree is closed, flagging checkin-needed. try is green.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c70a55f8feb8

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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