-
Notifications
You must be signed in to change notification settings - Fork 15
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
Create a print Event to replace the print usage in dbt-core #130
Changes from 5 commits
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: Features | ||
body: Add a print event to support firing event to console in --quiet mode | ||
time: 2024-05-15T16:11:06.815526-07:00 | ||
custom: | ||
Author: ChenyuLInx | ||
Issue: "8756" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ | |
from dbt_common.events.format import timestamp_to_datetime_string | ||
from dbt_common.utils.encoding import ForgivingJSONEncoder | ||
|
||
from dbt_common.events.types_pb2 import PrintEvent | ||
|
||
# A Filter is a function which takes a BaseEvent and returns True if the event | ||
# should be logged, False otherwise. | ||
Filter = Callable[[EventMsg], bool] | ||
|
@@ -120,7 +122,14 @@ def create_line(self, msg: EventMsg) -> str: | |
def write_line(self, msg: EventMsg): | ||
line = self.create_line(msg) | ||
if self._python_logger is not None: | ||
send_to_logger(self._python_logger, msg.info.level, line) | ||
# We send PrintEvent to logger as error so it goes to stdout | ||
# when --quiet flag is set. | ||
# --quiet flag will filter out all events lower than ERROR. | ||
if isinstance(msg.data, PrintEvent): | ||
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 This is a draft to help me understand some way of achieving the goal. Let's discuss Monday and see what you think. 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. related PR dbt-labs/dbt-core#10131 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. This looks like the best way to achieve what we want, the only drawback is we can not have core inherent this event and define new ones since the proto namespace will change. |
||
level = "error" | ||
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. I'm not following why this happens? 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. This is to make sure the PrintEvent still makes it to stdout even when quiet is specified. Let me add a comment since it is not obvious 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. Done |
||
else: | ||
level = msg.info.level | ||
send_to_logger(self._python_logger, level, line) | ||
|
||
def flush(self): | ||
if self._python_logger is not None: | ||
|
@@ -138,8 +147,11 @@ def create_line(self, msg: EventMsg) -> str: | |
return self.create_debug_line(msg) if self.use_debug_format else self.create_info_line(msg) | ||
|
||
def create_info_line(self, msg: EventMsg) -> str: | ||
ts: str = datetime.utcnow().strftime("%H:%M:%S") | ||
scrubbed_msg: str = self.scrubber(msg.info.msg) # type: ignore | ||
if isinstance(msg.data, PrintEvent): | ||
return scrubbed_msg | ||
|
||
ts: str = datetime.utcnow().strftime("%H:%M:%S") | ||
return f"{self._get_color_tag()}{ts} {scrubbed_msg}" | ||
|
||
def create_debug_line(self, msg: EventMsg) -> str: | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import json | ||
from dbt_common.events.logger import LoggerConfig, _TextLogger, _JsonLogger | ||
from dbt_common.events.base_types import EventLevel, msg_from_base_event | ||
from dbt_common.events.types import PrintEvent | ||
|
||
|
||
def test_create_print_line(): | ||
# No format, still fired even when error is the level | ||
config = LoggerConfig(name="test_logger", level=EventLevel.ERROR) | ||
logger = _TextLogger(config) | ||
msg = msg_from_base_event(PrintEvent(msg="This is a print event")) | ||
expected_line = "This is a print event" | ||
actual_line = logger.create_line(msg) | ||
assert actual_line == expected_line | ||
|
||
|
||
def test_create_print_json(): | ||
# JSON format still have event level being info | ||
config = LoggerConfig(name="test_logger", level=EventLevel.ERROR) | ||
logger = _JsonLogger(config) | ||
msg = msg_from_base_event(PrintEvent(msg="This is a print event")) | ||
expected_json = { | ||
"data": {"msg": "This is a print event"}, | ||
"info": { | ||
"category": "", | ||
"code": "Z052", | ||
"extra": {}, | ||
"level": "info", | ||
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 level is expected |
||
"msg": "This is a print event", | ||
"name": "PrintEvent", | ||
"thread": "MainThread", | ||
}, | ||
} | ||
actual_line = logger.create_line(msg) | ||
actual_json = json.loads(actual_line) | ||
assert "info" in actual_json | ||
actual_json["info"].pop("invocation_id") | ||
actual_json["info"].pop("ts") | ||
actual_json["info"].pop("pid") | ||
assert actual_json == expected_json |
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.
It would be nice if we could avoid importing from types_pb2 if we can, because those classes don't always behave like "real" Python classes and it would be nice to isolate the importing of those classes to just the matching xxx_types.py files. Could we look at msg.info.name instead? It should be the string "PrintEvent".
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.
Thanks!! This also makes it easier if we need to do some expansion.