-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix petits logging regressions from feature/click-cli
merge
#6940
Changes from all commits
e9d3da4
db50784
11b4a75
237e3ab
dd31989
05661a9
704b1af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Fixes | ||
body: Readd initialization events, --log-cache-events in new CLI | ||
time: 2023-02-13T13:05:22.989477+01:00 | ||
custom: | ||
Author: jtcohen6 | ||
Issue: "6933" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,7 @@ | |
from dbt.events.eventmgr import EventManager, LoggerConfig, LineFormat, NoFilter | ||
from dbt.events.helpers import env_secrets, scrub_secrets | ||
from dbt.events.types import Formatting | ||
from dbt.flags import get_flags | ||
import dbt.flags as flags_module | ||
from dbt.flags import get_flags, ENABLE_LEGACY_LOGGER | ||
from dbt.logger import GLOBAL_LOGGER, make_log_dir_if_missing | ||
from functools import partial | ||
import json | ||
|
@@ -23,7 +22,7 @@ def setup_event_logger(log_path: str, log_format: str, use_colors: bool, debug: | |
cleanup_event_logger() | ||
make_log_dir_if_missing(log_path) | ||
|
||
if get_flags().ENABLE_LEGACY_LOGGER: | ||
if ENABLE_LEGACY_LOGGER: | ||
EVENT_MANAGER.add_logger(_get_logbook_log_config(debug)) | ||
else: | ||
EVENT_MANAGER.add_logger(_get_stdout_config(log_format, debug, use_colors)) | ||
|
@@ -43,12 +42,18 @@ def setup_event_logger(log_path: str, log_format: str, use_colors: bool, debug: | |
|
||
|
||
def _get_stdout_config(log_format: str, debug: bool, use_colors: bool) -> LoggerConfig: | ||
flags = get_flags() | ||
fmt = LineFormat.PlainText | ||
if log_format == "json": | ||
fmt = LineFormat.Json | ||
elif debug: | ||
fmt = LineFormat.DebugText | ||
level = EventLevel.DEBUG if debug else EventLevel.INFO | ||
# We don't have access to these values when we need to setup the default stdout logger! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peterallenwebb I don't think we'll be able to solve #6847 in the same way you did #6886, because we now need a click Context to access That approach was always a bit opportunistic, so I don't feel like this is a huge deal. Open to ideas for sure. |
||
log_cache_events = ( | ||
bool(flags.LOG_CACHE_EVENTS) if hasattr(flags, "LOG_CACHE_EVENTS") else False | ||
) | ||
quiet = bool(flags.QUIET) if hasattr(flags, "QUIET") else False | ||
return LoggerConfig( | ||
name="stdout_log", | ||
level=level, | ||
|
@@ -57,9 +62,9 @@ def _get_stdout_config(log_format: str, debug: bool, use_colors: bool) -> Logger | |
scrubber=env_scrubber, | ||
filter=partial( | ||
_stdout_filter, | ||
bool(flags_module.LOG_CACHE_EVENTS), | ||
log_cache_events, | ||
debug, | ||
bool(flags_module.QUIET), | ||
quiet, | ||
log_format, | ||
), | ||
output_stream=sys.stdout, | ||
|
@@ -124,10 +129,11 @@ def cleanup_event_logger(): | |
# currently fire before logs can be configured by setup_event_logger(), we | ||
# create a default configuration with default settings and no file output. | ||
EVENT_MANAGER: EventManager = EventManager() | ||
# Problem: This needs to be set *BEFORE* we've resolved any flags (even CLI params) | ||
EVENT_MANAGER.add_logger( | ||
_get_logbook_log_config(flags_module.DEBUG) # type: ignore | ||
if flags_module.ENABLE_LEGACY_LOGGER | ||
else _get_stdout_config(flags_module.LOG_FORMAT, flags_module.DEBUG, flags_module.USE_COLORS) # type: ignore | ||
_get_logbook_log_config(debug=False) # type: ignore | ||
if ENABLE_LEGACY_LOGGER | ||
else _get_stdout_config(log_format="text", debug=False, use_colors=True) # type: ignore | ||
) | ||
|
||
# This global, and the following two functions for capturing stdout logs are | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values needs to be stringified first, because it's defined as
Dict[str, str]
:dbt-core/core/dbt/events/types.proto
Lines 82 to 85 in f1087e5
(The good news is, this was caught by some of our functional tests!)
As discussed during BLG on Friday, we should investigate if we can support the
Struct
proto type (Dict[str, Any]
): #6803 (comment)