1295093 - using flavor for structured logging

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Paul Yang [:pyang] (away)]

For structured logging we try to introduce flavor and filter them which are not supported. However flavor listing function is now in desktop_unittest.py

We can keep all flavor relative feature in desktop_unittest.py, that is to wrap func structured_output and get_test_output_parser and make them deal with flavor

or we can move _query_try_flavor to testbase.py.  This function serves as matcher and call following handlers.

Comment 1

[Paul Yang [:pyang] (away)]

Per discussion with ahal,
I'll try to wrapping structured logging functions in desktop_unittest.py with flavor.

Comment 2

[Paul Yang [:pyang] (away)]

Attachment: enable-structured-log

patch about using structured log with flavor

Comment 3

[Paul Yang [:pyang] (away)]

Attachment: enable-structured-log

Updated - remove flavor-filter in reftest because it's already enabled in m-c.

Comment 4

[Paul Yang [:pyang] (away)]

Try result looks positive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e40580b01888a94b970d8e1e6ed38187411eaa4e

Comment 5

[Paul Yang [:pyang] (away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbf4b48a80c5f5c966a6c15be630a05e0b20431d&selectedJob=26988086
Comparison test between structured_log and non strcuctured_log

Comment 6

[Paul Yang [:pyang] (away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c89f5ce76b80f7f15a0215ba1008a0733851f78
full try result

Comment 7

[Paul Yang [:pyang] (away)]

Attachment: unstructured_flavor

Hi Andrew, try result of this patch was showed above.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8790136 [details] [diff] [review]
unstructured_flavor

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

Thanks for the patch! I think the logic here is correct, but this is confusing because "unstructured_suites" gets treated like a blacklist by desktop_unittest.py, but gets treated like a whitelist by testbase.py. I had something more like this in mind:
https://pastebin.mozilla.org/8909604

The benefit of this way is that it will work with both suites that have flavors, and those that do not. So you could put it directly in testbase.py without needing to override it. The downside of putting it in testbase.py is that you'd need to update every mozharness config that uses testbase to get added to the blacklist. So, I guess if you don't want to deal with that for now, you can leave it as an overridden method in desktop_unittest.py. But if you do that, then just leave the testbase.py one alone and don't call it from desktop_unittest. You'd still need to add the other desktop_unittest.py suites, like mozbase, gtest and cppunittest though.

Let me know what you think!

::: testing/mozharness/scripts/desktop_unittest.py
@@ +479,5 @@
> +        return True
> +
> +    def get_test_output_parser(self, suite_category, flavor=None, strict=False,
> +                               **kwargs):
> +        self.info("Suite: %s, flavor: %s" % (suite_category, flavor))

I think you forgot to remove a debug statement

Comment 9

[Paul Yang [:pyang] (away)]

I think we should carry on discussion from https://bugzilla.mozilla.org/show_bug.cgi?id=1261194#c38 and earlier in the bug.
The problem is this option mixed with blacklist and whitelist which was bad practice.

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Comment on attachment 8790136 [details] [diff] [review]
> unstructured_flavor
> 
> Review of attachment 8790136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! I think the logic here is correct, but this is
> confusing because "unstructured_suites" gets treated like a blacklist by
> desktop_unittest.py, but gets treated like a whitelist by testbase.py. I had
> something more like this in mind:
> https://pastebin.mozilla.org/8909604
> 
> The benefit of this way is that it will work with both suites that have
> flavors, and those that do not. So you could put it directly in testbase.py
> without needing to override it. The downside of putting it in testbase.py is
> that you'd need to update every mozharness config that uses testbase to get
> added to the blacklist. So, I guess if you don't want to deal with that for
> now, you can leave it as an overridden method in desktop_unittest.py. But if
> you do that, then just leave the testbase.py one alone and don't call it
> from desktop_unittest. You'd still need to add the other desktop_unittest.py
> suites, like mozbase, gtest and cppunittest though.
> 

following are all suites in desktop_unittest.py
SUITE_CATEGORIES = ['gtest', 'cppunittest', 'jittest', 'mochitest', 'reftest', 'xpcshell', 'mozbase', 'mozmill']

If we return True for not listed suites then
gtest, cppunittest, jittest, mozbase, mozmill will use structured log, which was not integrated yet.

perfect condition should be
1. mochitest uses structured log, except jetpack flavor.
2. reftest uses structured log
3. any other suite uses desktop_unittest_output_parser

take out reftest we can't distinguish it from others, then we can only choose one of condition 2 & 3.


overriding method in desktop_unittest is because flavor is currently not used in any other place.

besides flavor features depend on 
"_query_try_flavor(self, category, suite)" which is in desktop_unittest.py
if we decide to move get_structure_output into testbase.py then we need to consider migrating all flavor features.


> Let me know what you think!
> 
> ::: testing/mozharness/scripts/desktop_unittest.py
> @@ +479,5 @@
> > +        return True
> > +
> > +    def get_test_output_parser(self, suite_category, flavor=None, strict=False,
> > +                               **kwargs):
> > +        self.info("Suite: %s, flavor: %s" % (suite_category, flavor))
> 
> I think you forgot to remove a debug statement

ya thanks :)

Comment 10

[Andrew Halberstadt [:ahal]]

(In reply to Paul Yang [:pyang] from comment #9)
> I think we should carry on discussion from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1261194#c38 and earlier in the
> bug.
> The problem is this option mixed with blacklist and whitelist which was bad
> practice.
> 
> (In reply to Andrew Halberstadt [:ahal] from comment #8)
> > Comment on attachment 8790136 [details] [diff] [review]
> > unstructured_flavor
> > 
> > Review of attachment 8790136 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for the patch! I think the logic here is correct, but this is
> > confusing because "unstructured_suites" gets treated like a blacklist by
> > desktop_unittest.py, but gets treated like a whitelist by testbase.py. I had
> > something more like this in mind:
> > https://pastebin.mozilla.org/8909604
> > 
> > The benefit of this way is that it will work with both suites that have
> > flavors, and those that do not. So you could put it directly in testbase.py
> > without needing to override it. The downside of putting it in testbase.py is
> > that you'd need to update every mozharness config that uses testbase to get
> > added to the blacklist. So, I guess if you don't want to deal with that for
> > now, you can leave it as an overridden method in desktop_unittest.py. But if
> > you do that, then just leave the testbase.py one alone and don't call it
> > from desktop_unittest. You'd still need to add the other desktop_unittest.py
> > suites, like mozbase, gtest and cppunittest though.
> > 
> 
> following are all suites in desktop_unittest.py
> SUITE_CATEGORIES = ['gtest', 'cppunittest', 'jittest', 'mochitest',
> 'reftest', 'xpcshell', 'mozbase', 'mozmill']
> 
> If we return True for not listed suites then
> gtest, cppunittest, jittest, mozbase, mozmill will use structured log, which
> was not integrated yet.

Yes, but that's the point. We want to make everything structured by default, the suites you mentioned would get added to the dict. To clarify, in each config the dict would look like:

{
    'gtest': [],
    'xpcshell': [],
    'mochitest': ['jetpack-addon', ...],
    'mozmill': [],
    'cppunittest': [],
    etc..
}

> 
> perfect condition should be
> 1. mochitest uses structured log, except jetpack flavor.
> 2. reftest uses structured log
> 3. any other suite uses desktop_unittest_output_parser
> 
> take out reftest we can't distinguish it from others, then we can only
> choose one of condition 2 & 3.
> 
> 
> overriding method in desktop_unittest is because flavor is currently not
> used in any other place.
> 
> besides flavor features depend on 
> "_query_try_flavor(self, category, suite)" which is in desktop_unittest.py
> if we decide to move get_structure_output into testbase.py then we need to
> consider migrating all flavor features.

I know, and it's ok if you keep it as an overridden method for now (I didn't have any issues with overriding). Though if you didn't want to override, you wouldn't need to move the _query_try_flavor method, instead you could pass in flavor to get_output_parser() as an optional argument.

My main complaint is that in testbase.py, structured_output() treats a config called 'unstructured_suites' as if it were actually called 'structured_suites'. What I'm saying to do here is just leave testbase.py alone for now so it keeps looking for 'structured_suites'. And just use 'unstructured_suites' in the desktop_unittest.py config.

Comment 11

[Paul Yang [:pyang] (away)]

Attachment: unstructured_flavor

Patch fixed.
- Not using parent fallback function for both structured_output(), get_structured_output_parser()

Comment 12

[Paul Yang [:pyang] (away)]

try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7123a7eb988b75899e3167f4a0cb9fa2953d470

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8793207 [details] [diff] [review]
unstructured_flavor

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

Thanks, this is the right idea! I think the you can't use a for loop there though, and also suggested some simplifications.

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +476,4 @@
>          of bug 1070041 and related bugs.
>          """
>          return ('structured_suites' in self.config and
> +                'suite_category' in self.config.get('structured_suites'))

We already test that 'suite_category' isn't in self.config, so there's no need to change this to get().

::: testing/mozharness/scripts/desktop_unittest.py
@@ +475,5 @@
> +        unstructured_suites = self.config.get('unstructured_suites')
> +        if not unstructured_suites:
> +            return False
> +        if unstructured_suites.get(suite_category) is None:
> +            return True

This reads better if it's:
    suite_category not in unstructured_suites

If you wanted you could then use None in the configs instead of [] to denote no flavors. Up to you though, I'm fine either way.

@@ +482,5 @@
> +        if not flavor:
> +            return True
> +        for ignored_suites in unstructured_suites.get(suite_category):
> +            if flavor in ignored_suites:
> +                return False

I don't think this should be a for loop. I think the whole block starting from line 480 can be simplified to:

    ignored_suites = unstructured_suites[suite_category]
    if not ignored_suites or flavor in ignored_suites:
        return False
    return True

This works because flavor defaults to None, and the value None should never appear in the ignored_suites list.

Comment 14

[Paul Yang [:pyang] (away)]

(In reply to Andrew Halberstadt [:ahal] from comment #13)
Thanks for feedback!  part of them I'll fix ASAP but something I still didn't understand clearly.  Comments inlined.

> Comment on attachment 8793207 [details] [diff] [review]
> unstructured_flavor
> 
> Review of attachment 8793207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this is the right idea! I think the you can't use a for loop there
> though, and also suggested some simplifications.
> 
> ::: testing/mozharness/mozharness/mozilla/testing/testbase.py
> @@ +476,4 @@
> >          of bug 1070041 and related bugs.
> >          """
> >          return ('structured_suites' in self.config and
> > +                'suite_category' in self.config.get('structured_suites'))
> 
> We already test that 'suite_category' isn't in self.config, so there's no
> need to change this to get().
> 

I guess I just copy it from tesbase; let me fix it :)


> ::: testing/mozharness/scripts/desktop_unittest.py
> @@ +475,5 @@
> > +        unstructured_suites = self.config.get('unstructured_suites')
> > +        if not unstructured_suites:
> > +            return False
> > +        if unstructured_suites.get(suite_category) is None:
> > +            return True
> 
> This reads better if it's:
>     suite_category not in unstructured_suites
> 
> If you wanted you could then use None in the configs instead of [] to denote
> no flavors. Up to you though, I'm fine either way.
> 

It's a little bit tricky, when get suite_category failed might result of no such key.
So we may test whether key exists and then do get.


> @@ +482,5 @@
> > +        if not flavor:
> > +            return True
> > +        for ignored_suites in unstructured_suites.get(suite_category):
> > +            if flavor in ignored_suites:
> > +                return False
> 
> I don't think this should be a for loop. I think the whole block starting
> from line 480 can be simplified to:
> 
>     ignored_suites = unstructured_suites[suite_category]
>     if not ignored_suites or flavor in ignored_suites:
>         return False
>     return True
> 
> This works because flavor defaults to None, and the value None should never
> appear in the ignored_suites list.

Agreed loop looks ugly. In face we pass flavor but not suite name like "jetpack_package_clipboard" in get_structured_output. Not sure if we agree to use flavor to filter since it's not specific.  However it works now for jetpack.

Comment 15

[Paul Yang [:pyang] (away)]

Attachment: unstructured_flavor

Updated based on discussion.
- simplify filtering logic.
- replace unstructured_suites by unstructured_flavors

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8797064 [details] [diff] [review]
unstructured_flavor

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

Thanks, looks great!

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +476,4 @@
>          of bug 1070041 and related bugs.
>          """
>          return ('structured_suites' in self.config and
> +                suite_category in self.config.get('structured_suites'))

This change doesn't do anything, might as well remove it.

Comment 17

[Paul Yang [:pyang] (away)]

Attachment: unstructured_flavor

- Remove redundant modification

Comment 18

[Paul Yang [:pyang] (away)]

Comment on attachment 8797356 [details] [diff] [review]
unstructured_flavor

Below is the try result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c75f8c6d271f3051c0870523853ab12b0dfb975b

Please help to check if anything missed, thanks!

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8797356 [details] [diff] [review]
unstructured_flavor

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

Thanks this looks good! I'll do a quick try run and land later.

Comment 20

[Andrew Halberstadt [:ahal]]

I was just about to land, and realized this patch already turns on mochitest structured logging, is that intentional? Shouldn't that part be landed in bug 1261194?

Comment 21

[Paul Yang [:pyang] (away)]

I think it makes sense to split into two piece.  Thanks!
Will push again and see the result.

Comment 22

[Paul Yang [:pyang] (away)]

Attachment: unstructured_flavor

Carrying forward review, removing mochitest config and not turning on in m-c.

Comment 23

[Paul Yang [:pyang] (away)]

Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7124f844d9432794a57f7d3b899e39c5236c132

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bcbf7313917
[mozharness] Allow excluding flavors when determining whether to use StructuredOutputParser, r=ahal

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5bcbf7313917