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

Feature/logbook structured rpc logging (#1703, #1704) #1715

Merged

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Aug 30, 2019

Fixes #1703
Fixes #1704

As part of this PR, I stumbled on a pair of odd connection related bugs that I fixed.

  • Log compilation to an array of messages as well as stdout/dbt.log, and return that in the status endpoint. (In the RPC server, have the "status" method return the logs from the last compile #1703)
  • Log using the logbook package, spit out json in dbt rpc (Rationalize logging in the rpc server #1704)
    • This json output matches the data returned in the logs field of rpc responses
  • dbt.logger now provides a log_manager that lets you toggle the various global logging settings:
    • debug mode (level + text format changes for dbt --debug ...)
    • human-readable text output vs json (for dbt rpc)
    • stdout vs stderr (for dbt ls)
  • made dbt/rpc.py into a package dbt/rpc, added it to mypy.ini.
  • Added some convenience methods to the log manager for logging control

This will break anything that hooks in to the dbt logger :)

Jacob Beck added 4 commits August 27, 2019 10:54
 - fix a bug where failed connections caused an AttributeError
 - fix an issue where the rpc server and its child processes secretly shared mutable state
Log rpc as json, preserve existing log behavior
Force werkzeug logs into logbook handling
Make rpc a package and set up mypy checking on it
fix a lot of logging-related bugs
Provide a log manager to toggle various log functionalities
add disabled flag to file handler for "dbt debug" and friends
Test changes to support logbook
lots of fiddly process/thread handling
@cla-bot cla-bot bot added the cla:yes label Aug 30, 2019
@beckjake beckjake marked this pull request as ready for review August 30, 2019 15:03
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Couple of questions / comments here, but overall this looks very good! I did some quick testing locally and I was really impressed with how well everything worked! Logbook looks really incredible - I think that's a perfect library to use here.

One quick follow up question: You mentioned that you "stumbled on a pair of odd connection related bugs that I fixed" - can you say more about that?

Really nice work, Jake!

core/dbt/logger.py Show resolved Hide resolved
core/dbt/main.py Outdated Show resolved Hide resolved
core/dbt/rpc/response_manager.py Show resolved Hide resolved
core/dbt/rpc/task_manager.py Show resolved Hide resolved
core/dbt/adapters/sql/connections.py Outdated Show resolved Hide resolved
@beckjake
Copy link
Contributor Author

You mentioned that you "stumbled on a pair of odd connection related bugs that I fixed" - can you say more about that?

The first I fixed in this commit - on failed connections, we would have connection.handle = None (because we got no handle) but were still calling close() on the handle from SQLConnectionManager.cancel_open

The second was in the integration tests - in some failure paths, we did a similar thing where we tried to rollback a handle, but the handle might be None, which obscured the actual failing error message.

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

This LGTM when the tests pass :)

@beckjake beckjake merged commit 5c3874a into dev/louisa-may-alcott Aug 30, 2019
@beckjake beckjake deleted the feature/logbook-structured-rpc-logging branch August 30, 2019 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants