Add debug logs to packager #934
Conversation
Codecov Report
@@ Coverage Diff @@
## master #934 +/- ##
==========================================
+ Coverage 95.52% 95.52% +<.01%
==========================================
Files 27 27
Lines 4449 4474 +25
Branches 558 560 +2
==========================================
+ Hits 4250 4274 +24
Misses 128 128
- Partials 71 72 +1
Continue to review full report at Codecov.
|
chalice/deploy/packager.py
Outdated
| @@ -27,6 +28,9 @@ | |||
| OptStr = Optional[str] | |||
| OptBytes = Optional[bytes] | |||
|
|
|||
| LOGGER_NAME = 'packager' | |||
| logger = logging.getLogger(LOGGER_NAME) | |||
There was a problem hiding this comment.
You can just use __name__ here instead of LOGGER_NAME. This allows the file to get moved around without having to update the string.
There was a problem hiding this comment.
Yeah I'm fine with just using __name__ and having this be part of our standard debug logs.
tests/integration/test_cli.py
Outdated
| req_file = os.path.join(local_app, 'requirements.txt') | ||
| with open(req_file, 'w') as f: | ||
| f.write('pip==10.0.0\n') | ||
| p = subprocess.Popen(['chalice', '--debug', 'package', 'out'], |
There was a problem hiding this comment.
So the coverage is down because it is being ran in a subprocess?
There was a problem hiding this comment.
Yes unless i'm missing something I cant see how to even test top level args with the click test runner.
There was a problem hiding this comment.
Does just passing the cli function to the click runner not work?
chalice/deploy/packager.py
Outdated
| @@ -405,6 +415,7 @@ def _download_dependencies(self, directory, requirements_filename): | |||
| # compiler. | |||
| compatible_wheels, incompatible_wheels = self._categorize_wheel_files( | |||
| directory) | |||
| logger.debug("compatible: %s", compatible_wheels) | |||
There was a problem hiding this comment.
It looks like there are a few compatible repeats. It may be good to update it so the statements are unique so it is not ambiguous where the debug line is coming from and worded in a way that I do not have to dive into the code to see at what step the compatible log entry is getting written.
There was a problem hiding this comment.
Yea so I think this is unavoidable. The logs are only useful if I can see the state of the world between each step.
There was a problem hiding this comment.
I think Kyle's just asking to add something besides "compatible" so that similar lines (such as line 404) can be distinguished without having to dig through the source code. Maybe something like compatible wheels after second download pass, compatible wheels after categorization, etc.
chalice/cli/__init__.py
Outdated
| @@ -363,6 +380,9 @@ def generate_sdk(ctx, sdk_type, stage, outdir): | |||
| def package(ctx, single_file, stage, out): | |||
| # type: (click.Context, bool, str, str) -> None | |||
| factory = ctx.obj['factory'] # type: CLIFactory | |||
| debug = ctx.obj['debug'] # type: bool | |||
There was a problem hiding this comment.
I wonder if it makes sense to instantiate this at the cli level so it gets essentially set for all chalice commands? While it is not bad for two commands, I think we should try to avoid having to do this every time to a new command that needs logs displayed when running --debug. We also set it when we create the botocore session. So there is definitely some potential refactoring. Also what was the reasoning behind not setting the logger name to an empty string?
There was a problem hiding this comment.
This was more of a POC to facilitate this discussion. Not sure how we want to do logging, nor am I sure how it should be done with click or if there is a better pattern available.
There was a problem hiding this comment.
I think at least for now, we should have a single place that configures logging. We were previously relying on the set_debug_logger() in the botocore session previously, but now that we're adding our own logging in chalice, we should just do the configuration ourselves and not rely on botocore's configuration.
It might be simplest to just call _configure_logging() in cli() and remove the call to set_debug_logger() in the CLIFactory.
Added log statements to track the state of all packages in between each step during the packaging process. A function in the CLI layer was added to enable this logger when the --debug flag is passed.
stealthycoin
force-pushed
the
package-debug
branch
from
4a107b9
to
06dccce
Compare
tests/functional/cli/test_factory.py
Outdated
| def test_can_create_botocore_session(): | ||
| session = factory.create_botocore_session() | ||
| assert session.user_agent().startswith('aws-chalice/') | ||
| assert session.get_default_client_config() is None | ||
|
|
||
|
|
||
| def test_can_create_botocore_session_debug(): |
There was a problem hiding this comment.
Not sure why this was removed. Can you add this back (unless you know why it needs to be removed)?
Add debug logs to packager
Added log statements to track the state of all packages in between each
step during the packaging process. A function in the CLI layer was added
to enable this logger when the --debug flag is passed.