Enable debug logging using the PYTHON_ENABLE_DEBUG_LOGGING flag #939
Conversation
| from azure_functions_worker.constants import PYTHON_ENABLE_DEBUG_LOGGING | ||
|
|
||
|
|
||
| class TestDebugLoggingEnabledFunctions(testutils.WebHostTestCase): |
There was a problem hiding this comment.
Checking - didn't you have to update the host.json file for enabling the logging? Or is it enabled by default now?
If yes - Do we need another test where it is not enabled by default and we are still enabling the flag and checking that the new debug log doesn't get logged?
Ideally - there is a potential issue -
If a customer didn't update host.json, but enables the new feature flag, we start sending the logs from our end. This could result the choking of the gRPC channel that we already know about. If we can also get the host.json information from the host (which I think we do get during worker_init), then we can also do some check if we need to actually send the logs at all or not.
Note: Not very important but something to be aware of.
There was a problem hiding this comment.
Good catch, will add a test case for that. As discussed, the host.json logging filter is set at function level while this flag is at function app level for the shared root logger. It is not possible to define the flag validity if functions have inconsistent log levels in the host.json. Separate investigation/change will be made to warn customers in case of inconsistent host.json log level and PYTHON_ENABLE_DEBUG_LOGGING value being set.
ghost
added
| @@ -0,0 +1,14 @@ | |||
| # Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Why add this function? Can we just reuse the functions in log_filtering_functions directory?
There was a problem hiding this comment.
good catch, will remove
| @@ -98,34 +97,22 @@ def check_log_async_logging(self, host_out: typing.List[str]): | |||
| self.assertIn('hello info', host_out) | |||
| self.assertIn('and another error', host_out) | |||
|
|
|||
| @unittest.skip("Reverting the debug logs PR as host currently cannot handle" | |||
| "apps with lot of debug statements. Reverting PR: " | |||
| "azure-functions-python-worker/pull/745") | |||
| def test_debug_logging(self): | |||
There was a problem hiding this comment.
Do we need all of these tests if you are already testing this in the test_enable_debug_logging_functions.py? If not, then I guess these can be removed
There was a problem hiding this comment.
yes, this is kept for testing flag unset case
vrdmr
changed the title
| self.assertIn('logging error', host_out) | ||
|
|
||
|
|
||
| class TestDebugLoggingDisabledFunctions(testutils.WebHostTestCase): |
There was a problem hiding this comment.
Can you add PyDoc comments to describe what the tests do? That'll help in the future.
| self.assertNotIn('logging debug', host_out) | ||
|
|
||
|
|
||
| class TestLogFilteringFunctions(testutils.WebHostTestCase): |
There was a problem hiding this comment.
Can you please change the class name to describe what this test is specifically testing?
There was a problem hiding this comment.
Also the same comments of comments.
Description
Per customers' requests to log messages at debug level in their python function code and view them in App Insights, there will be a new field named PYTHON_ENABLE_DEBUG_LOGGING in Function App Settings supported. It will determine whether debug level logs in the customer function code will be recorded to App Insights. When the field is set to 0 or unspecified in the App Settings, debug level logs will not be recorded; when the field is set to 1, debug level logs will be recorded.
Fixes #
PR information
Quality of Code and Contribution Guidelines