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

Create a print Event to replace the print usage in dbt-core #130

Merged
merged 6 commits into from
May 21, 2024

Conversation

ChenyuLInx
Copy link
Contributor

@ChenyuLInx ChenyuLInx commented May 10, 2024

related to dbt-labs/dbt-core#8756

Related PR dbt-labs/dbt-core#10131

Description

We want to have some event that

  • would still output to stdout when --quiet is specified,
  • without the timestamps in front
    This PR does so by creating a specific event in dbt-common and implemented logic to have this event always passed to logger as 'error' level.

Checklist

@cla-bot cla-bot bot added the cla:yes label May 10, 2024
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@@ -120,7 +122,11 @@ 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)
if isinstance(msg.data, PrintEvent):
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@ChenyuLInx ChenyuLInx marked this pull request as ready for review May 15, 2024 23:12
@ChenyuLInx ChenyuLInx requested a review from a team as a code owner May 15, 2024 23:12
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 56.66%. Comparing base (3b61b6f) to head (24a7b7d).
Report is 1 commits behind head on main.

Files Patch % Lines
dbt_common/events/types_pb2.py 10.90% 49 Missing ⚠️
dbt_common/events/logger.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   56.37%   56.66%   +0.28%     
==========================================
  Files          50       50              
  Lines        3090     3106      +16     
==========================================
+ Hits         1742     1760      +18     
+ Misses       1348     1346       -2     
Flag Coverage Δ
unit 56.66% <28.57%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

"category": "",
"code": "Z052",
"extra": {},
"level": "info",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterallenwebb level is expected

@@ -120,7 +122,11 @@ 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)
if isinstance(msg.data, PrintEvent):
level = "error"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following why this happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ChenyuLInx ChenyuLInx requested a review from emmyoop May 20, 2024 21:57
@@ -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
Copy link
Contributor

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".

Copy link
Contributor Author

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.

@ChenyuLInx ChenyuLInx requested a review from gshank May 21, 2024 06:11
@ChenyuLInx ChenyuLInx added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit e37d9c9 May 21, 2024
18 checks passed
@ChenyuLInx ChenyuLInx deleted the cl/print_event branch May 21, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants