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

Misc logging cleanup + commentary #4293

Closed
wants to merge 5 commits into from
Closed

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Nov 17, 2021

I played around with the logging module this morning from an end-user perspective. It was a lot of fun! There are some fixes we should definitely merge; some questions are open for discussion. I tried to keep the commits separate + clean, so we could split up and cherry-pick accordingly. This is all in the interest of making dbt look + feel "like dbt" by v1 final.

Critical fixes

Touch-ups + cosmetic fixes

  • For events whose messages were prefixed with \n, instead precede them with fire_event(EmptyLine()). This is much cleaner to look at, since all (non-wrapped) log lines get the same frontmatter (timestamp + log level)
  • I happened to notice that ConcurrencyLine could be more structured (unless there's a good reason why it wasn't)
  • If we're going to include log levels on the CLI (more on this below), disable duplicative WARNING prefixes in messages / formatted via warning_tag

Commentary

  • Log levels: These are amazing to have in the File. I find they add a bit too much noise on the CLI. I'd be happy with either (a) turning them off for CLI logging, or (b) colorizing warn/yellow + error/red to raise the signal from the noise. (Also, I believe Minor Cleanup of Structured Logging Module #4266 removed width-fixing for log levels, so I added that back in.)
  • Colorization: Ideally, we'd have a way to turn this off for both File + JSON-formatted logging. This would require a little bit of plumbing: dbt.ui.color would need information from the events module, beyond just flags.USE_COLORS. Since color methods are mostly called in messages defined on event classes, I wonder if this could work by adding a use_color(self) -> bool method to the File + Cli base types, and reimplementing the color methods to check self.use_color (where self = event class).
  • EmptyLine: Just like colorization, this is great for CLI + text formatting, but we'd ideally have a way to turn it off for File or JSON logging. (This is what logbook accomplished via the TextOnly() wrapper; I realize I was a bit inconsistent in whether I used this.)

Broadly, I find that I want:

  • color, concision, clean spacing on the CLI
  • colorless + compact verbosity in the File + JSON-formatted

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Nov 17, 2021
@jtcohen6
Copy link
Contributor Author

@nathaniel-may @emmyoop @iknox-fa Tagging you here for visibility. The critical fix noted above is the only thing we must have for RC2. I know the rest of this is lower priority than the other work we've itemized in #4260.

@nathaniel-may
Copy link
Contributor

If we're going to include log levels on the CLI (more on this below), disable duplicative WARNING prefixes in messages / formatted via warning_tag

This is a good reason why I think the warning stuff should live in the events mod eventually.

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +240 to +244
log_line = (
create_json_log_line(e, msg_fn=lambda x: x.file_msg())
if this.format_json else
create_text_log_line(e, msg_fn=lambda x: x.file_msg())
)
Copy link
Contributor

@nathaniel-may nathaniel-may Nov 17, 2021

Choose a reason for hiding this comment

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

let's encapsulate this into a function since it's used 3 times.

Suggested change
log_line = (
create_json_log_line(e, msg_fn=lambda x: x.file_msg())
if this.format_json else
create_text_log_line(e, msg_fn=lambda x: x.file_msg())
)
log_line = create_log_line(json=this.format_json, e=e, msg_fn=lambda x: x.file_msg())

where

def create_log_line(json: bool, e: T_Event, msg_fn: Callable[[T_Event], str]) -> str:
    return (
        create_json_log_line(e, msg_fn)
        if this.format_json else
        create_text_log_line(e, msg_fn)
    )

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Dec 2, 2021

Closing in favor of #4388

@jtcohen6 jtcohen6 closed this Dec 2, 2021
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.

3 participants