Skip to content

Enable debug logging using the PYTHON_ENABLE_DEBUG_LOGGING flag #939

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

Merged
merged 7 commits into from
Jan 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
# For all file changes, github would automatically
# include the following people in the PRs.

* @vrdmr @gavin-aguiar
* @vrdmr @gavin-aguiar @YunchuWang
1 change: 1 addition & 0 deletions azure_functions_worker/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
PYTHON_THREADPOOL_THREAD_COUNT = "PYTHON_THREADPOOL_THREAD_COUNT"
PYTHON_ISOLATE_WORKER_DEPENDENCIES = "PYTHON_ISOLATE_WORKER_DEPENDENCIES"
PYTHON_ENABLE_WORKER_EXTENSIONS = "PYTHON_ENABLE_WORKER_EXTENSIONS"
PYTHON_ENABLE_DEBUG_LOGGING = "PYTHON_ENABLE_DEBUG_LOGGING"
FUNCTIONS_WORKER_SHARED_MEMORY_DATA_TRANSFER_ENABLED = \
"FUNCTIONS_WORKER_SHARED_MEMORY_DATA_TRANSFER_ENABLED"
"""
Expand Down
14 changes: 10 additions & 4 deletions azure_functions_worker/dispatcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@
from .constants import (PYTHON_THREADPOOL_THREAD_COUNT,
PYTHON_THREADPOOL_THREAD_COUNT_DEFAULT,
PYTHON_THREADPOOL_THREAD_COUNT_MAX_37,
PYTHON_THREADPOOL_THREAD_COUNT_MIN)
PYTHON_THREADPOOL_THREAD_COUNT_MIN,
PYTHON_ENABLE_DEBUG_LOGGING)
from .logging import disable_console_logging, enable_console_logging
from .logging import (logger, error_logger, is_system_log_category,
CONSOLE_LOG_PREFIX)
from .extension import ExtensionManager
from .utils.common import get_app_setting
from .utils.common import get_app_setting, is_envvar_true
from .utils.tracing import marshall_exception_trace
from .utils.dependency import DependencyManager
from .utils.wrappers import disable_feature_by
Expand Down Expand Up @@ -145,8 +146,9 @@ async def dispatch_forever(self):
logging_handler = AsyncLoggingHandler()
root_logger = logging.getLogger()

# Don't change this unless you read #780 and #745
root_logger.setLevel(logging.INFO)
log_level = logging.INFO if not is_envvar_true(
PYTHON_ENABLE_DEBUG_LOGGING) else logging.DEBUG
root_logger.setLevel(log_level)
root_logger.addHandler(logging_handler)
logger.info('Switched to gRPC logging.')
logging_handler.flush()
Expand Down Expand Up @@ -484,6 +486,10 @@ async def _handle__function_environment_reload_request(self, req):
self._create_sync_call_tp(self._get_sync_tp_max_workers())
)

if is_envvar_true(PYTHON_ENABLE_DEBUG_LOGGING):
root_logger = logging.getLogger()
root_logger.setLevel(logging.DEBUG)

# Reload azure google namespaces
DependencyManager.reload_customer_libraries(
func_env_reload_request.function_app_directory
Expand Down
5 changes: 2 additions & 3 deletions azure_functions_worker/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@ def parse_args():
parser.add_argument('--host',
help="host address")
parser.add_argument('--port', type=int,
help='id for the requests')
help='port number')
parser.add_argument('--workerId', dest='worker_id',
help='id for the worker')
parser.add_argument('--requestId', dest='request_id',
help='log destination: stdout, stderr, '
'syslog, or a file path')
help='id of the request')
parser.add_argument('--log-level', type=str, default='INFO',
choices=['TRACE', 'INFO', 'WARNING', 'ERROR'],
help="log level: 'TRACE', 'INFO', 'WARNING', "
Expand Down
58 changes: 56 additions & 2 deletions tests/endtoend/test_linux_consumption.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from unittest import TestCase, skip
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.
from unittest import TestCase

import os
import sys
Expand All @@ -9,7 +11,6 @@
)


@skip('Flaky test and needs stabilization')
class TestLinuxConsumption(TestCase):
"""Test worker behaviors on specific scenarios.

Expand Down Expand Up @@ -134,6 +135,59 @@ def test_old_protobuf(self):
self.assertIn('grpc', content)
self.assertEqual(resp.status_code, 200)

def test_debug_logging_disabled(self):
"""An HttpTrigger function app with 'azure-functions' library
should return 200 and by default customer debug logging should be
disabled.
"""
with LinuxConsumptionWebHostController("3", self._py_version) as ctrl:
ctrl.assign_container(env={
"AzureWebJobsStorage": self._storage,
"SCM_RUN_FROM_PACKAGE": self._get_blob_url("EnableDebugLogging")
})
req = Request('GET', f'{ctrl.url}/api/HttpTrigger1')
resp = ctrl.send_request(req)

self.assertEqual(resp.status_code, 200)
container_log = ctrl.get_container_logs()
func_start_idx = container_log.find(
"Executing 'Functions.HttpTrigger1'")
self.assertTrue(func_start_idx > -1,
"HttpTrigger function is not executed.")
func_log = container_log[func_start_idx:]

self.assertIn('logging info', func_log)
self.assertIn('logging warning', func_log)
self.assertIn('logging error', func_log)
self.assertNotIn('logging debug', func_log)

def test_debug_logging_enabled(self):
"""An HttpTrigger function app with 'azure-functions' library
should return 200 and with customer debug logging enabled, debug logs
should be written to container logs.
"""
with LinuxConsumptionWebHostController("3", self._py_version) as ctrl:
ctrl.assign_container(env={
"AzureWebJobsStorage": self._storage,
"SCM_RUN_FROM_PACKAGE": self._get_blob_url(
"EnableDebugLogging"),
"PYTHON_ENABLE_DEBUG_LOGGING": "1"
})
req = Request('GET', f'{ctrl.url}/api/HttpTrigger1')
resp = ctrl.send_request(req)

self.assertEqual(resp.status_code, 200)
container_log = ctrl.get_container_logs()
func_start_idx = container_log.find(
"Executing 'Functions.HttpTrigger1'")
self.assertTrue(func_start_idx > -1)
func_log = container_log[func_start_idx:]

self.assertIn('logging info', func_log)
self.assertIn('logging warning', func_log)
self.assertIn('logging error', func_log)
self.assertIn('logging debug', func_log)

def _get_blob_url(self, scenario_name: str) -> str:
return (
f'https://pythonworker{self._py_shortform}sa.blob.core.windows.net/'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

import azure.functions


logger = logging.getLogger('my function')
logger.setLevel(logging.DEBUG)


def main(req: azure.functions.HttpRequest):
Expand Down
119 changes: 119 additions & 0 deletions tests/unittests/test_enable_debug_logging_functions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.
import typing
import os
from unittest.mock import patch

from azure_functions_worker import testutils
from azure_functions_worker.constants import PYTHON_ENABLE_DEBUG_LOGGING
from azure_functions_worker.testutils import TESTS_ROOT, remove_path

HOST_JSON_TEMPLATE_WITH_LOGLEVEL_INFO = """\
{
"version": "2.0",
"logging": {
"logLevel": {
"default": "Information"
}
},
"functionTimeout": "00:05:00"
}
"""


class TestDebugLoggingEnabledFunctions(testutils.WebHostTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@classmethod
def setUpClass(cls):
os_environ = os.environ.copy()
os_environ[PYTHON_ENABLE_DEBUG_LOGGING] = '1'
cls._patch_environ = patch.dict('os.environ', os_environ)
cls._patch_environ.start()
super().setUpClass()

@classmethod
def tearDownClass(self):
super().tearDownClass()
self._patch_environ.stop()

@classmethod
def get_script_dir(cls):
return testutils.UNIT_TESTS_FOLDER / 'log_filtering_functions'

def test_debug_logging_enabled(self):
r = self.webhost.request('GET', 'debug_logging')
self.assertEqual(r.status_code, 200)
self.assertEqual(r.text, 'OK-debug')

def check_log_debug_logging_enabled(self, host_out: typing.List[str]):
self.assertIn('logging info', host_out)
self.assertIn('logging warning', host_out)
self.assertIn('logging debug', host_out)
self.assertIn('logging error', host_out)


class TestDebugLoggingDisabledFunctions(testutils.WebHostTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add PyDoc comments to describe what the tests do? That'll help in the future.

@classmethod
def setUpClass(cls):
os_environ = os.environ.copy()
os_environ[PYTHON_ENABLE_DEBUG_LOGGING] = '0'
cls._patch_environ = patch.dict('os.environ', os_environ)
cls._patch_environ.start()
super().setUpClass()

@classmethod
def tearDownClass(self):
super().tearDownClass()
self._patch_environ.stop()

@classmethod
def get_script_dir(cls):
return testutils.UNIT_TESTS_FOLDER / 'log_filtering_functions'

def test_debug_logging_disabled(self):
r = self.webhost.request('GET', 'debug_logging')
self.assertEqual(r.status_code, 200)
self.assertEqual(r.text, 'OK-debug')

def check_log_debug_logging_disabled(self, host_out: typing.List[str]):
self.assertIn('logging info', host_out)
self.assertIn('logging warning', host_out)
self.assertIn('logging error', host_out)
self.assertNotIn('logging debug', host_out)


class TestLogFilteringFunctions(testutils.WebHostTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Can you please change the class name to describe what this test is specifically testing?

Copy link
Member

Choose a reason for hiding this comment

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

Also the same comments of comments.

@classmethod
def setUpClass(cls):
host_json = TESTS_ROOT / cls.get_script_dir() / 'host.json'

with open(host_json, 'w+') as f:
f.write(HOST_JSON_TEMPLATE_WITH_LOGLEVEL_INFO)

os_environ = os.environ.copy()
os_environ[PYTHON_ENABLE_DEBUG_LOGGING] = '1'
cls._patch_environ = patch.dict('os.environ', os_environ)
cls._patch_environ.start()
super().setUpClass()

@classmethod
def tearDownClass(self):
host_json = TESTS_ROOT / self.get_script_dir() / 'host.json'
remove_path(host_json)

super().tearDownClass()
self._patch_environ.stop()

@classmethod
def get_script_dir(cls):
return testutils.UNIT_TESTS_FOLDER / 'log_filtering_functions'

def test_debug_logging_filtered(self):
r = self.webhost.request('GET', 'debug_logging')
self.assertEqual(r.status_code, 200)
self.assertEqual(r.text, 'OK-debug')

def check_log_debug_logging_filtered(self, host_out: typing.List[str]):
self.assertIn('logging info', host_out)
self.assertIn('logging warning', host_out)
self.assertNotIn('logging debug', host_out)
self.assertIn('logging error', host_out)
15 changes: 1 addition & 14 deletions tests/unittests/test_http_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import filecmp
import typing
import os
import unittest
import pytest

from azure_functions_worker import testutils
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this is kept for testing flag unset case

r = self.webhost.request('GET', 'debug_logging')
self.assertEqual(r.status_code, 200)
self.assertEqual(r.text, 'OK-debug')

@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 check_log_debug_logging(self, host_out: typing.List[str]):
self.assertIn('logging info', host_out)
self.assertIn('logging warning', host_out)
self.assertIn('logging debug', host_out)
self.assertIn('logging error', host_out)
self.assertNotIn('logging debug', 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_with_user_logging(self):
r = self.webhost.request('GET', 'debug_user_logging')
self.assertEqual(r.status_code, 200)
self.assertEqual(r.text, 'OK-user-debug')

@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 check_log_debug_with_user_logging(self, host_out: typing.List[str]):
self.assertIn('logging info', host_out)
self.assertIn('logging warning', host_out)
Expand Down