1014760 - [mozlog] Move structured logging out of 'structured' submodule and into the top-level one

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

The structured logging module has proven itself in a few places now. We should deprecate the old "unstructured" logger and advocate the structured one instead.

Since this will be the recommended way to use mozlog, it doesn't make sense to have it in the structured submodule. We should move it now before tons of things start depending on it.

Though if we wanted to maintain backwards compat, we could creates a class called "structured" in __init__.py that inherits from types.ModuleType and contains all the same properties the old submodule did. This way mozlog.structured would still work.. but shouldn't be used in new code.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Bug 1014760 - Move mozlog.structured into top-level modules; remove old implementation

Comment 2

[Andrew Halberstadt [:ahal]]

The above is a WIP.

I propose that we delete the old mozlog API. It hasn't received a patch in a very long time. If some out of tree consumer depends on it, they can peg their required version to what they need, and they'd receive exactly the same amount of support from us on it as they did before.

I'll still need to update in-tree consumers to not use this (either replacing it with logging or the new mozlog API).

Comment 3

[Andrew Halberstadt [:ahal]]

Eh, looking into it more, it is still going to be a PITA to update certain things from old mozlog to logging. Now I'm thinking of renaming the old API to something like mozlog.unstructured. So basically:

mozlog.structured -> mozlog
mozlog -> mozlog.unstructured

and mozlog.structured will still work for backwards compatibility.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Mozlog currently has two implementations. The top level package is based on the logging module and is deprecated. The newer structured logging implementation lives in mozlog.structured. This patch swaps the two, so mozlog contains the recommended implementation, while mozlog.unstructured contains the old one.

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Update in-tree consumers

Update in-tree consumers

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r?jgraham

Mozlog currently has two implementations. The top level package is based on the logging module and is
deprecated. The newer structured logging implementation lives in mozlog.structured. This patch swaps the
two, so the top level mozlog module contains the recommended implementation, while mozlog.unstructured
contains the old one.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

Update in-tree consumers

Comment 8

[Andrew Halberstadt [:ahal]]

Here's a reasonably green try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7695b82288f

I split updating consumers into a separate patch to make reviewing a little easier, but I'll fold them back together before landing.

Comment 9

[James Graham [:jgraham]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

https://reviewboard.mozilla.org/r/12929/#review11957

::: testing/mozbase/docs/mozlog_structured.rst:293
(Diff revision 3)
> -The `mozlog.structured.commandline` module provides integration with
> +The `mozlog.commandline` module provides integration with

If you are changing something else anyway it would be nice to rewrap this paragraph.

::: testing/mozbase/mozlog/mozlog/commandline.py:171
(Diff revision 3)
> -    can be retrieved with :py:func:`~mozlog.structured.structuredlog.get_default_logger`.
> +    can be retrieved with :py:func:`~mozlog.structuredlog.get_default_logger`.

You can also remove the .structuredlog here, I think?

::: testing/mozbase/mozlog/tests/test_logger.py:14
(Diff revision 3)
> -import mozlog
> +import mozlog.unstructured as mozlog

Did you run these tests?

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r?jgraham

Mozlog currently has two implementations. The top level package is based on the logging module and is
deprecated. The newer structured logging implementation lives in mozlog.structured. This patch swaps the
two, so the top level mozlog module contains the recommended implementation, while mozlog.unstructured
contains the old one.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

Update in-tree consumers

Comment 12

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/12929/#review11957

> Did you run these tests?

Yes, they should also have ran as part of make-check in the builds from the try run in the bug.

Comment 13

[James Graham [:jgraham]]

Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

https://reviewboard.mozilla.org/r/12995/#review11963

::: testing/remotecppunittests.py:253
(Diff revision 2)
>                                                 options,

Indentation

::: testing/runcppunittests.py:145
(Diff revision 2)
> -        self.log = structured.structuredlog.get_default_logger()
> +        self.log = mozlog.structuredlog.get_default_logger()

s/.structuredlog//

::: testing/runcppunittests.py:228
(Diff revision 2)
>                                                 options,

Indentation

::: testing/marionette/client/marionette/marionette_test.py:27
(Diff revision 2)
> -from mozlog.structured.structuredlog import get_default_logger
> +from mozlog.structuredlog import get_default_logger

s/.structuredlog//

::: testing/marionette/client/marionette/marionette_test.py:27
(Diff revision 2)
> -from mozlog.structured.structuredlog import get_default_logger
> +from mozlog.structuredlog import get_default_logger

s/.structuredlog//

::: testing/marionette/client/marionette/runner/base.py:24
(Diff revision 2)
> -from mozlog.structured.structuredlog import get_default_logger
> +from mozlog.structuredlog import get_default_logger

s/.structuredlog//

::: testing/marionette/client/marionette/runner/mixins/reporting.py:14
(Diff revision 2)
> -from mozlog.structured.structuredlog import get_default_logger
> +from mozlog.structuredlog import get_default_logger

s/.structuredlog//

::: testing/mozbase/test.py:22
(Diff revision 2)
> +import mozlog

This should go above the "from <foo> import <bar>" style imports.

::: testing/mozbase/test.py:57
(Diff revision 2)
>                                                    options,

Indentation

::: testing/mozbase/test.py:58
(Diff revision 2)
>                                                    {

Shall we clean this up to something that doesn't take three lines whilst we're here?

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:39
(Diff revision 2)
> -    structured_logger = structuredlog.get_default_logger("mozcrash")
> +    structured_logger = mozlog.structuredlog.get_default_logger("mozcrash")

s/.structuredlog//

::: testing/mozbase/mozdevice/mozdevice/devicemanagerSUT.py:38
(Diff revision 2)
> -            deviceRoot = None, logLevel = mozlog.ERROR, **kwargs):
> +            deviceRoot = None, logLevel = logging.ERROR, **kwargs):

Indentation

::: testing/mozbase/mozprofile/mozprofile/addons.py:15
(Diff revision 2)
> -import mozlog
> +import mozlog.unstructured as mozlog

I wonder if this import style is going to lead to confusion for people that don't clearly read the code?

::: testing/web-platform/harness/docs/usage.rst:157
(Diff revision 2)
> -wptrunner uses the :py:mod:`mozlog.structured` package for output. This
> +wptrunner uses the :py:mod:`mozlog` package for output. This

General comment about wptharness; mind opening a PR upstream with the changes so they don't get lost next time we update?

::: testing/xpcshell/runxpcshelltests.py:561
(Diff revision 2)
> -        """Stores or logs a json log message in mozlog.structured
> +        """Stores or logs a json log message in mozlog

re-wrap

::: testing/xpcshell/runxpcshelltests.py:1550
(Diff revision 2)
>                                                 options,

Indentation

Comment 14

[James Graham [:jgraham]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

https://reviewboard.mozilla.org/r/12929/#review11971

::: testing/mozbase/mozlog/mozlog/commandline.py:5
(Diff revisions 3 - 4)
> +from collections import defaultdict

I would always put this type of import after import <foo> imports (despite the broken example elsewhere in this file).

Comment 15

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/12995/#review11963

> This should go above the "from <foo> import <bar>" style imports.

PEP8 doesn't actually make a comment on that, but I'll do it your way :)

> General comment about wptharness; mind opening a PR upstream with the changes so they don't get lost next time we update?

https://github.com/w3c/wptrunner/pull/133

For the record nothing will break if the changes get merged back in, `import mozlog.structured` should still work the same as before.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

Mozlog currently has two implementations. The top level package is based on the logging module and is
deprecated. The newer structured logging implementation lives in mozlog.structured. This patch swaps the
two, so the top level mozlog module contains the recommended implementation, while mozlog.unstructured
contains the old one.

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

Update in-tree consumers

Comment 18

[James Graham [:jgraham]]

Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

https://reviewboard.mozilla.org/r/12995/#review12049

Ship It!

Comment 19

[James Graham [:jgraham]]

Comment on attachment 8631763 [details]
MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham

https://reviewboard.mozilla.org/r/12929/#review12051

Ship It!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2f87f734be

Comment 21

[Andrew Halberstadt [:ahal]]

Btw, here was another try run with the latest changes and debug builds included:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7ec3e9d8c5

Comment 22

[Andrew Halberstadt [:ahal]]

The inbound push looks like it'll stick, uploaded mozlog 3.0 to pypi:
https://pypi.python.org/packages/source/m/mozlog/mozlog-3.0.tar.gz

Comment 23

[Andrew Halberstadt [:ahal]]

I forgot I'll also need to bump and upload the other mozbase modules that now depend on mozlog 3.0. Hooray mozbase.

Comment 24

[Andrew Halberstadt [:ahal]]

Attachment: Version bump mozbase packages that depend on mozlog 3.0

Discussed with wlach that mozbase version bumps don't need review since they're always just automatic r+'s anyway. Here's a little try run while I wait for the trees to open:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cf21c09b0b

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c2f87f734be

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3175cac497

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f3175cac497

Comment 28

[Martijn Wargers (dead)]

Ok, so mozlog.unstructured is the way to go? And not mozlog.structured? Is there any documentation about this, by any change?

Comment 29

[James Graham [:jgraham]]

unstructured is not the way to go except in cases where you are making a minimum effort conversion of existing code that used mozlog < 3.0. For new code you should prefer mozlog[.structured] or, if that really doesn't work for some reason, the stdlib logging module.

Comment 30

[Andrew Halberstadt [:ahal]]

Mozlog.unstructured is an old implementation that we aren't really supporting anymore. It's just a really thin wrapper around python's logging module, so if you just need some kind of basic logger, I'd recommend just using logging directly.

But if you were previously using 'mozlog', it's easiest to just update your imports, e.g:
import mozlog.unstructured as mozlog

Mozlog.structured (now just 'mozlog') is the recommended way to do logging. But it can be overkill for many cases, so it might not be worth upgrading to it. The documentation for mozlog structured is here:
https://mozbase.readthedocs.org/en/latest/mozlog_structured.html

There is purposefully no documentation for mozlog.unstructured because we don't want any new consumers using it.