Skip to content
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

[CT-1844] Improve Granularity of Log Configuration #6639

Closed
peterallenwebb opened this issue Jan 17, 2023 · 11 comments · Fixed by #6994
Closed

[CT-1844] Improve Granularity of Log Configuration #6639

peterallenwebb opened this issue Jan 17, 2023 · 11 comments · Fixed by #6994
Assignees
Labels
enhancement New feature or request logging

Comments

@peterallenwebb
Copy link
Contributor

peterallenwebb commented Jan 17, 2023

Describe the feature

This is a proposal which would subsume the following four issues:

The proposal is to modify log configuration so that the following command line help would be accurate, allowing for independent file and console log configuration of color, format, and level:

  --log-format {text,json,default}
  --log-format-file {text,json,default}
                        Specify the log format, overriding the command's
                        default. The standard option will apply the format
                        to both console and file output, unless the -file
                        variant (which can also appear alone) is used to 
                        override the log file format.
   --log-level {debug, info, warn, error, none}
   --log-level-file {debug, info, warn, error, none}
                        Set the logging level. Dbt will log events at or above
                        the severity level specified. The standard option will
                        apply to both console and file output, unless the
                        -file variant (which can also appear alone) is used to 
                        override the log file format. The 'none' level disables
                        all log output, including log file creation.
  --use-colors          Colorize (or do not colorize when using the 'no' 
  --use-colors-file     variants) dbt's log output. Output is colorized by
  --no-use-colors       by default and may also be set in a profile or at 
  --no-use-colors-file  the command line. The standard option will apply to
                        to both console and file output, unless the -file
                        variant (which can also appear alone) is used to 
                        override the log file colorization.
@peterallenwebb peterallenwebb added enhancement New feature or request triage labels Jan 17, 2023
@github-actions github-actions bot changed the title Improve Granularity of Log Configuraiton [CT-1844] Improve Granularity of Log Configuraiton Jan 17, 2023
@leahwicz leahwicz removed the triage label Jan 18, 2023
@jtcohen6 jtcohen6 changed the title [CT-1844] Improve Granularity of Log Configuraiton [CT-1844] Improve Granularity of Log Configuration Jan 18, 2023
@peterallenwebb
Copy link
Contributor Author

@jtcohen6 and @MichelleArk I'd be interested in your feedback on the basic ideas here. Do you think it makes the command line options too numerous or confusing?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 18, 2023

@peterallenwebb Nice work consolidating the many requests into a single coherent proposal! I buy it. I don't think we should be afraid of adding more flags when they're justified, even if it makes dbt --help a bit busier.

The target audience for these configs is CLI users. There have been two (and exactly two) default loggers in dbt-core since the dawn of time: "stdout" + "file." I don't anticipate adding any new ones. These configs would make it possible for CLI users to have more granular control over the two loggers that are set up by default.

If I'm following you correctly:

  • dbt --log-level none would disable ALL logging
  • dbt --log-level-file none would disable file logging ONLY
  • dbt --log-level none --log-level-file debug would disable stdout logging, but preserve debug-level file logging

It would also be possible to pass these flags into a programmatic invocation. For anything else, though—configuring additional loggers, with greater flexibility, etc—there's registering a callback on the EventManager.

@peterallenwebb
Copy link
Contributor Author

@jtcohen6 Your examples capture my intentions, yes.

@DustinMoriarty
Copy link

I think that in general a user should get able to override the entire logging configuration with their own config file. There is no reason for dbt to have to support every nuance of user preferences around logging. Just allow a logging config file to be provided using the standard python logging library configuration. The file logging is the most annoying part. However, usually when I use a library I define all of the loggers with my own configuration so that all of the logs are formatted the same and can be parsed the same with downstream tools. If someone wants a more managed approach they can use the existing option. However, if someone wants to do something else they can provide a log config.

Otherwise you will end up having to reproduce all the options in the python logging config in the cli.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 4, 2023

@DustinMoriarty To help us better understand - could you provide a specific example of the logging config file you're imagining? Is it what's outlined in the docs here? (I've just copy-pasted the following from those docs, I'm not specifically endorsing or advocating for something like this:)

handlers:
  console:
    class : logging.StreamHandler
    formatter: brief
    level   : INFO
    filters: [allow_foo]
    stream  : ext://sys.stdout
  file:
    class : logging.handlers.RotatingFileHandler
    formatter: precise
    filename: logconfig.log
    maxBytes: 1024
    backupCount: 3

From my perspective: There is some genuine dbt-specific complexity with the way that our loggers operate, and we've put a lot of thought into its default behaviors and out-of-the-box configuration options, even at the risk of them being somewhat non-standard. For folks who want full control over exactly how to configure and run dbt-core, we are undertaking the work (for v1.5) of supporting a Python API / programmatic invocations for the first time, and providing a clean entry point to register your own callback (#6763), which should enable whichever logging handlers with whatever configuration you want.

@DustinMoriarty
Copy link

@DustinMoriarty To help us better understand - could you provide a specific example of the logging config file you're imagining? Is it what's outlined in the docs here? (I've just copy-pasted the following from those docs, I'm not specifically endorsing or advocating for something like this:)

handlers:

  console:

    class : logging.StreamHandler

    formatter: brief

    level   : INFO

    filters: [allow_foo]

    stream  : ext://sys.stdout

  file:

    class : logging.handlers.RotatingFileHandler

    formatter: precise

    filename: logconfig.log

    maxBytes: 1024

    backupCount: 3

From my perspective: There is some genuine dbt-specific complexity with the way that our loggers operate, and we've put a lot of thought into its default behaviors and out-of-the-box configuration options, even at the risk of them being somewhat non-standard. For folks who want full control over exactly how to configure and run dbt-core, we are undertaking the work (for v1.5) of supporting a Python API / programmatic invocations for the first time, and providing a clean entry point to register your own callback (#6763), which should enable whichever logging handlers with whatever configuration you want.

@jtcohen6 : yes I am suggesting allowing the logging config to be passed in. However, for CLI use a file config is more common and appropriate in my mind. The dictionary config is for calling directly within python. A --logging-config-file argument would probably meet all future logging needs. file config docs

@DustinMoriarty
Copy link

The format of the file config is standardized using INI, not yaml or json. It is good to use this format because users may be using the same config file to configure other parts of their application.

@DustinMoriarty
Copy link

Also, for python invocation, I would leave logging unconfigured. Configuration of logging is the job of the top level application.

@DustinMoriarty
Copy link

For the complexity of current log config, as long as all that extra data in JSON structured logging is passed to the extra argument in the log statements then it can be picked up by a structured log formatter.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 7, 2023

@DustinMoriarty Ok! Could I ask you to open a new issue requesting that functionality? We're going to move ahead with this issue as presently scoped. At the same time, I can see merit in this feature request: If an end user provides logging config in a log.ini (?) configuration file, we should do our best to respect that file's configuration. I'd be curious to see how many other users / community members would be interested in the same.

@jtcohen6
Copy link
Contributor

This has come up again in a more recent discussion, so I've opened a separate issue to track it, and serve as forum for other folks to weigh in if they'd also be interested: #6993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request logging
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants