1065402 - [mozlog] Provide a way to include configuration section in HTML formatter

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The HTML report generated by the Marionette test runner includes configuration details regarding the application under test. At the moment this is very specific to Firefox OS, but in order to switch to using the mozlog HTML formatter it would be important to include such a section.

The configuration for the Marionette HTML report is set here: http://hg.mozilla.org/mozilla-central/file/843332cc69af/testing/marionette/client/marionette/runner/mixins/reporting.py#l150

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Screenshot of configuration section from Marionette HTML report.

Comment 2

[Andrew Halberstadt [:ahal]]

I think what you are asking for is some kind of global metadata object stored on the logger instance that the formatters could then use as a kind of initialization message. We shouldn't implement this inside any one specific formatter though. I'm not really sure if a global metadata object is a good idea or not, I'll defer to jgraham and chmanchester there.

In case it's not, would subclassing the mozlog.HTML formatter in marionette work ok for you?

Comment 3

[Chris Manchester (limited bugmail, email directly)]

suite_start takes a run_info dict that might reasonably be overloaded for this use.

Comment 4

[James Graham [:jgraham]]

Yes, I think the right place to put this is in the suite_start message. Or, if there's something here that isn't testing-specific inside another message type (for example we could have a "b2g_config" message, if we want).

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Include environment details in HTML formatter. v1.0

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather environment details for HTML log formatter. v1.0

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather Gaia specific environment details for HTML log formatter. v1.0

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sorry for the flood of review requests! I've created separate patches for mozlog, marionette, and gaiatest. It occurs to me now that we'll need a version bump and release of mozlog after that patch lands, before moving onto the others. I'll attach a screenshot of the awesome new section this creates in the gaiatest report though. :)

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Screenshot of new report section for gaiatest

Here's a screenshot. It would be good to have links for the gaia and gecko revisions, but that can be a later enhancement (unless there are suggestions for how we might do this).

Comment 10

[James Graham [:jgraham]]

So, I think before I review this we should have a discussion about whether dumping mozversion data together with mozinfo data in the suite_start message is a good idea. My feeling is that it isn't and that we should take advantage of the structure we have to keep these things seperate.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I see what you're saying (although mozinfo is not used here, it's a combination of mozversion and device manager's getInfo). Is this potentially something we could implement as in the attached patches and then improve once we've had this discussion? There's no immediate need for this - it would be nice to move over to mozlog from the test runner's custom HTML output, but there are a couple of other blockers anyway.

Comment 12

[James Graham [:jgraham]]

Right, mozinfo not being used here is really the point. In other places mozinfo is being used to populate run_info. So we have the choice between making run_info a free-for-all that's used to dump any type of configuration-related information, or splitting it up in some way. I think that both approaches have advantages, but splitting it up should theoretically be cleaner; you won't get a situation where mozinfo (say) adds a foo key with one meaning and mozversion adds a key also called foo with a different meaning, making the semantics ambiguous.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

For the purposes of this patch we're even modifying the keys for presentation purposes. I was a little hesitant about this, so perhaps there's a better approach? I'm keen to hear your suggestions.

Comment 14

[James Graham [:jgraham]]

Yup, so I think the right thing to do is to include the information from those sources wholesale, and have the formatter change the names for presentation purposes. For example we could have optional version_info and device_info keys that would correspond to the output of mozversion.get_version() and deviceManager.getInfo, respectively.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Include environment details in HTML formatter. v2.0

Thanks for the suggestions :jgraham here's an improved patch for review.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Screenshot of new report section for gaiatest

Now with links restored!

Comment 17

[James Graham [:jgraham]]

Comment on attachment 8507955 [details] [diff] [review]
Include environment details in HTML formatter. v2.0

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

I think this is the right approach; I'll look in more detail once chmanchester has signed off on the general direction this takes the API.

::: testing/mozbase/mozlog/mozlog/structured/structuredlog.py
@@ +181,5 @@
>          return all_data
>  
>      @log_action(List("tests", Unicode),
> +                Dict("run_info", default=None, optional=True),
> +                Dict("version_info", default=None, optional=True),

Can you document these two parameters, please?

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8507955 [details] [diff] [review]
Include environment details in HTML formatter. v2.0

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

::: testing/mozbase/mozlog/mozlog/structured/formatters/html/html.py
@@ +76,5 @@
> +
> +        d = data.get("device_info", {})
> +        self.env["Device uptime"] = d.get("uptime")
> +        self.env["Device memory"] = d.get("memtotal")
> +        self.env["Device serial"] = d.get("id")

This all looks specific to running from a device. Maybe we can leave the initialization of "env" and putting values in the html here and then have a subclass that overrides suite_start to populate specific fields.

::: testing/mozbase/mozlog/mozlog/structured/structuredlog.py
@@ +182,5 @@
>  
>      @log_action(List("tests", Unicode),
> +                Dict("run_info", default=None, optional=True),
> +                Dict("version_info", default=None, optional=True),
> +                Dict("device_info", default=None, optional=True))

The question is whether run_info really should subsume device_info in a sense. If we decided it was important enough to implement, would we take a patch to make everything from device info be returned as a part of mozinfo? If so, maybe we shouldn't be making the split here.

My preference is to keep this part of the api as small as possible. I'd personally be ok with logging suite start with a list of tests and a dictionary of "stuff", even if some users need to log a dictionary of dictionaries to disambiguate keys.

::: testing/mozbase/mozlog/setup.py
@@ +4,5 @@
>  
>  from setuptools import setup, find_packages
>  
>  PACKAGE_NAME = 'mozlog'
> +PACKAGE_VERSION = '2.7'

I don't have a horse in that race, but I think we usually do version bumps in separate bugs for tracking.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Chris Manchester [:chmanchester] from comment #18)
> Comment on attachment 8507955 [details] [diff] [review]
> Include environment details in HTML formatter. v2.0
> 
> Review of attachment 8507955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozbase/mozlog/mozlog/structured/formatters/html/html.py
> @@ +76,5 @@
> > +
> > +        d = data.get("device_info", {})
> > +        self.env["Device uptime"] = d.get("uptime")
> > +        self.env["Device memory"] = d.get("memtotal")
> > +        self.env["Device serial"] = d.get("id")
> 
> This all looks specific to running from a device. Maybe we can leave the
> initialization of "env" and putting values in the html here and then have a
> subclass that overrides suite_start to populate specific fields.

This is specific to running against a device, but I'm not sure I follow your suggestion. Could you elaborate?

> ::: testing/mozbase/mozlog/mozlog/structured/structuredlog.py
> @@ +182,5 @@
> >  
> >      @log_action(List("tests", Unicode),
> > +                Dict("run_info", default=None, optional=True),
> > +                Dict("version_info", default=None, optional=True),
> > +                Dict("device_info", default=None, optional=True))
> 
> The question is whether run_info really should subsume device_info in a
> sense. If we decided it was important enough to implement, would we take a
> patch to make everything from device info be returned as a part of mozinfo?
> If so, maybe we shouldn't be making the split here.

I think this may have been discussed before. I seem to recall it being undesirable to have mozdevice as a dependency for mozinfo.

> My preference is to keep this part of the api as small as possible. I'd
> personally be ok with logging suite start with a list of tests and a
> dictionary of "stuff", even if some users need to log a dictionary of
> dictionaries to disambiguate keys.

This was similar to my first implementation. I did consider run_info being a dictionary of dictionaries too, but went with James' suggestion of adding version_info and device_info.

> ::: testing/mozbase/mozlog/setup.py
> @@ +4,5 @@
> >  
> >  from setuptools import setup, find_packages
> >  
> >  PACKAGE_NAME = 'mozlog'
> > +PACKAGE_VERSION = '2.7'
> 
> I don't have a horse in that race, but I think we usually do version bumps
> in separate bugs for tracking.

The reason for doing it here is that I change Marionette test runner code that depends on the mozinfo changes. I'm happy to split this into multiple patches once we've agreed on an approach.

James & Chris: Could you get together to discuss this and add a comment when you've decided on the way you'd like this implemented? Thanks.

Comment 20

[James Graham [:jgraham]]

(In reply to Dave Hunt (:davehunt) from comment #19)

> > ::: testing/mozbase/mozlog/mozlog/structured/structuredlog.py
> > @@ +182,5 @@
> > >  
> > >      @log_action(List("tests", Unicode),
> > > +                Dict("run_info", default=None, optional=True),
> > > +                Dict("version_info", default=None, optional=True),
> > > +                Dict("device_info", default=None, optional=True))
> > 
> > The question is whether run_info really should subsume device_info in a
> > sense. If we decided it was important enough to implement, would we take a
> > patch to make everything from device info be returned as a part of mozinfo?
> > If so, maybe we shouldn't be making the split here.
> 
> I think this may have been discussed before. I seem to recall it being
> undesirable to have mozdevice as a dependency for mozinfo.

Yeah, that doesn't sound like a great idea.
 
> > My preference is to keep this part of the api as small as possible. I'd
> > personally be ok with logging suite start with a list of tests and a
> > dictionary of "stuff", even if some users need to log a dictionary of
> > dictionaries to disambiguate keys.
> 
> This was similar to my first implementation. I did consider run_info being a
> dictionary of dictionaries too, but went with James' suggestion of adding
> version_info and device_info.

There is a backwards-compatibility constraint here. Code — at least wpt — already assumes that run_info contains the data from mozinfo rather than e.g. a dictionary of dictionaries pointing to different kinds of data. I am quite strongly opposed to making the structure of the data here depend on the producer; that violates the entire premise of structured logging which is that the format should offer guarantees to consumers so that they can process output from many different sources. It's already slightly troubling that mozinfo contains different keys in different situations, so you have to be quite careful when accessing the data.

I think that having a dictionary of dictionaries would be OK if you are prepared to expand this patch to fix the breakage that changing the format would cause. But I think that seperate top-level fields are also OK, if arguably slightly more ugly. Stuffing everything into one flat field, or making the format change depending on how much information is included is not OK.

> > ::: testing/mozbase/mozlog/setup.py
> > @@ +4,5 @@
> > >  
> > >  from setuptools import setup, find_packages
> > >  
> > >  PACKAGE_NAME = 'mozlog'
> > > +PACKAGE_VERSION = '2.7'
> > 
> > I don't have a horse in that race, but I think we usually do version bumps
> > in separate bugs for tracking.
> 
> The reason for doing it here is that I change Marionette test runner code
> that depends on the mozinfo changes. I'm happy to split this into multiple
> patches once we've agreed on an approach.

I think that wlach prefers the separate change for some reason, but I feel it adds overhead and I have never felt I got any benefit from it. I suggest asking him why he likes the separate bug.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

(In reply to Dave Hunt (:davehunt) from comment #19)
> (In reply to Chris Manchester [:chmanchester] from comment #18)
> > Comment on attachment 8507955 [details] [diff] [review]
> > Include environment details in HTML formatter. v2.0
> > 
> > Review of attachment 8507955 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: testing/mozbase/mozlog/mozlog/structured/formatters/html/html.py
> > @@ +76,5 @@
> > > +
> > > +        d = data.get("device_info", {})
> > > +        self.env["Device uptime"] = d.get("uptime")
> > > +        self.env["Device memory"] = d.get("memtotal")
> > > +        self.env["Device serial"] = d.get("id")
> > 
> > This all looks specific to running from a device. Maybe we can leave the
> > initialization of "env" and putting values in the html here and then have a
> > subclass that overrides suite_start to populate specific fields.
> 
> This is specific to running against a device, but I'm not sure I follow your
> suggestion. Could you elaborate?

My idea is that having an env class member populate a table in the html output is not specific to running on a device, so keep that here, while these fields are, so have a subclass of htmlformatter here or in gaiatest (probably in gaiatest) that implements a suite_start method to do it. As is it looks like if you're using this formatter but don't have these fields each of these will be None, then iterated over and excluded from the table based on that. It's a small number of fields, so it's a small point, but it might help keep the implementation more understandable if we want to add features to the htmlformatter down the road.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Include environment details in HTML formatter. v3.0

Ready for review. I've removed the version bump and Marionette changes, which will both be handled in separate bugs.

Comment 23

[James Graham [:jgraham]]

Comment on attachment 8508852 [details] [diff] [review]
Include environment details in HTML formatter. v3.0

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

With the line continuation characters fixed somehow.

::: testing/mozbase/mozlog/mozlog/structured/formatters/html/html.py
@@ +50,5 @@
> +        version_info = data.get("version_info")
> +        if version_info:
> +            self.env["Device identifier"] = version_info.get("device_id")
> +            self.env["Device firmware (base)"] = version_info.get("device_firmware_version_base")
> +            self.env["Device firmware (date)"] = version_info.get("device_firmware_date") and \

Please don't use line continuation characters. I think my preferred style here would be 

self.env["Device firmware (date)"] = (
     time.strftime(date_format, time.localtime(int(version_info.get("device_firmware_date")))) if
     "device_firmware_date" in version_info else None)

I'm also not sure that time.localtime makes sense, but I'm not exactly sure what the input is. It feels like the output should be in UTC or something though rather than the local time of whatever machine happened to produce the report.

@@ +55,5 @@
> +                time.strftime(date_format, time.localtime(int(version_info.get("device_firmware_date"))))
> +            self.env["Device firmware (incremental)"] = version_info.get("device_firmware_version_incremental")
> +            self.env["Device firmware (release)"] = version_info.get("device_firmware_version_release")
> +            self.env["Gaia date"] = version_info.get("gaia_date") and \
> +                time.strftime(date_format, time.localtime(int(version_info.get("gaia_date"))))

As above

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Include environment details in HTML formatter. v3.1

I've changed the report to be UTC based and improved the line continuations.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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