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

Fix petits logging regressions from feature/click-cli merge #6940

Merged
merged 7 commits into from
Feb 14, 2023

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Feb 10, 2023

resolves #6933

Description

  • Add "initialization" events that previously lived in old main.py
  • Update how flags are used in EventManager initialization, to fix --log-cache-events. I think we have access to less than we used to(?)
  • Try adding exception handling / event firing back to track_run, matching old main.py. Not sure if we need/want to do this

Checklist

@cla-bot cla-bot bot added the cla:yes label Feb 10, 2023
@github-actions
Copy link
Contributor

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.

fmt = LineFormat.PlainText
if log_format == "json":
fmt = LineFormat.Json
elif debug:
fmt = LineFormat.DebugText
level = EventLevel.DEBUG if debug else EventLevel.INFO
# We don't have access to these values when we need to setup the default stdout logger!
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 I don't think we'll be able to solve #6847 in the same way you did #6886, because we now need a click Context to access flags.QUIET (even if it's passed in on the CLI), and we don't have access to that as early as we used to.

That approach was always a bit opportunistic, so I don't feel like this is a huge deal. Open to ideas for sure.

Comment on lines 460 to 464
# import click.Abort as clickAbortError
# except clickAbortError: # a.k.a. MainKeyboardInterrupt
# if the logger isn't configured yet, it will use the default logger
# fire_event(MainKeyboardInterrupt()) # how much do we care about this?
# raise
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to emulate this logic:

dbt-core/core/dbt/main.py

Lines 142 to 145 in 4d0ee2f

except KeyboardInterrupt:
# if the logger isn't configured yet, it will use the default logger
fire_event(MainKeyboardInterrupt())
exit_code = ExitCodes.UnhandledError.value

Not sure of exactly how this works now. The only thing we're losing out on is a structured event (MainKeyboardInterrupt) recording that a run was cancelled. Still, that might be cause enough to want to keep this logic around.

Comment on lines 457 to 458
if not isinstance(e, dbtException):
fire_event(MainStackTrace(stack_trace=traceback.format_exc()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emulating this logic:

dbt-core/core/dbt/main.py

Lines 151 to 155 in 4d0ee2f

except BaseException as e:
fire_event(MainEncounteredError(exc=str(e)))
if not isinstance(e, dbtException):
fire_event(MainStackTrace(stack_trace=traceback.format_exc()))
exit_code = ExitCodes.UnhandledError.value

@jtcohen6
Copy link
Contributor Author

Try adding exception handling / event firing back to track_run, matching old main.py. Not sure if we need/want to do this

I'm going to kick this piece out of the present PR, and prioritize the simpler & less controversial changes for now

Comment on lines +45 to +46
flags_dict_str = cast_dict_to_dict_of_strings(get_flag_dict())
fire_event(MainReportArgs(args=flags_dict_str))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values needs to be stringified first, because it's defined as Dict[str, str]:

// A002
message MainReportArgs {
map<string, string> args = 1;
}

(The good news is, this was caught by some of our functional tests!)

As discussed during BLG on Friday, we should investigate if we can support the Struct proto type (Dict[str, Any]): #6803 (comment)

@jtcohen6 jtcohen6 force-pushed the jerco/logging-fixes-on-main branch from 60a0f03 to 05661a9 Compare February 13, 2023 14:28
@jtcohen6 jtcohen6 closed this Feb 13, 2023
@jtcohen6 jtcohen6 reopened this Feb 13, 2023
@jtcohen6 jtcohen6 marked this pull request as ready for review February 13, 2023 20:34
@jtcohen6 jtcohen6 requested review from a team as code owners February 13, 2023 20:34
@jtcohen6 jtcohen6 requested review from emmyoop and Fleid February 13, 2023 20:34
Copy link
Contributor

@peterallenwebb peterallenwebb left a comment

Choose a reason for hiding this comment

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

Looks good.

@jtcohen6 jtcohen6 merged commit 1250f23 into main Feb 14, 2023
@jtcohen6 jtcohen6 deleted the jerco/logging-fixes-on-main branch February 14, 2023 09:48
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.

[CT-2078] Logging issues since feature branch merge
3 participants