Chris Manchester (limited bugmail, email directly) Assignee
	Description • 10 years ago

Some refactoring is required to make the StructuredOutputParser a suitable drop-in replacement for the DesktopUnittestOutputParser and others, particularly implementing or establishing the obsolescence of things like tbox_print_summary and the "crashed" and "leaked" statuses.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 1 • 10 years ago

We want a tuple or list in the in-tree config, something like "structured_suites", if we find it, we use the StructuredOutputParser, if not, we proceed exactly as before.

This is probably a different bug, but I'd like to propose something in mozharness.mozilla.testing.unittest whose purpose is to de-duplicate logic in *_unittest.py scripts and perhaps others for interpreting options, feature detection, and whatever other directives from an in-tree config.

Armen, this seems really closely related to some proposals you've had. I see this as a blocker for structured logging work I'm presently engaged with. Let's put our heads together to decide how to approach this.

	Armen [:armenzg]
	Comment 2 • 10 years ago

I want jlund to be in the loop for this since he wrote the original output parser IIRC.

The second part of 3.1 [1] should help us by allow us reading more info from the in-tree configs.
I'm still working to get approval and coverage to work on it.
If you're happy to block on that it would be great.
Otherwise, you can start by hacking the current in-mozharness configs.

What do you think?

[1] https://wiki.mozilla.org/User:Armenzg/Proposals/Mozharness_changes#Move_and_extend_information_used_from_in-tree_mozharness_configs

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 3 • 10 years ago

I'll definitely flag jlund for feedback once we get to that point, thanks. I'm thinking that what I want to do here is exactly the realm of "Extend which information is read from in-tree configs", but whether we read it from in-tree or elsewhere, we're making something (the output parser to be used) configurable, where it wasn't before. I'm strongly inclined to get this from in-tree because that seems much easier than hacking existing configs to have it apply only to certain branches. In part because there are also a small number of modifications to logging in harness that really ought to land precisely when this swap is made, and in part because my mental model of how configuration coming from in tree works is much better developed than how this might work from in mozharness/buildbot. But I'm sure that this second point could be addressed with a little guidance :)

What sort of approval does this require, generally? This impacts proposed structured logging deliverables for Q4; I could certainly justify working on it.

	Armen [:armenzg]
	Comment 4 • 10 years ago

I think we're fine with reading from the tree.

I believe jlund would be OK with reading this info from the tree. I assume his approval as module owner is good.

In this bug you're focused on a subset of the info I want.
With Bug 1070041 I want to make it a more general approach.

I don't know yet when I can tackle bug 1070041 as I'm still working on mulet reftests.
I hope it will be in less than 2 weeks.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 5 • 10 years ago

Great, thank you for the info Armen. It doesn't sound critical which modification lands first as long as we're all on the same page. I think I just need to deliver an output parser based on a config and make it work independent of whether the config comes from in tree or out.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 6 • 10 years ago

This is what I'm after. This shouldn't have an observable effect without a corresponding tree config change. Adding "--log-raw=-" tells mozlog to log line-delimited json to stdout instead of tbpl style formatted strings.

On ash (turned on for marionette desktop, the failures are my addition):
https://treeherder.mozilla.org/ui/#/jobs?repo=ash&revision=140ee4ffe0f4

	Jordan Lund (:jlund)
	Comment 7 • 10 years ago

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #4)
> I think we're fine with reading from the tree.
> 
> I believe jlund would be OK with reading this info from the tree. I assume
> his approval as module owner is good.
> 
> In this bug you're focused on a subset of the info I want.
> With Bug 1070041 I want to make it a more general approach.
> 
> I don't know yet when I can tackle bug 1070041 as I'm still working on mulet
> reftests.
> I hope it will be in less than 2 weeks.

I think this a great proposal. Getting more things configurable, especially in-tree, empowers devs and increases turn around time. tests that stem from the tree should have its parsing logic live within the tree too.

will look at patch shortly.

	Jordan Lund (:jlund)
	Comment 8 • 10 years ago

Comment on attachment 8496086 [details] [diff] [review]
Determine an output parser to use based on a set of suites using structured logging derived from an in tree config

Review of attachment 8496086 [details] [diff] [review]:
-----------------------------------------------------------------

disclaimer: I have not looked too deeply at mozlog so I am not sure of the logic behind its formatters/handlers. I can look at that if this is needed but I think this is something that you folks would know better than us. As long as this is tested (are there logs from ash with this working?), I don't see any issues here.

This is less logic in mozharness and I think that is a good thing. DesktopUnittestOutputParser was large and over complicated. lvgtm :D

::: mozharness/mozilla/testing/testbase.py
@@ -67,5 @@
>       "type": "choice",
>       "choices": ['ondemand', 'true'],
>       "help": "Download and extract crash reporter symbols.",
>        }],
> -    [["--structured-output"],

I can't see this being used anywhere within buildbot infra. Is it used anywhere that we have to remove?

@@ +376,5 @@
> +        """Defines whether structured logging is in use in this configuration. This
> +        may need to be replaced with data from a different config at the resolution
> +        of bug 1070041 and related bugs.
> +        """
> +        return ('structured_suites' in self.tree_config and

where is 'structured_suites' right now? landed on all trees? Do we plan to ride the trains or release everywhere at once? What scripts in this patch don't avail of a 'tree config' yet?

I'm guessing if those parts are still in progress, I'm assuming you're hoping to avail of some of armen's work

@@ +379,5 @@
> +        """
> +        return ('structured_suites' in self.tree_config and
> +                suite_category in self.tree_config['structured_suites'])
> +
> +    def get_output_parser(self, suite_category, strict=True,

do you think we should use script=False since everytime we call this method, we overwrite this default?

@@ +381,5 @@
> +                suite_category in self.tree_config['structured_suites'])
> +
> +    def get_output_parser(self, suite_category, strict=True,
> +                          fallback_parser_class=DesktopUnittestOutputParser,
> +                          **kwargs):

I wonder if the default fallback should be OutputParser? If that's annoying because we are only going to call this method with fallback == DesktopUnittestOutputParser, maybe we should change the name of this method so it less generic. e.g. get_test_output_parser

::: scripts/android_panda.py
@@ +262,2 @@
>  
> +                parser = self.get_output_parser(

this looks like the only script we are not adding '--log-raw=-'. I'm assuming that's because it's not supported but thought I'd sanity check

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 9 • 10 years ago

Thanks for the feedback! I'll update and run through ash with this turned on for mochitests to see how we fare.

(In reply to Jordan Lund (:jlund) from comment #8)
> Comment on attachment 8496086 [details] [diff] [review]
> Determine an output parser to use based on a set of suites using structured
> logging derived from an in tree config
> 
> Review of attachment 8496086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> disclaimer: I have not looked too deeply at mozlog so I am not sure of the
> logic behind its formatters/handlers. I can look at that if this is needed
> but I think this is something that you folks would know better than us. As
> long as this is tested (are there logs from ash with this working?), I don't
> see any issues here.
> 
> This is less logic in mozharness and I think that is a good thing.
> DesktopUnittestOutputParser was large and over complicated. lvgtm :D
> 
> ::: mozharness/mozilla/testing/testbase.py
> @@ -67,5 @@
> >       "type": "choice",
> >       "choices": ['ondemand', 'true'],
> >       "help": "Download and extract crash reporter symbols.",
> >        }],
> > -    [["--structured-output"],
> 
> I can't see this being used anywhere within buildbot infra. Is it used
> anywhere that we have to remove?

It isn't.

> 
> @@ +376,5 @@
> > +        """Defines whether structured logging is in use in this configuration. This
> > +        may need to be replaced with data from a different config at the resolution
> > +        of bug 1070041 and related bugs.
> > +        """
> > +        return ('structured_suites' in self.tree_config and
> 
> where is 'structured_suites' right now? landed on all trees? Do we plan to
> ride the trains or release everywhere at once? What scripts in this patch
> don't avail of a 'tree config' yet?

My idea is to leave this key as optional for all tree configs. All the scripts in this patch mention tree_config. Landing these so far has meant landing on every tree (bug 981030, etc.).

> 
> I'm guessing if those parts are still in progress, I'm assuming you're
> hoping to avail of some of armen's work
> 
> @@ +379,5 @@
> > +        """
> > +        return ('structured_suites' in self.tree_config and
> > +                suite_category in self.tree_config['structured_suites'])
> > +
> > +    def get_output_parser(self, suite_category, strict=True,
> 
> do you think we should use script=False since everytime we call this method,
> we overwrite this default?

Yes. This may change, but reflects the needs of most harnesses right now.

> 
> @@ +381,5 @@
> > +                suite_category in self.tree_config['structured_suites'])
> > +
> > +    def get_output_parser(self, suite_category, strict=True,
> > +                          fallback_parser_class=DesktopUnittestOutputParser,
> > +                          **kwargs):
> 
> I wonder if the default fallback should be OutputParser? If that's annoying
> because we are only going to call this method with fallback ==
> DesktopUnittestOutputParser, maybe we should change the name of this method
> so it less generic. e.g. get_test_output_parser

get_test_output_parser seems appropriate.

> 
> ::: scripts/android_panda.py
> @@ +262,2 @@
> >  
> > +                parser = self.get_output_parser(
> 
> this looks like the only script we are not adding '--log-raw=-'. I'm
> assuming that's because it's not supported but thought I'd sanity check

That's my oversight.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 10 • 10 years ago

This is a lot like the prior patch, but I got rid of the idea of each script adding --log-raw=- because we can already do this through the in-tree config, and we may move to a version of this where we don't parse the raw log from stdout in the medium term.

On ash with a recent m-c as a sanity check:
https://treeherder.mozilla.org/ui/#/jobs?repo=ash&revision=68405d63b588

On ash with the structured output parser turned on for mochitest and xpcshell:
https://treeherder.mozilla.org/ui/#/jobs?repo=ash&revision=4772aa3dbb3e

There are a few things in that second push to sort out before we can turn this on, but with this patch landed I can at least push things to try and toggle things with the in-tree config to get them straightened out.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 11 • 10 years ago

Also, :ahal, feel free to treat this r? as f? if you prefer, but I wanted to get your feedback on the approach.

	Andrew Halberstadt [:ahal]
	Comment 12 • 10 years ago

Comment on attachment 8516141 [details] [diff] [review]
Determine an output parser to use based on a set of suites using structured logging derived from an in tree config

Review of attachment 8516141 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, this seems reasonable to me.

::: scripts/android_emulator_unittest.py
@@ +676,5 @@
>                      for line in output.splitlines():
>                          parser.parse_single_line(line)
>  
>                      # After parsing each line we should know what the summary for this suite should be
> +                    tbpl_status, log_level = parser.evaluate_parser(0)

I guess we no longer use the return code to determine status anymore? That's cool, but also kind of scary.. What happens if there is a non-zero return code but nothing showed up in the logs?

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 13 • 10 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #12)
> Comment on attachment 8516141 [details] [diff] [review]
> Determine an output parser to use based on a set of suites using structured
> logging derived from an in tree config
> 
> Review of attachment 8516141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, this seems reasonable to me.
> 
> ::: scripts/android_emulator_unittest.py
> @@ +676,5 @@
> >                      for line in output.splitlines():
> >                          parser.parse_single_line(line)
> >  
> >                      # After parsing each line we should know what the summary for this suite should be
> > +                    tbpl_status, log_level = parser.evaluate_parser(0)
> 
> I guess we no longer use the return code to determine status anymore? That's
> cool, but also kind of scary.. What happens if there is a non-zero return
> code but nothing showed up in the logs?

I know I'm a little late for Halloween, but the scary part is the function we're using today:

http://hg.mozilla.org/build/mozharness/file/baa1b3294d2d/mozharness/mozilla/testing/unittest.py#l159

You might notice the return_code parameter is ignored altogether. Some users of StructuredOutputParser (w-p-t and marionette) do or will be able to use a return code to contribute to job status, so we're doing a little better there but haven't fixed the underlying issue causing us to ignore return codes in the first place.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 14 • 10 years ago

https://hg.mozilla.org/build/mozharness/rev/0a2b30f409c8

	Pete Moore [:pmoore][:pete]
	Comment 15 • 10 years ago

Checked in code deployed to production