1718052 - Replace usage of marionette log.js by remote/shared/Log.jsm

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P2)

Description

[Julian Descottes [:jdescottes]]

We should only use shared/Log.jsm module and delete the marionette one.

The shared module should check both remote.log & marionette.log preferences and consider the log level with the highest value. For instance if remote.log.level is TRACE and marionette.log.level is lower (INFO?), then the actual log level should be TRACE.

And ultimately we should stop using the marionette.log preference completely.

Comment 1

[Julian Descottes [:jdescottes]]

The solution mentioned in the summary is going to be complicated to implement correctly with the current logger we use.

When a logger is created either for remote agent or marionette, it is setup to track a specific pref: https://searchfox.org/mozilla-central/rev/e070f79f953b9e793e48828dfe645198e8b27ee4/toolkit/modules/Log.jsm#282

This means that if we switch the preference used to drive the logger based on which on has the "highest" level, it will be impossible for users to know which preference they need to update at runtime if they want to change the log level.

Maybe this is not a huge problem if we consider that marionette.log.level should be considered as deprecated and we are fine with having a slightly unexpected behavior for users who rely on this preference. But I think it's worth mentioning and it's also worth taking some time to think about other options.

There are some alternatives:

• we create the logger with a permanent log level. We read the value of both prefs when the logger is accessed for the first time, and we directly set its level without using manageLevelFromPref. No way to update the log level dynamically, but this removes the confusion around which preference to update at runtime
• we synchronize the preferences. We create the logger to track remote.log.level, but we observe marionette.log.level, and whenever marionette.log.level is updated, we also update remote.log.level with the same value
• we internally create 2 loggers, one tracking each preference. We expose a Logger wrapper which overrides each and every log method to pick the Logger with the higher level from the 2

In the meantime, I will implement the initial solution presented in the summary, since this is what was discussed with :whimboo.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Julian Descottes [:jdescottes] from comment #1)

• we create the logger with a permanent log level. We read the value of both prefs when the logger is accessed for the first time, and we directly set its level without using manageLevelFromPref. No way to update the log level dynamically, but this removes the confusion around which preference to update at runtime

Not sure if it removes the confusion given that you still have to know which preference to set initially, but at least geckodriver helps here because it sets it on its own. For users of CDP it's not a problem given that the preference stays the same.

For geckodriver we clearly would have to update the following lines again that should remove webdriver.log.level:
https://searchfox.org/mozilla-central/rev/a2b327f9696ab94b80d9ebc7d233abfc04b36d7a/testing/geckodriver/src/browser.rs#228-232

Otherwise I do not see a reason why the preference needs to be modified at runtime. Evaluation on first access should be totally fine.

• we synchronize the preferences. We create the logger to track remote.log.level, but we observe marionette.log.level, and whenever marionette.log.level is updated, we also update remote.log.level with the same value

This adds quite a bit of complexity and not sure if that is needed for just the logging preference. Clear documentation which preference has to be used with what version of Firefox should be fine IMHO.

• we internally create 2 loggers, one tracking each preference. We expose a Logger wrapper which overrides each and every log method to pick the Logger with the higher level from the 2

I think that this is an option we should not favor due to complexity and code that will go away anyway soon.

In the meantime, I will implement the initial solution presented in the summary, since this is what was discussed with :whimboo.

This sounds like a fine approach. It might be even fine to get it landed with the 91 release so that it will be present in the next ESR too. That way we could bump the minimum version for geckodriver, and don't have to keep around the documentation for marionette.log.level.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1718052 - [marionette] Replace usage of marionette log.js by remote/shared/Log.jsm

Depends on D119071

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37c1d285a974
[marionette] Replace usage of marionette log.js by remote/shared/Log.jsm r=webdriver-reviewers,whimboo

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/37c1d285a974