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

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

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.
Bug 1014760 - Move mozlog.structured into top-level modules; remove old implementation
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).
Depends on: 1098592
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.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
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.
Attachment #8631763 - Attachment description: MozReview Request: Bug 1014760 - Move mozlog.structured into top-level modules; remove old implementation → MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham
Update in-tree consumers
Attachment #8631763 - Attachment description: MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham → MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r?jgraham
Attachment #8631763 - Flags: review?(james)
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 on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

Update in-tree consumers
Attachment #8632101 - Flags: review?(james)
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 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?
Attachment #8631763 - Flags: review?(james)
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.
Attachment #8631763 - Flags: review?(james)
Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

Update in-tree consumers
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.
Attachment #8632101 - Flags: review?(james)
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 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).
Attachment #8631763 - Flags: review?(james)
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 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.
Attachment #8631763 - Attachment description: MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r?jgraham → MozReview Request: Bug 1014760 - Move mozlog.structured to mozlog; Move mozlog to mozlog.unstructured, r=jgraham
Attachment #8631763 - Flags: review?(james)
Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

Update in-tree consumers
Attachment #8632101 - Flags: review?(james)
Attachment #8632101 - Flags: review?(james) → review+
Comment on attachment 8632101 [details]
MozReview Request: Update in-tree consumers

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

Ship It!
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!
Attachment #8631763 - Flags: review?(james) → review+
Btw, here was another try run with the latest changes and debug builds included:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7ec3e9d8c5
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
I forgot I'll also need to bump and upload the other mozbase modules that now depend on mozlog 3.0. Hooray mozbase.
Whiteboard: [leave-open]
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
Attachment #8635292 - Flags: review+
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/7f3175cac497
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1185479
Depends on: 1185501
Ok, so mozlog.unstructured is the way to go? And not mozlog.structured? Is there any documentation about this, by any change?
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.
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.
Blocks: 1197359
You need to log in before you can comment on or make changes to this bug.