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

A few final logging touch-ups #4388

Merged
merged 8 commits into from
Dec 2, 2021
Merged

A few final logging touch-ups #4388

merged 8 commits into from
Dec 2, 2021

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Dec 2, 2021 •
edited
Loading

Pick up from things I noticed in #4293. These are tiny non-functional / cosmetic changes only:

  • Remove two unused events (they were removed in Parser no longer takes greedy. Accepts indirect selection, a bool. #4104)
  • More structured ConcurrencyLine
  • Instead of starting or ending messages with \n, precede/follow them with EmptyLine() events. Note: The legacy logger used to wrap these in with TextOnly(), in order to avoid writing them to JSON-formatted logs. I don't care too much about the legacy logger, but I bet we could do something very simple to achieve the same behavior with the new event logger, that is, avoid firing EmptyLine() events when JSON formatting is enabled.

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 — centralizing in Changelog entries for rc3 -> final #4389

@cla-bot cla-bot bot added the cla:yes label Dec 2, 2021
@jtcohen6 jtcohen6 mentioned this pull request Dec 2, 2021
Closed
4 tasks
@jtcohen6 jtcohen6 force-pushed the jerco/final-logging-touchups branch 2 times, most recently from e7c11a2 to 137ba88 Compare December 2, 2021 11:53
@jtcohen6 jtcohen6 requested review from emmyoop and nathaniel-may and removed request for emmyoop December 2, 2021 12:13
emmyoop
emmyoop requested changes Dec 2, 2021
View reviewed changes
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.

Just a couple more cleanup spots.

core/dbt/task/runnable.py Outdated
warn_or_error("\nWARNING: Nothing to do. Try checking your model "
with TextOnly():
fire_event(EmptyLine())
warn_or_error("WARNING: Nothing to do. Try checking your model "
Copy link
Member

Choose a reason for hiding this comment

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

Take out the "WARNING" text too since we want to remove that duplication.

Copy link
Contributor Author

@jtcohen6 jtcohen6 Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We're no longer including the log level in the CLI output (since #4379), so it's not as duplicative as it used to be. But I can sub this out to use ui.warning_tag, so at least the logic is consistent/centralized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize I changed warning_tag to do nothing! Let me change it to do something again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking we could use these for deprecation warnings too, just to give them a little colorful pop like the others:
Screenshot 2021-12-02 at 18 20 05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emmyoop let me know if you hate it!

core/dbt/events/types.py Outdated
@@ -863,7 +829,7 @@ class InvalidVarsYAML(ErrorLevel, Cli, File):
code: str = "A008"

def message(self) -> str:
return "The YAML provided in the --vars argument is not valid.\n"
return "The YAML provided in the --vars argument is not valid."


# TODO: Remove? (appears to be uncalled)
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove these too?

Copy link
Member

Choose a reason for hiding this comment

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

This highlighted wrong. I'm referring to HandleInternalException, CatchRunException, DetailsHandleGenericException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep can do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also removing MessageHandleGenericException, since it doesn't have a code and appears to be uncalled

@nathaniel-may
Copy link
Contributor

nathaniel-may commented Dec 2, 2021 •
edited
Loading

... avoid firing EmptyLine() events when JSON formatting is enabled.

yup. we really should do this.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Dec 2, 2021 •
edited
Loading

... avoid firing EmptyLine() events when JSON formatting is enabled.

yup. we really should do this.

@nathaniel-may I did a very simple, very janky version of this in a8ecaf5 a9cbdc8. Let me know if you hate it and I'll happily revert it.

I imagine the better approach would be to implement JSON + Text base types. FWIW I checked through all instances of TextOnly(), and EmptyLine is the only event that ever applies, so I don't feel too bad about making it a special case for now.

@jtcohen6 jtcohen6 requested a review from emmyoop December 2, 2021 20:13
@jtcohen6 jtcohen6 mentioned this pull request Dec 2, 2021
Closed
26 tasks
emmyoop
emmyoop approved these changes Dec 2, 2021
View reviewed changes
core/dbt/deprecations.py
@@ -62,7 +62,7 @@ class PackageInstallPathDeprecation(DBTDeprecation):

class ConfigPathDeprecation(DBTDeprecation):
_description = '''\
The `{deprecated_path}` config has been deprecated in favor of `{exp_path}`.
The `{deprecated_path}` config has been renamed to `{exp_path}`.
Copy link
Member

Choose a reason for hiding this comment

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

Love the wording updates.

@jtcohen6 jtcohen6 merged commit b90ab74 into main Dec 2, 2021
@jtcohen6 jtcohen6 deleted the jerco/final-logging-touchups branch December 2, 2021 21:09
jtcohen6 added a commit that referenced this pull request Dec 2, 2021
@jtcohen6
A few final logging touch-ups (#4388)
804fa49 
* Rm unused events, per #4104

* More structured ConcurrencyLine

* Replace \n prefixes with EmptyLine

* Reimplement ui.warning_tag to centralize logic

* Use warning_tag for deprecations too

* Rm more unused event types

* Exclude EmptyLine from json logs

* loglines are not always created by events (#4406)

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
@jtcohen6 jtcohen6 mentioned this pull request Dec 2, 2021
Merged
jtcohen6 added a commit that referenced this pull request Dec 2, 2021
@jtcohen6
A few final logging touch-ups (#4388)
1c5c853 
* Rm unused events, per #4104

* More structured ConcurrencyLine

* Replace \n prefixes with EmptyLine

* Reimplement ui.warning_tag to centralize logic

* Use warning_tag for deprecations too

* Rm more unused event types

* Exclude EmptyLine from json logs

* loglines are not always created by events (#4406)

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
leahwicz pushed a commit that referenced this pull request Dec 2, 2021
@jtcohen6
[Backport] #4388 + #4405 (#4408)
c92e1ed 
* A few final logging touch-ups (#4388)

* Rm unused events, per #4104

* More structured ConcurrencyLine

* Replace \n prefixes with EmptyLine

* Reimplement ui.warning_tag to centralize logic

* Use warning_tag for deprecations too

* Rm more unused event types

* Exclude EmptyLine from json logs

* loglines are not always created by events (#4406)

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

* Rollover + backup for dbt.log (#4405)

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@jtcohen6 @iknox-fa
A few final logging touch-ups (#4388)
9e0fdf9 
* Rm unused events, per #4104

* More structured ConcurrencyLine

* Replace \n prefixes with EmptyLine

* Reimplement ui.warning_tag to centralize logic

* Use warning_tag for deprecations too

* Rm more unused event types

* Exclude EmptyLine from json logs

* loglines are not always created by events (#4406)

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

automatic commit by git-black, original commits:
  8bdaa69
  b90ab74
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
@jtcohen6 @iknox-fa
A few final logging touch-ups (#4388)
be0ff2d 
* Rm unused events, per #4104

* More structured ConcurrencyLine

* Replace \n prefixes with EmptyLine

* Reimplement ui.warning_tag to centralize logic

* Use warning_tag for deprecations too

* Rm more unused event types

* Exclude EmptyLine from json logs

* loglines are not always created by events (#4406)

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

automatic commit by git-black, original commits:
  b90ab74
@aranke aranke mentioned this pull request May 20, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmyoop emmyoop emmyoop approved these changes

@nathaniel-may nathaniel-may Awaiting requested review from nathaniel-may

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jtcohen6 @nathaniel-may @emmyoop