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.

Sign up for GitHub

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

Jump to bottom

[CT-3175] Convert print call sites to logging events #8756

Closed
1 of 3 tasks
jtcohen6 opened this issue Oct 2, 2023 · 6 comments · Fixed by #10131
Closed
1 of 3 tasks

[CT-3175] Convert print call sites to logging events #8756

jtcohen6 opened this issue Oct 2, 2023 · 6 comments · Fixed by #10131
Assignees
@ChenyuLInx
Labels
bug Something isn't working High Severity bug with significant impact that should be resolved in a reasonable timeframe Impact: CLI Impact: Exp logging

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Oct 2, 2023

Housekeeping

  • I am a maintainer of dbt-core

Short description

There are two places where we currently call print, and should instead fire events. The reason these call print is that we don't want them to include timestamps or other formatting additions when sent to stdout. This enables easy copy-paste or piping to file/jq/etc.

  1. Custom user print Jinja context method (source, docs). Convert to a new UserPrint event.
  2. dbt list with non-JSON logging (source). Convert to existing ListCmdOut event.

We also need a new behavior for the text + stdout logger (and only the combination of text + stdout). We should skip adding formatting (including timestamp/spacing) for the UserPrint + ListOutput events.

Acceptance criteria

  • Tests for these events with different loggers:
    • stdout + text-formatted → change: no formatting
    • stdout + JSON-formatted → no change
    • log file + text-formatted → no change
    • log file + JSON-formatted → no change
  • Catalog any remaining instances of Python print method within dbt-core codebase

Impact to Other Teams

  • IDE & CCLI to support custom user-provided print
  • CCLI will also need to implement the same no-formatting logic for UserPrint + ListOutput events

Will backports be required?

Let's just fix this going forward

Context
@jtcohen6 jtcohen6 added tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality logging labels Oct 2, 2023
@github-actions github-actions bot changed the title Convert print call sites to logging events [CT-3175] Convert print call sites to logging events Oct 2, 2023
@jtcohen6 jtcohen6 mentioned this issue Oct 2, 2023
Closed
@mrhallak
Copy link

mrhallak commented Nov 16, 2023 •
edited

@jtcohen6 if I understand correctly, this also affects the python CLI? I've been trying to suppress the output of the below command but it seems to ignore the -q and --no-print flags.

command = ["ls", "--models"] + models + ["--output", "json", "--output-keys", "config", "name", "--project-dir", "../../../dbt/", "-q", "--no-print"]
parse_result = dbtRunner().invoke(command)

@dbeatty10 dbeatty10 mentioned this issue Jan 18, 2024
Closed
@emmyoop
Copy link
Member

emmyoop commented May 1, 2024

Relates to #10030

@ChenyuLInx
Copy link
Contributor

We should consider deprecating the jinja print in favor of using log when working on this ticket.

@graciegoheen graciegoheen added bug Something isn't working High Severity bug with significant impact that should be resolved in a reasonable timeframe and removed tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality labels May 7, 2024
@graciegoheen
Copy link
Contributor

Note: This may also close #9174

@peterallenwebb
Copy link
Contributor

There is one potential complication with the dbt list behavior, and it is related to @mrhallak's observation. We don't want to show the dbt version notification or line timestamps, etc., but we do want to show the list output itself. I think that's why we've left this as a print() for so long. So we'll need a plan to deal with that.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented May 8, 2024

@peterallenwebb You're right about the need to hide timestamps. We suggest that users who wish to hide unrelated lines use dbt list --quiet, which suppresses all events that aren't "printed" to stdout — the output of dbt list should continue to appear.

Could you imagine adding logic within the EventManager such that:

  • certain events are "print-like" and not suppressed by --quiet
  • those same events should also not be prefixed with timestamps

@dbeatty10 has another good related proposal, which is that --quiet should actually be the default for dbt list (or at least dbt list --output json): #7994

This was referenced May 10, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ChenyuLInx ChenyuLInx

Labels
bug Something isn't working High Severity bug with significant impact that should be resolved in a reasonable timeframe Impact: CLI Impact: Exp logging
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove Print and use event system dbt-labs/dbt-core
6 participants
@peterallenwebb @emmyoop @jtcohen6 @mrhallak @ChenyuLInx @graciegoheen