[mozversion] Update mozversion to use structured logging

RESOLVED FIXED in mozilla35

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: davehunt, Assigned: davehunt)

Tracking

Trunk
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Let's update mozversion to use structured logging.
Created attachment 8487133 [details] [diff] [review]
Update mozversion to use structured logging. v1.0
Attachment #8487133 - Flags: review?(james)
Comment on attachment 8487133 [details] [diff] [review]
Update mozversion to use structured logging. v1.0

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

This isn't quite using mozlog in the idiomatic way.

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +40,3 @@
>  
>      def __init__(self):
> +        self.logger = StructuredLogger('mozversion',

You don't want to do this.

You want to use get_default_logger() and fall back to stdlib logging if it returns None.

@@ +266,5 @@
>                                     device_serial=device_serial)
> +
> +    for (key, value) in sorted(version._info.items()):
> +        if value:
> +            version.logger.info('%s: %s' % (key, value))

Why are we destructuring this stuff to log it? Do we want a new action for this data?

@@ +283,4 @@
>      parser.add_option('--device',
>                        help='serial identifier of device to target (Firefox OS '
>                             'only)')
>      (options, args) = parser.parse_args(args)

This should add the logging command line options, and set up the logger using those options (preferably using argparse).
Attachment #8487133 - Flags: review?(james) → review-
Created attachment 8488768 [details] [diff] [review]
Update mozversion to use structured logging. v2.0

(In reply to James Graham [:jgraham] from comment #2)
> ::: testing/mozbase/mozversion/mozversion/mozversion.py
> @@ +40,3 @@
> >  
> >      def __init__(self):
> > +        self.logger = StructuredLogger('mozversion',
> 
> You don't want to do this.
> 
> You want to use get_default_logger() and fall back to stdlib logging if it
> returns None.

Thanks. I knew this didn't feel right. :)

> @@ +266,5 @@
> >                                     device_serial=device_serial)
> > +
> > +    for (key, value) in sorted(version._info.items()):
> > +        if value:
> > +            version.logger.info('%s: %s' % (key, value))
> 
> Why are we destructuring this stuff to log it? Do we want a new action for
> this data?

For presentation. I'm not sure what you mean by a new action? The main purpose for the command line usage is to print the version details. For some reason I wasn't using the logging for this previously.

> @@ +283,4 @@
> >      parser.add_option('--device',
> >                        help='serial identifier of device to target (Firefox OS '
> >                             'only)')
> >      (options, args) = parser.parse_args(args)
> 
> This should add the logging command line options, and set up the logger
> using those options (preferably using argparse).

The reason I previously avoided this was bug that certain log formats are not appropriate for mozversion. I've raised this as bug 1066643.

Thanks for the review, the output is looking much nicer now - especially when used alongside other packages using structured logging.
Attachment #8487133 - Attachment is obsolete: true
Attachment #8488768 - Flags: review?(james)
(In reply to Dave Hunt (:davehunt) from comment #3)
> Created attachment 8488768 [details] [diff] [review]
> Update mozversion to use structured logging. v2.0
> 
> (In reply to James Graham [:jgraham] from comment #2)
> > ::: testing/mozbase/mozversion/mozversion/mozversion.py
> > @@ +40,3 @@
> > >  
> > >      def __init__(self):
> > > +        self.logger = StructuredLogger('mozversion',
> > 
> > You don't want to do this.
> > 
> > You want to use get_default_logger() and fall back to stdlib logging if it
> > returns None.
> 
> Thanks. I knew this didn't feel right. :)
> 
> > @@ +266,5 @@
> > >                                     device_serial=device_serial)
> > > +
> > > +    for (key, value) in sorted(version._info.items()):
> > > +        if value:
> > > +            version.logger.info('%s: %s' % (key, value))
> > 
> > Why are we destructuring this stuff to log it? Do we want a new action for
> > this data?
> 
> For presentation. I'm not sure what you mean by a new action? The main
> purpose for the command line usage is to print the version details. For some
> reason I wasn't using the logging for this previously.

So the question is whether the version data is something that log consumers might care about. For example if you use mozversion from a testsuite, should it be possible to include the version data as part of a report? It seems to me that it should, in which case we should add this to mozlog as a new datatype (i.e. a new "action") rather than logging it as a string.
Comment on attachment 8488768 [details] [diff] [review]
Update mozversion to use structured logging. v2.0

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

Can you  file a followup on adding the version information as a specific structured logging action? I think this should be machine-parsable information in the logs rather than just human-readable.

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +279,5 @@
> +    parser = argparse.ArgumentParser(
> +        description='Display version information for Mozilla applications')
> +    parser.add_argument(
> +        '--binary',
> +        dest='binary',

No need for dest if it matches the argument name.
Attachment #8488768 - Flags: review?(james) → review+
Created attachment 8489409 [details] [diff] [review]
Update mozversion to use structured logging. v2.1

Carrying r+
Attachment #8488768 - Attachment is obsolete: true
Attachment #8489409 - Flags: review+
Keywords: checkin-needed
Followup raised as bug 1067423.
https://hg.mozilla.org/mozilla-central/rev/e060088b31df
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.