-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
{Log} Adapt az_command_data_logger
to Knack 0.8.0
#17324
Conversation
with mock.patch('azure.cli.core.util.handle_exception', _handle_exception_with_log): | ||
result = execute(cli_ctx, command, expect_failure=expect_failure).assert_with_checks(checks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code registers Logger
as a Handler
. It works simply because both Logger
and Handler
expose handle
method by coincidence. It is never guaranteed to work as such and may break any time the internal logic of logging
module changes.
@@ -69,89 +69,88 @@ def handle_exception(ex): # pylint: disable=too-many-locals, too-many-statement | |||
# Print the traceback and exception message | |||
logger.debug(traceback.format_exc()) | |||
|
|||
with CommandLoggerContext(logger): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommandLoggerContext
will be called again at:
azure-cli/src/azure-cli-core/azure/cli/core/azclierror.py
Lines 62 to 65 in 4560e96
def print_error(self): | |
from azure.cli.core.azlogging import CommandLoggerContext | |
from azure.cli.core.style import print_styled_text | |
with CommandLoggerContext(logger): |
Calling it here is redundant.
Besides, no log is written here, so these lines shouldn't be surrounded by CommandLoggerContext
.
# print recommendations to action | ||
if self.recommendations: | ||
for recommendation in self.recommendations: | ||
print(recommendation, file=sys.stderr) | ||
|
||
if self.aladdin_recommendations: | ||
print('\nTRY THIS:', file=sys.stderr) | ||
for recommendation, description in self.aladdin_recommendations: | ||
print_styled_text(recommendation, file=sys.stderr) | ||
print_styled_text(description, file=sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No log is written here. The scope of CommandLoggerContext
should be as small as possible.
Log |
az_command_data_logger
to Knack 0.8.0
Adapt to microsoft/knack#228: [Log]
CLILogging.configure
returns as early as possibleSymptom
az feedback
no longer works with knackdev
branch:Cause
In
pytest
environment,pytest
will configure theroot
logger to capture all logs.The old knack always forcibly set
root
logger level toDEBUG
.The new knack can detect that and it leaves
pytest
root
logger as is ([Log] CLILogging.configure returns as early as possible microsoft/knack#228).In such case, we have to manually set
az_command_data_logger
's level toDEBUG
, otherwise it will honorpytest
root
logger's level and beWARNING
by default (when--log-level
is not set). This makesaz_command_data_logger
's log be ignored. For more info on how Python logging works, seelogging.Logger.getEffectiveLevel
.Additional Context
az feedback
relies onaz_command_data_logger
(which are written to~/.azure/commands/
).az_command_data_logger
is written byazure.cli.core.util.handle_exception
.During testing,
azure.cli.core.util.handle_exception
is mocked byazure.cli.testsdk.patches.patch_main_exception_handler
so that the originalException
can be thrown and asserted.However,
azure.cli.testsdk.patches.patch_main_exception_handler
doesn't write toaz_command_data_logger
.In the oldest code ([wip] az feedback #8829), this is worked around by calling the leaked file handler after
execute
finishes.As [Core] fix logging file fd leaking #13102 fixed the leaked file handler, the old code no longer worked. To work around it again, [Core] fix logging file fd leaking #13102 manually delayed the cleanup for file handler.
This PR refactors [Core] fix logging file fd leaking #13102 with a more elegant
mock.patch
onazure.cli.core.util.handle_exception
again, so that no manual delay of file handler cleanup is necessary.