Instantiating b2gpopulate instance invalidates device logging setting

RESOLVED FIXED in mozilla34

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Because the logger instances of devicemanager are shared, when b2gpopulate instantiates a new logger (with a default logging setting of error), any existing instances of devicemanager's logging level is reset to error. This makes it impossible to enable verbose logging throughout the eideticker harness, which we sometimes want to do for debugging purposes.
Created attachment 8459593 [details] [diff] [review]
Patch to allow passing in an existing instance of a logger to b2gpopulate

I suspect a better solution would be switch b2gpopulate to structured logging, or perhaps to remove the notion of "logging level" from devicemanager altogether. Need to think about it. In the mean time, this should fix the issues we're seeing.
Attachment #8459593 - Flags: review?(dave.hunt)
Comment on attachment 8459593 [details] [diff] [review]
Patch to allow passing in an existing instance of a logger to b2gpopulate

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

Couldn't we fix this in mozdevice, so that any new instances inherit the existing log level unless it's explicitly set? In the near future we should be able to let b2gpopulate use the device manager available from marionette.runner.device.dm, so we shouldn't need to instantiate a new one.

::: b2gpopulate/b2gpopulate.py
@@ +55,4 @@
>      handler.setFormatter(mozlog.MozFormatter(include_timestamp=True))
>      logger = mozlog.getLogger('B2GPopulate', handler)
>  
> +    def __init__(self, marionette, dm=None, log_level='INFO',

Shouldn't new keyword arguments be added to the end, in case they're being provided in order without their keywords?
Attachment #8459593 - Flags: review?(dave.hunt) → review-
After discussions with davehunt, I think the proper fix here is to set the logging level to None by default in mozdevice, then only set it to a new value if that's passed in.
Component: Eideticker → Mozbase
QA Contact: hskupin
Created attachment 8460792 [details] [diff] [review]
[checked-in] Update mozdevice to not reset global log level

This should prevent the log level in mozdevice being accidentally reset when using old-style logging.
Attachment #8459593 - Attachment is obsolete: true
Attachment #8460792 - Flags: review?(armenzg)

Comment 5

4 years ago
Comment on attachment 8460792 [details] [diff] [review]
[checked-in] Update mozdevice to not reset global log level

I see what you're doing here.
WFM.
Attachment #8460792 - Flags: review?(armenzg) → review+

Comment 6

4 years ago
Comment on attachment 8460792 [details] [diff] [review]
[checked-in] Update mozdevice to not reset global log level

https://hg.mozilla.org/integration/mozilla-inbound/rev/10dc968f23ea
Attachment #8460792 - Attachment description: Update mozdevice to not reset global log level → [checked-in] Update mozdevice to not reset global log level
https://hg.mozilla.org/mozilla-central/rev/10dc968f23ea
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.