New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure logging via TOML config #2483

Merged
merged 68 commits into from Nov 25, 2018

Conversation

Projects
None yet
4 participants
@orsinium
Copy link
Contributor

orsinium commented Aug 7, 2018

Description

  1. Now you can fully configure logging from TOML-config.
  2. Option no_configure_logging disable any logging configuring.
  3. Separate logging configuration from other code for better maintainability.
  4. Also I update Apache copyright notice from my previous PR. Cindicator's leader recommend to use "Vote Inc." company name in copyrights.

Motivation and Context

Q. Why do you use same file for Luigi's config and logging config?
A. Because this is all project config. Also in future Pull Requests I plan to integrate in Luigi configs inheritance which allow also use separate file for logging, local settings or something else.

Q. Why do you do this? We already have support for logging.conf.
A. Because logging via ini-file is ugly, since ini format doesn't support inherit dicts. But TOML (and YAML. JSON and other modern formats) can do it.

Q. Why do you change no_configure_logging behavior?
A. Option no_configure_logging useful when you're configure logging in project bypass to Luigi. For example, when you want to use JSON or YAML configs for it. This is intuitive behavior for option with this name.

Q. Why do you change code structure?
A. Old code structure is hard to test, explore and extend. Now it's beautiful :)

Have you tested this? If so, how?

  1. I have included unit tests.
  2. Don't trust me! I update example repository, and you can play with new logging configuring yourself.

And...

Thank you for best pipeline library. And best music streaming service too. All this things is amazing!

orsinium added some commits Jul 13, 2018

@orsinium orsinium changed the title WIP: Configure logging via TOML config Configure logging via TOML config Aug 8, 2018

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Aug 8, 2018

Now all tests is fine :)

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Aug 9, 2018

Thank you for review :) All your advice applied, tests is passed. Do you have any other improvements?

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Aug 20, 2018

@Tarrasch, how is your review? ^^

@Tarrasch
Copy link
Collaborator

Tarrasch left a comment

So cool to see that we're getting somewhere with improving the logging experience in luigi!

I just added a lot of comments on the code so that the code for this important core part of luigi keeps a high standard. I hope you can find time to address this stuff. I'm happy to merge (without more approval from me) once the issues are addressed.

Thanks!

5. logging_conf_file option
6. ``logging`` section
7. ``--log-level``
8. log_level option

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

Thanks for doing the deep-dive and finding these things out.

try:
from ConfigParser import NoSectionError
except ImportError:
from configparser import NoSectionError

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

I recall this was a standard trick, can you link to some stack overflow thread or something?

Alternatively say # codepath for python < 2.7 something



class DaemonLogging(BaseLogging):
configured = False

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

Is this internal state? Make it _configured maybe?


class DaemonLogging(BaseLogging):
configured = False
log_format = "%(asctime)s %(name)s[%(process)s] %(levelname)s: %(message)s"

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

Can you document if you can override this? If it's private do as commented above.

return configured


class DaemonLogging(BaseLogging):

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

Maybe document this is to be used for luigid

return True

@classmethod
def _default(cls, opts):

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

add a more descriptive name please

InterfaceLogging.config = get_config()

def _clean_config(self):
DaemonLogging.config = LuigiTomlParser()

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

Looks weird. Where does toml come in in this generic context?

Should code like this be in a class TomlParserCmdlineTest(unittest.TestCase): context or something??

This comment has been minimized.

@orsinium

orsinium Aug 24, 2018

Contributor

It is for creating minimal config for tests. It's here for old tests.

self.assertTrue(result)


class PatchedLogging(InterfaceLogging):

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

Was it possible to mock one layer less?


@mock.patch("warnings.warn")
@mock.patch("luigi.interface.setup_interface_logging")
def test_cmdline_logger(self, setup_mock, warn):

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

It's so much code that it's hard to review it all. But can you make sure that all tests you delete are added somewhere else?

This comment has been minimized.

@orsinium

orsinium Aug 24, 2018

Contributor

Yes, all logging well tested in test/setup_logging_test.py

result = self.cls._cli(opts)
self.assertFalse(result)

def test_section(self):

This comment has been minimized.

@Tarrasch

Tarrasch Aug 21, 2018

Collaborator

This test name setup_logging_test.TestDaemonLogging.test_section could have more context if you rename test_section to maybe test_section_returns_true_with_config and another test test_section_returns_false_without_config.

I know we don't follow this style at most places in this code base. But code testing improvements can start somewhere. If you want you can do this is too much you can do this as a follow up.

This comment has been minimized.

@orsinium

orsinium Aug 24, 2018

Contributor

It's test all what we need test for _section method

This comment has been minimized.

@Tarrasch

Tarrasch Aug 25, 2018

Collaborator

The point is that test_section is not saying much compared to test_section_returns_true_with_config.

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Aug 24, 2018

Added some @Tarrasch's improvements. Other requests commented

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Sep 28, 2018

CI is OK now. What about this PR? :)

@dlstadther

This comment has been minimized.

Copy link
Collaborator

dlstadther commented Sep 28, 2018

I'm good here.

Have you been using this feature in production? All is good for your use?

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Sep 28, 2018

Now I'm in another team in the same company. I'll get actual feedback on this feature by next few days :)

@dlstadther

This comment has been minimized.

Copy link
Collaborator

dlstadther commented Sep 28, 2018

Congrats on your move @orsinium ! Appreciate the feedback

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Nov 5, 2018

I got feedback and have fixed example based on this:

  1. disable_existing_loggers should be false for saving already existing loggers.
  2. dot in luigi.server logging means inheritance so we can just configure only luigi logger.

I think, it will be better change luigi-interface to luigi.interface, but I don't do it in this PR because it can broke logging in existing projects.

So. now it is ready, I hope :)

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Nov 22, 2018

@dlstadther, @Tarrasch, what are we going to do with this Pull Request?

@dlstadther

This comment has been minimized.

Copy link
Collaborator

dlstadther commented Nov 25, 2018

I just looked back through this PR and I think it LGTM. Given @Tarrasch had some changes requested, I'll wait for him to give his approval that his comments were addressed.

Thanks for continuing to follow up @orsinium !

@Tarrasch
Copy link
Collaborator

Tarrasch left a comment

I think things can be improved (as always). But anyway you've waited long already (sorry for my slow review), and this PR is actually a big improvement. Thanks!

'core', 'no_configure_logging', False):
setup_interface_logging(logging_conf, env_params.log_level)

InterfaceLogging.setup(env_params)

This comment has been minimized.

@Tarrasch

Tarrasch Nov 25, 2018

Collaborator

Oh nice you even cleaned up this part!

#
"""
This module contains helper classes for configuring logging for luigid and
workers via command line arguments and options from config files.

This comment has been minimized.

@Tarrasch

Tarrasch Nov 25, 2018

Collaborator

Do these docs render nicely? Can you be explicit about if other's can use these methods or not?

return True

@classmethod
def _default(cls, opts):

This comment has been minimized.

@Tarrasch

Tarrasch Nov 25, 2018

Collaborator

(just a reminder to feel free to do this)

@Tarrasch Tarrasch merged commit 0a098f6 into spotify:master Nov 25, 2018

4 checks passed

codecov/changes No unexpected coverage changes found.
Details
codecov/patch 100% of diff hit (target 76.89%)
Details
codecov/project/core 93.19% (target 92%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@orsinium orsinium deleted the orsinium:logging branch Nov 29, 2018

@@ -38,31 +36,7 @@
from luigi import worker
from luigi import execution_summary
from luigi.cmdline_parser import CmdlineParser


def setup_interface_logging(conf_file='', level_name='DEBUG'):

This comment has been minimized.

@honnix

honnix Dec 18, 2018

Contributor

A quick question. We call this method directly to set up logging. What is the equivalence now? Or it is not needed anymore?

@orsinium

This comment has been minimized.

Copy link
Contributor

orsinium commented Dec 18, 2018

Yeah, of course, we call InterfaceLogging instead of:

InterfaceLogging.setup(env_params)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment