Added debug messages and use the logging framework for stdout #178
Conversation
There was a problem hiding this comment.
Thank you for taking this on @douglaz and going through the code to find good candidates for conversion. I do not have extensive experience with logger so I will defer to your judgement on a few things here.
Is the idea to convert all print() statements over to logger? If not, what is the rule or heuristic for when something should remain a print()?
Looks like you need to rebase the PR btw.
flintrock/ec2.py
Outdated
| @@ -785,7 +785,7 @@ def _create_instances( | |||
| if spot_requests: | |||
| request_ids = [r['SpotInstanceRequestId'] for r in spot_requests] | |||
| if any([r['State'] != 'active' for r in spot_requests]): | |||
| logger.info("Canceling spot instance requests...", file=sys.stderr) | |||
| print("Canceling spot instance requests...", file=sys.stderr) | |||
There was a problem hiding this comment.
Why did you revert this logger statement?
There was a problem hiding this comment.
Because it doesn't work. When you call a log, you can't decide where it will go. You need to configure on log creation where the output will be (stdout, stderr, file or a combination of all this...)
flintrock/ec2.py
Outdated
| @@ -121,6 +125,12 @@ def wait_for_state(self, state: str): | |||
| ec2 = boto3.resource(service_name='ec2', region_name=self.region) | |||
|
|
|||
| while any([i.state['Name'] != state for i in self.instances]): | |||
| if logger.isEnabledFor(logging.DEBUG): | |||
| waiting_instances = [i for i in self.instances if i.state['Name'] != state] | |||
| sample_size = 3 | |||
There was a problem hiding this comment.
Don't see the need for this variable. I think [:3] on the next line is OK.
flintrock/ec2.py
Outdated
| waiting_instances = [i for i in self.instances if i.state['Name'] != state] | ||
| sample_size = 3 | ||
| sample = [i.id for i in waiting_instances][:sample_size] | ||
| logger.debug('{size} instances not in state {state}, like: {sample} ...'.format(size=len(waiting_instances), state=state, sample=sample)) |
There was a problem hiding this comment.
Formatting nitpick: It's awkward to have a closed list followed by ellipses. We should also quote keywords to distinguish them from prose.
e.g.:
72 instances not in state 'running': 'i-0ff168f921510cf35', 'i-05cabf994eb94dba8', 'i-039411476cc0bd181', ...
flintrock/flintrock.py
Outdated
| @@ -38,6 +39,9 @@ | |||
| THIS_DIR = os.path.dirname(os.path.realpath(__file__)) | |||
|
|
|||
|
|
|||
| logger = logging.getLogger('flintrock') | |||
There was a problem hiding this comment.
I'm not too familiar with logging, but judging from the pattern, shouldn't this be logging.getLogger('flintrock.flintrock')?
flintrock/ssh.py
Outdated
| break | ||
| except socket.timeout as e: | ||
| logger.debug("[{h}] SSH got a timeout...".format(h=host)) |
There was a problem hiding this comment.
I would prefer these messages to be without the "got a". So simply:
- "SSH timeout."
- "SSH exception: ..."
- "SSH AuthenticationException."
flintrock/flintrock.py
Outdated
| @click.group() | ||
| @click.option( | ||
| '--config', | ||
| help="Path to a Flintrock configuration file.", | ||
| default=get_config_file()) | ||
| @click.option('--provider', default='ec2', type=click.Choice(['ec2'])) | ||
| @click.version_option(version=__version__) | ||
| # TODO: implement some solution like in https://github.com/pallets/click/issues/108 | ||
| @click.option('--debug/--no-debug', default=False, help="Whether to show debug messages") |
There was a problem hiding this comment.
For consistency with the other help messages, I prefer: "Show debug information."
|
@nchammas ideally everything would use logging. I kept all stderr prints because you can't make some logging calls go to stdout and others to stderr. Perhaps we can configure a stderr_logger to use in those cases. I'll try that later. |
|
@nchammas Please take a look. Regarding stdout/stderr. I think we can't customize it by call. We can configure for everything go to stdout or to stderr, but not some messages to stdout and others to stderr. All stderr messages are still using Now the log with |
|
Also this: |
flintrock/flintrock.py
Outdated
| @@ -161,15 +161,17 @@ def get_config_file() -> str: | |||
|
|
|||
|
|
|||
| def configure_log(debug: bool): | |||
| handler = logging.StreamHandler() | |||
| root_logger = logging.getLogger('flintrock') | |||
| import sys | |||
There was a problem hiding this comment.
Do we need this? sys is already imported at the top of the file.
|
Thanks for sharing the sample output. Looks really neat! 👍 In addition to my minor review comment, we also need to add I think with those 2 changes in place I'll be able to merge this in. |
|
@nchammas please review the changes |
|
I tested this out and it looks good to me! Thanks for working on this @douglaz. |
This PR makes the following changes:
I tested this PR by launching a cluster. Output sample:
Fixes #177.
Helps #47 (I'm not sure it completely solves it).