Closed Bug 1436830 Opened 6 years ago Closed 6 years ago

Remove slog dependency

Categories

(Testing :: geckodriver, enhancement, P1)

Version 3
enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Rust beta is about to become stable.  The work to upgrade Rust
crates discovered a problem with slog tracked in [1].  slog is an
unnecessarily complicated dependency to geckodriver which we only
use because we need the ability to change log level at runtime.

Instead of upgrading testing/geckodriver/src/logging.rs to the new
slog syntax, we can roll our own implementation of log::Log.
Assignee: nobody → ato
Blocks: 1436105
Status: NEW → ASSIGNED
Priority: -- → P1
Severity: normal → blocker
Comment on attachment 8949541 [details]
Bug 1436830 - Roll own log::Log implementation in geckodriver.

Here’s a naïve log::Log implementation using for the log crate [1]
that we previously used slog_stdlog [2] for.  It uses a std::sync::Arc
to keep a thread-safe pointer to the heap for storing logging::LogLevel.

Because log::set_boxed_logger() only takes a Box, I have had to share
the Arc<logging::LogLevel> between a public-facing logging::Logger that
is returned from logging::init() and an internal Box<InnerLogger>.
This is the first time I use synchronisation primitives in Rust,
so I’m sure there is a better way.

The implementation seems to work and removes over 12.5k lines of code
from third_party.  Better still, it eliminates four slog dependencies
from geckodriver making the build vastly quicker.  In terms of
verbosity of implementation in geckodriver, it is about the same
number of lines, but I would humbly say it is a lot easier to understand.

What do you think?

  [1] https://crates.io/crates/log
  [2] https://crates.io/crates/slog-stdlog
Attachment #8949541 - Flags: feedback?(james)
Attachment #8949541 - Flags: feedback?(giles)
Comment on attachment 8949541 [details]
Bug 1436830 - Roll own log::Log implementation in geckodriver.

Speaking to jgraham we seem to have found a way to make this
work without std::sync::Arc, as the log crate these days exposes a
log::set_max_level() function that appears to work.  I’m reworking
the patch.
Attachment #8949541 - Flags: feedback?(james)
Attachment #8949541 - Flags: feedback?(giles)
Blocks: 1401129
Blocks: 1399441
Comment on attachment 8949541 [details]
Bug 1436830 - Roll own log::Log implementation in geckodriver.

https://reviewboard.mozilla.org/r/218890/#review225696

It's too bad you need all this boilerplate just to add two new logging levels, but I don't see an obvious way to make it simpler without reducing scope. Thanks for writing tests! Fine to land.

::: testing/geckodriver/src/logging.rs:1
(Diff revision 3)
> +//! Gecko-esque logger frontend for the [`log`] crate.

s/frontend/implementation/ to match the terminology in the `log` crate. It's not clear what's a frontend and what's a backend here. :)
Attachment #8949541 - Flags: review?(giles) → review+
Comment on attachment 8949542 [details]
Bug 1436830 - Drop slog dependency.

https://reviewboard.mozilla.org/r/218892/#review225746

I wish updating dependencies was easier. It would be good to file issues (or patches, it should straightforward) to update other `log` crate users to 0.4 so we can drop the 0.3.8 variant.
Attachment #8949542 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #10)
> Comment on attachment 8949542 [details]
> Bug 1436830 - Drop slog dependency.
> 
> https://reviewboard.mozilla.org/r/218892/#review225746
> 
> I wish updating dependencies was easier. It would be good to file issues (or
> patches, it should straightforward) to update other `log` crate users to 0.4
> so we can drop the 0.3.8 variant.

I’ve already taken care of that in https://bugzilla.mozilla.org/show_bug.cgi?id=1437570. (-:
Comment on attachment 8949541 [details]
Bug 1436830 - Roll own log::Log implementation in geckodriver.

https://reviewboard.mozilla.org/r/218890/#review225974

This seems fine. Added a few nits that should be addressed. I guess the only advantage of the previous design would be if we wanted to support multiple firefoxes hanging off a single geckodriver, but that doesn't seem like a priority (and would require many other changes).

::: testing/geckodriver/src/logging.rs:10
(Diff revision 3)
> +//! to provide a log implementation that shares many aesthetical traits with
> +//! [Log.jsm] from Gecko.
> +//!
> +//! Using the [`error!`], [`warn!`], [`info!`], [`debug!`], and
> +//! [`trace!`] macros from `log` will output a timestamp field, followed by the
> +//! log level, and then the message.  The fields are separated by a tabulator

I think literally no one else ever uses the term "tabulator". The technical name is "CHARACTER TABULATION; horizonatal tabulattion (HT), tab", so let's go with "tab" in the comment.

::: testing/geckodriver/src/logging.rs:17
(Diff revision 3)
> +//! `awk(1)`.
> +//!
> +//! This module shares the same API as `log`, except it provides additional
> +//! entry functions [`init`] and [`init_with_level`] and additional log levels
> +//! `Level::Fatal` and `Level::Config`.  Converting these into the
> +//! [`log::Level`] is reductionistic (lossy) so that `Level::Fatal` becomes

s/reductionistic (lossy)/lossy/

::: testing/geckodriver/src/logging.rs:173
(Diff revision 3)
> -        Ok(())
> +    Ok(())
> -    }
> +}
> +
> +/// Returns the current maximum log level.
> +pub fn max_level() -> Level {
> +    Level::from(MAX_LOG_LEVEL.load(Ordering::Relaxed))

I think this can just be written 
```
MAX_LOG_LEVEL.load(Ordering::Relaxed).into()
```
Attachment #8949541 - Flags: review?(james) → review+
Comment on attachment 8949542 [details]
Bug 1436830 - Drop slog dependency.

https://reviewboard.mozilla.org/r/218892/#review225986

\o/
Attachment #8949542 - Flags: review?(james) → review+
Comment on attachment 8949541 [details]
Bug 1436830 - Roll own log::Log implementation in geckodriver.

https://reviewboard.mozilla.org/r/218890/#review225974

Good point.  I think the short-term benefit is the heavier argument
here.  We would need to do substantive work to make slog also work
with multiple Firefoxen.
Blocks: 1437570
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c521b8c4a28
Roll own log::Log implementation in geckodriver. r=rillian,jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/532bca04e409
Drop slog dependency. r=rillian,jgraham
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cbd3741a4bb0
Roll own log::Log implementation in geckodriver. r=jgraham,rillian
https://hg.mozilla.org/integration/autoland/rev/7270c9ee964b
Drop slog dependency. r=jgraham,rillian
https://hg.mozilla.org/mozilla-central/rev/cbd3741a4bb0
https://hg.mozilla.org/mozilla-central/rev/7270c9ee964b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: