1041533 - Instantiating b2gpopulate instance invalidates device logging setting

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

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.

Comment 1

[William Lachance (:wlach)]

Attachment: 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.

Comment 2

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

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?

Comment 3

[William Lachance (:wlach)]

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.

Comment 4

[William Lachance (:wlach)]

Attachment: [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.

Comment 5

[Armen [:armenzg]]

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.

Comment 6

[Armen [:armenzg]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10dc968f23ea