From fd29ae08d36fe4cb65a317edb8b5a75e37c76b01 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Wed, 10 Jan 2024 17:36:17 -0800 Subject: [PATCH 1/9] Python to SW_APM_LOGNAME, Reporter to SW_APM_LOGNAME_ext --- solarwinds_apm/apm_config.py | 39 ++++++++++++++++++- solarwinds_apm/apm_logging.py | 35 +++++++++++++++++ tests/unit/test_apm_config/test_apm_config.py | 3 ++ .../test_apm_config_cnf_file.py | 21 ++++++++-- 4 files changed, 93 insertions(+), 5 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 7a636ecf..a003835a 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -135,9 +135,16 @@ def __init__( self.__config["service_key"], self.service_name, ) + + # Update and apply all logging settings + self.fix_logname() self.update_log_type() - # update logging level of Python logging logger + apm_logging.set_sw_log_type( + self.__config["log_type"], + self.__config["logname"], + ) apm_logging.set_sw_log_level(self.__config["debug_level"]) + self.update_logname_for_reporter() # Calculate c-lib extension usage ( @@ -463,6 +470,27 @@ def _update_service_key_name( # Else no need to update service_key when not reporting return service_key + def fix_logname( + self, + ) -> None: + """Checks SW_APM_LOGNAME path to file else fileHandler will fail. + If invalid, create path to match Boost.log behaviour. + If not possible, switch to default log settings. + """ + logname_path = os.path.dirname(self.__config["logname"]) + if not os.path.exists(logname_path): + try: + os.makedirs(logname_path) + logger.debug( + "Created directory path from provided SW_APM_LOGNAME." + ) + except FileNotFoundError: + logger.error( + "Could not create log file directory path from provided SW_APM_LOGNAME. Using default log settings." + ) + self.__config["logname"] = "" + self.__config["log_type"] = apm_logging.ApmLoggingType.default_type() + def update_log_type( self, ) -> None: @@ -478,6 +506,15 @@ def update_log_type( elif self.__config["logname"]: self.__config["log_type"] = apm_logging.ApmLoggingType.file_type() + def update_logname_for_reporter( + self, + ) -> None: + """Updates logname for extension Reporter to avoid conflict""" + orig_logname = self.__config["logname"] + if orig_logname: + logname, logname_ext = os.path.splitext(orig_logname) + self.__config["logname"] = f"{logname}_ext{logname_ext}" + def _calculate_metric_format(self) -> int: """Return one of 1 (TransactionResponseTime only) or 2 (default; ResponseTime only). Note: 0 (both) is no longer supported. Based on collector URL which may have a port e.g. foo-collector.com:443""" metric_format = 2 diff --git a/solarwinds_apm/apm_logging.py b/solarwinds_apm/apm_logging.py index 2bbc6b15..911f4f89 100644 --- a/solarwinds_apm/apm_logging.py +++ b/solarwinds_apm/apm_logging.py @@ -36,6 +36,7 @@ import logging import os +from logging.handlers import RotatingFileHandler class ApmLoggingType: @@ -188,6 +189,40 @@ def disable_logger(disable=True): logger.propagate = True +def set_sw_log_type(log_type, logname=""): + """Set the logging 'type' of the agent-internal logger. + + This function expects the log_type to be one of the integer + representations of the levels defined in ApmLoggingType.log_types. + If an invalid type has been provided, the logging handler will not be + changed but a warning message will be emitted. + + If logname is provided and log_type is file_type, logger stream handler + is swapped with a rotating file handler. + """ + if not ApmLoggingType.is_valid_log_type(log_type): + logger.warning( + "set_sw_log_type: Ignore attempt to set solarwinds_apm logger with invalid logging handler." + ) + return + + if log_type == ApmLoggingType.file_type() and logname: + try: + file_hander = RotatingFileHandler( + filename=logname, + maxBytes=1000000, # 1MB + backupCount=5, + ) + logger.addHandler(file_hander) + # stop logging to stream + logger.removeHandler(_stream_handler) + except FileNotFoundError: + # path should be checked first by ApmConfig.check_logname + logger.error( + "Could not write logs to file; please check configured SW_APM_LOGNAME." + ) + + def set_sw_log_level(level): """Set the logging level of the agent-internal logger to the provided level. This function expects the level to be one of the integer representations of the levels defined in ApmLoggingLevel.debug_levels. diff --git a/tests/unit/test_apm_config/test_apm_config.py b/tests/unit/test_apm_config/test_apm_config.py index c5b19742..f82c3a59 100644 --- a/tests/unit/test_apm_config/test_apm_config.py +++ b/tests/unit/test_apm_config/test_apm_config.py @@ -297,6 +297,9 @@ def test__init_oboe_api_and_options_configured_valid(self, mocker): mock_oboe_api_options_swig, ) ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + ) apm_config.SolarWindsApmConfig() mock_oboe_api_options_swig.assert_called_once() diff --git a/tests/unit/test_apm_config/test_apm_config_cnf_file.py b/tests/unit/test_apm_config/test_apm_config_cnf_file.py index 6e1f02bf..eb0584e4 100644 --- a/tests/unit/test_apm_config/test_apm_config_cnf_file.py +++ b/tests/unit/test_apm_config/test_apm_config_cnf_file.py @@ -103,6 +103,10 @@ def test_update_with_cnf_file_all_valid( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + ) + # use key from env var (Python APM only uses key from here), # agent enabled, nothing has errored resulting_config = apm_config.SolarWindsApmConfig() @@ -117,7 +121,7 @@ def test_update_with_cnf_file_all_valid( assert resulting_config.get("reporter") == "udp" assert resulting_config.get("debug_level") == 6 assert resulting_config.get("log_type") == 2 # because logname not none - assert resulting_config.get("logname") == "foo-bar" + assert resulting_config.get("logname") == "foo-bar_ext" assert resulting_config.get("hostname_alias") == "foo-bar" assert resulting_config.get("trustedpath") == "foo-bar" assert resulting_config.get("events_flush_interval") == 2 @@ -184,6 +188,9 @@ def test_update_with_cnf_file_mostly_invalid( mock_get_cnf_dict.configure_mock( return_value=mostly_invalid_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + ) # use key from env var (Python APM only uses key from here), # agent enabled, nothing has errored resulting_config = apm_config.SolarWindsApmConfig() @@ -212,7 +219,7 @@ def test_update_with_cnf_file_mostly_invalid( assert resulting_config.get("collector") == "False" assert resulting_config.get("hostname_alias") == "False" assert resulting_config.get("trustedpath") == "False" - assert resulting_config.get("logname") == "False" + assert resulting_config.get("logname") == "False_ext" # update_transaction_filters was called mock_update_txn_filters.assert_called_once_with(mostly_invalid_cnf_dict) @@ -264,6 +271,9 @@ def test_update_with_cnf_file_and_all_validenv_vars( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + ) resulting_config = apm_config.SolarWindsApmConfig() # update_transaction_filters was called mock_update_txn_filters.assert_called_once_with(fixture_cnf_dict) @@ -286,7 +296,7 @@ def test_update_with_cnf_file_and_all_validenv_vars( assert resulting_config.get("ec2_metadata_timeout") == 2222 assert resulting_config.get("max_flush_wait_time") == 3 assert resulting_config.get("max_transactions") == 3 - assert resulting_config.get("logname") == "other-foo-bar" + assert resulting_config.get("logname") == "other-foo-bar_ext" assert resulting_config.get("trace_metrics") == 3 assert resulting_config.get("token_bucket_capacity") == 3 assert resulting_config.get("token_bucket_rate") == 3 @@ -343,6 +353,9 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + ) resulting_config = apm_config.SolarWindsApmConfig() # update_transaction_filters was called mock_update_txn_filters.assert_called_once_with(fixture_cnf_dict) @@ -375,7 +388,7 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( assert resulting_config.get("collector") == "False" assert resulting_config.get("hostname_alias") == "False" assert resulting_config.get("trustedpath") == "False" - assert resulting_config.get("logname") == "False" + assert resulting_config.get("logname") == "False_ext" # Restore old collector if old_collector: From 08886c445ff4a77f10a7c023cf1265616a4e6a42 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Wed, 10 Jan 2024 17:44:05 -0800 Subject: [PATCH 2/9] lint --- solarwinds_apm/apm_config.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index a003835a..0ecfe235 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -489,7 +489,9 @@ def fix_logname( "Could not create log file directory path from provided SW_APM_LOGNAME. Using default log settings." ) self.__config["logname"] = "" - self.__config["log_type"] = apm_logging.ApmLoggingType.default_type() + self.__config[ + "log_type" + ] = apm_logging.ApmLoggingType.default_type() def update_log_type( self, From 3f1fa54e5b1881e642dfab2f94a99ad966df0866 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 11 Jan 2024 10:44:55 -0800 Subject: [PATCH 3/9] Add logname_for_reporter unit tests --- tests/unit/test_apm_config/test_apm_config.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/unit/test_apm_config/test_apm_config.py b/tests/unit/test_apm_config/test_apm_config.py index f82c3a59..d143b7bb 100644 --- a/tests/unit/test_apm_config/test_apm_config.py +++ b/tests/unit/test_apm_config/test_apm_config.py @@ -806,6 +806,30 @@ def test_update_log_type_logname_but_disabled(self): assert test_config.get("log_type") == 4 assert test_config.get("logname") == "some-file-path" + def test_update_logname_for_reporter_empty(self): + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "") + test_config.update_logname_for_reporter() + assert test_config.get("logname") == "" + + def test_update_logname_for_reporter_no_ext(self): + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "/path/to/foo") + test_config.update_logname_for_reporter() + assert test_config.get("logname") == "/path/to/foo_ext" + + def test_update_logname_for_reporter_ext(self): + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "/path/to/foo.log") + test_config.update_logname_for_reporter() + assert test_config.get("logname") == "/path/to/foo_ext.log" + + def test_update_logname_for_reporter_ext_multiple_dots(self): + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "/path/to/foo.abc.def.xyz") + test_config.update_logname_for_reporter() + assert test_config.get("logname") == "/path/to/foo.abc.def_ext.xyz" + def test_convert_to_bool_bool_true(self): test_config = apm_config.SolarWindsApmConfig() assert test_config.convert_to_bool(True) From 340e60152c91681905a088c48e07c2fe19e53bad Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 11 Jan 2024 14:06:57 -0800 Subject: [PATCH 4/9] no rollover to match oboe logs --- solarwinds_apm/apm_logging.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/solarwinds_apm/apm_logging.py b/solarwinds_apm/apm_logging.py index 911f4f89..36cbbe3e 100644 --- a/solarwinds_apm/apm_logging.py +++ b/solarwinds_apm/apm_logging.py @@ -208,10 +208,11 @@ def set_sw_log_type(log_type, logname=""): if log_type == ApmLoggingType.file_type() and logname: try: + # no rollover to match oboe logs file_hander = RotatingFileHandler( filename=logname, - maxBytes=1000000, # 1MB - backupCount=5, + maxBytes=0, + backupCount=0, ) logger.addHandler(file_hander) # stop logging to stream From 9b86ca6239f012fedc0c1f725aaa26e3a1380ba2 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 11 Jan 2024 16:27:13 -0800 Subject: [PATCH 5/9] update_logname handles filenotfounderror; add tests --- solarwinds_apm/apm_config.py | 44 ++++++---- tests/unit/test_apm_config/test_apm_config.py | 82 ++++++++++++++++++- .../test_apm_config_cnf_file.py | 8 +- 3 files changed, 111 insertions(+), 23 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 0ecfe235..37f787f3 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -136,14 +136,14 @@ def __init__( self.service_name, ) - # Update and apply all logging settings - self.fix_logname() - self.update_log_type() + # Update and apply logging settings to Python logger + self.update_log_settings() apm_logging.set_sw_log_type( self.__config["log_type"], self.__config["logname"], ) apm_logging.set_sw_log_level(self.__config["debug_level"]) + self.update_logname_for_reporter() # Calculate c-lib extension usage @@ -470,7 +470,14 @@ def _update_service_key_name( # Else no need to update service_key when not reporting return service_key - def fix_logname( + def update_log_settings( + self, + ) -> None: + """Update logname and log type""" + self.update_logname() + self.update_log_type() + + def update_logname( self, ) -> None: """Checks SW_APM_LOGNAME path to file else fileHandler will fail. @@ -478,20 +485,21 @@ def fix_logname( If not possible, switch to default log settings. """ logname_path = os.path.dirname(self.__config["logname"]) - if not os.path.exists(logname_path): - try: - os.makedirs(logname_path) - logger.debug( - "Created directory path from provided SW_APM_LOGNAME." - ) - except FileNotFoundError: - logger.error( - "Could not create log file directory path from provided SW_APM_LOGNAME. Using default log settings." - ) - self.__config["logname"] = "" - self.__config[ - "log_type" - ] = apm_logging.ApmLoggingType.default_type() + if logname_path: + if not os.path.exists(logname_path): + try: + os.makedirs(logname_path) + logger.debug( + "Created directory path from provided SW_APM_LOGNAME." + ) + except FileNotFoundError: + logger.error( + "Could not create log file directory path from provided SW_APM_LOGNAME. Using default log settings." + ) + self.__config["logname"] = "" + self.__config[ + "log_type" + ] = apm_logging.ApmLoggingType.default_type() def update_log_type( self, diff --git a/tests/unit/test_apm_config/test_apm_config.py b/tests/unit/test_apm_config/test_apm_config.py index d143b7bb..7e9a31d6 100644 --- a/tests/unit/test_apm_config/test_apm_config.py +++ b/tests/unit/test_apm_config/test_apm_config.py @@ -298,7 +298,7 @@ def test__init_oboe_api_and_options_configured_valid(self, mocker): ) ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" ) apm_config.SolarWindsApmConfig() @@ -766,6 +766,86 @@ def test__update_service_key_name_agent_enabled_and_service_name_ok(self): ) assert result == "valid_key_with:bar-service" + def test_update_log_settings(self, mocker): + mock_logname = mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + ) + mock_log_type = mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_type" + ) + # init includes update_log_settings() + apm_config.SolarWindsApmConfig() + mock_logname.assert_called_once() + mock_log_type.assert_called_once() + + def test_update_logname_none(self, mocker): + mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists") + mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") + + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "") + test_config._set_config_value("log_type", 2) + test_config.update_logname() + mock_exists.assert_not_called() + mock_makedirs.assert_not_called() + assert test_config.get("logname") == "" + assert test_config.get("log_type") == 2 + + def test_update_logname_no_parent_path(self, mocker): + mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists") + mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") + + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "foo") + test_config._set_config_value("log_type", 2) + test_config.update_logname() + mock_exists.assert_not_called() + mock_makedirs.assert_not_called() + assert test_config.get("logname") == "foo" + assert test_config.get("log_type") == 2 + + def test_update_logname_path_exists(self, mocker): + mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists", return_value=True) + mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") + + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "/path/to/foo") + test_config._set_config_value("log_type", 2) + test_config.update_logname() + mock_exists.assert_called_once_with("/path/to") + mock_makedirs.assert_not_called() + assert test_config.get("logname") == "/path/to/foo" + assert test_config.get("log_type") == 2 + + def test_update_logname_create_path(self, mocker): + mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists", return_value=False) + mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") + + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "/path/to/foo") + test_config._set_config_value("log_type", 2) + test_config.update_logname() + mock_exists.assert_called_once_with("/path/to") + mock_makedirs.assert_called_once_with("/path/to") + assert test_config.get("logname") == "/path/to/foo" + assert test_config.get("log_type") == 2 + + def test_update_logname_cannot_create_reset_settings(self, mocker): + mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists", return_value=False) + mock_makedirs = mocker.patch( + "solarwinds_apm.apm_config.os.makedirs", + side_effect=FileNotFoundError("mock error") + ) + + test_config = apm_config.SolarWindsApmConfig() + test_config._set_config_value("logname", "/path/to/foo") + test_config._set_config_value("log_type", 2) + test_config.update_logname() + mock_exists.assert_called_once_with("/path/to") + mock_makedirs.assert_called_once_with("/path/to") + assert test_config.get("logname") == "" + assert test_config.get("log_type") == 0 + def test_update_log_type_no_change(self): test_config = apm_config.SolarWindsApmConfig() test_config._set_config_value("debug_level", 2) diff --git a/tests/unit/test_apm_config/test_apm_config_cnf_file.py b/tests/unit/test_apm_config/test_apm_config_cnf_file.py index eb0584e4..8cd3bc03 100644 --- a/tests/unit/test_apm_config/test_apm_config_cnf_file.py +++ b/tests/unit/test_apm_config/test_apm_config_cnf_file.py @@ -104,7 +104,7 @@ def test_update_with_cnf_file_all_valid( return_value=fixture_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" ) # use key from env var (Python APM only uses key from here), @@ -189,7 +189,7 @@ def test_update_with_cnf_file_mostly_invalid( return_value=mostly_invalid_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" ) # use key from env var (Python APM only uses key from here), # agent enabled, nothing has errored @@ -272,7 +272,7 @@ def test_update_with_cnf_file_and_all_validenv_vars( return_value=fixture_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" ) resulting_config = apm_config.SolarWindsApmConfig() # update_transaction_filters was called @@ -354,7 +354,7 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( return_value=fixture_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.fix_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" ) resulting_config = apm_config.SolarWindsApmConfig() # update_transaction_filters was called From 5ce784a657202e169e374974256f99da7c96e8d5 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 11 Jan 2024 17:05:05 -0800 Subject: [PATCH 6/9] Add TestSetSwLog --- tests/unit/test_apm_logging.py | 95 ++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/tests/unit/test_apm_logging.py b/tests/unit/test_apm_logging.py index 8a09dd2d..2e6ba422 100644 --- a/tests/unit/test_apm_logging.py +++ b/tests/unit/test_apm_logging.py @@ -54,3 +54,98 @@ def test_is_valid_level_int_out_of_range(self): assert not apm_logging.ApmLoggingLevel.is_valid_level(-2) assert not apm_logging.ApmLoggingLevel.is_valid_level(7) assert not apm_logging.ApmLoggingLevel.is_valid_level(9999) + + +class TestSetSwLog: + def get_mock_logger_and_rfhandler( + self, + mocker, + error=False, + ): + mock_apm_logger = mocker.patch( + "solarwinds_apm.apm_logging.logger" + ) + mock_warning = mocker.Mock() + mock_error = mocker.Mock() + mock_addhandler = mocker.Mock() + mock_rmhandler = mocker.Mock() + mock_apm_logger.configure_mock( + **{ + "addHandler": mock_addhandler, + "error": mock_error, + "removeHandler": mock_rmhandler, + "warning": mock_warning, + } + ) + if error: + mock_rfhandler = mocker.patch( + "solarwinds_apm.apm_logging.RotatingFileHandler", + side_effect=FileNotFoundError("mock error") + ) + else: + mock_rfhandler = mocker.patch( + "solarwinds_apm.apm_logging.RotatingFileHandler" + ) + + return mock_apm_logger, mock_rfhandler + + def test_set_sw_log_type_invalid(self, mocker): + mock_apm_logger, mock_rfhandler = self.get_mock_logger_and_rfhandler(mocker) + + apm_logging.set_sw_log_type(9999, "foo") + mock_rfhandler.assert_not_called() + mock_apm_logger.addHandler.assert_not_called() + mock_apm_logger.removeHandler.assert_not_called() + mock_apm_logger.error.assert_not_called() + mock_apm_logger.warning.assert_called_once_with( + "set_sw_log_type: Ignore attempt to set solarwinds_apm logger with invalid logging handler." + ) + + def test_set_sw_log_type_not_file(self, mocker): + mock_apm_logger, mock_rfhandler = self.get_mock_logger_and_rfhandler(mocker) + + apm_logging.set_sw_log_type(0, "foo") + mock_rfhandler.assert_not_called() + mock_apm_logger.addHandler.assert_not_called() + mock_apm_logger.removeHandler.assert_not_called() + mock_apm_logger.error.assert_not_called() + mock_apm_logger.warning.assert_not_called() + + def test_set_sw_log_type_no_logname(self, mocker): + mock_apm_logger, mock_rfhandler = self.get_mock_logger_and_rfhandler(mocker) + + apm_logging.set_sw_log_type(2, "") + mock_rfhandler.assert_not_called() + mock_apm_logger.addHandler.assert_not_called() + mock_apm_logger.removeHandler.assert_not_called() + mock_apm_logger.error.assert_not_called() + mock_apm_logger.warning.assert_not_called() + + def test_set_sw_log_type_filenotfounderror(self, mocker): + mock_apm_logger, mock_rfhandler = self.get_mock_logger_and_rfhandler( + mocker, + error=True, + ) + + apm_logging.set_sw_log_type(2, "foo") + mock_rfhandler.assert_called_once_with( + filename='foo', maxBytes=0, backupCount=0 + ) + mock_apm_logger.addHandler.assert_not_called() + mock_apm_logger.removeHandler.assert_not_called() + mock_apm_logger.error.assert_called_once_with( + "Could not write logs to file; please check configured SW_APM_LOGNAME." + ) + mock_apm_logger.warning.assert_not_called() + + def test_set_sw_log_type_update_handlers(self, mocker): + mock_apm_logger, mock_rfhandler = self.get_mock_logger_and_rfhandler(mocker) + + apm_logging.set_sw_log_type(2, "foo") + mock_rfhandler.assert_called_once_with( + filename='foo', maxBytes=0, backupCount=0 + ) + mock_apm_logger.addHandler.assert_called_once() + mock_apm_logger.removeHandler.assert_called_once() + mock_apm_logger.error.assert_not_called() + mock_apm_logger.warning.assert_not_called() From 79c80d0dbf7811e3fe6046b0de8af1a8437c99f3 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 11 Jan 2024 18:32:54 -0800 Subject: [PATCH 7/9] Add patches to stop file creation during tests --- tests/unit/test_apm_config/test_apm_config.py | 2 +- .../test_apm_config_agent_enabled.py | 264 ++++++++++++++++++ .../test_apm_config_cnf_file.py | 52 +++- 3 files changed, 309 insertions(+), 9 deletions(-) diff --git a/tests/unit/test_apm_config/test_apm_config.py b/tests/unit/test_apm_config/test_apm_config.py index 7e9a31d6..0e2a1b13 100644 --- a/tests/unit/test_apm_config/test_apm_config.py +++ b/tests/unit/test_apm_config/test_apm_config.py @@ -298,7 +298,7 @@ def test__init_oboe_api_and_options_configured_valid(self, mocker): ) ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" ) apm_config.SolarWindsApmConfig() diff --git a/tests/unit/test_apm_config/test_apm_config_agent_enabled.py b/tests/unit/test_apm_config/test_apm_config_agent_enabled.py index 3463487f..7f67700e 100644 --- a/tests/unit/test_apm_config/test_apm_config_agent_enabled.py +++ b/tests/unit/test_apm_config/test_apm_config_agent_enabled.py @@ -33,6 +33,20 @@ def test_calculate_agent_enabled_service_key_missing(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) + resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -70,6 +84,20 @@ def test_calculate_agent_enabled_service_key_env_var_set_cnf_file_ignored( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) + resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config.get("service_key") == "valid:key-will-be-used" assert resulting_config.agent_enabled @@ -112,6 +140,20 @@ def test_calculate_agent_enabled_service_key_env_var_not_set_cnf_file_used( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) + resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config.get("service_key") == "not-good-to-put-here:still-could-be-used" assert resulting_config.agent_enabled @@ -137,6 +179,19 @@ def test_calculate_agent_enabled_service_key_bad_format(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -153,6 +208,19 @@ def test_calculate_agent_enabled_service_key_ok(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "key" @@ -170,6 +238,19 @@ def test_calculate_agent_enabled_env_var_true(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "key" @@ -187,6 +268,19 @@ def test_calculate_agent_enabled_env_var_false(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -204,6 +298,19 @@ def test_calculate_agent_enabled_env_var_false_mixed_case(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -231,6 +338,19 @@ def test_calculate_agent_enabled_env_var_not_set_cnf_file_false( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict_enabled_false ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config.agent_enabled assert resulting_config.service_name == "" @@ -258,6 +378,19 @@ def test_calculate_agent_enabled_env_var_not_set_cnf_file_false_mixed_case( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict_enabled_false_mixed_case ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config.agent_enabled assert resulting_config.service_name == "" @@ -285,6 +418,19 @@ def test_calculate_agent_enabled_env_var_true_cnf_file_false( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict_enabled_false ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config.agent_enabled assert resulting_config.service_name == "key" @@ -312,6 +458,20 @@ def test_calculate_agent_enabled_env_var_false_cnf_file_true( mock_get_cnf_dict.configure_mock( return_value=fixture_cnf_dict ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) + # cnf file fixture has "agentEnabled": True resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config.agent_enabled @@ -332,6 +492,19 @@ def test_calculate_agent_enabled_ok_all_env_vars(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "key" @@ -341,6 +514,19 @@ def test_calculate_agent_enabled_no_sw_propagator(self, mocker): "OTEL_PROPAGATORS": "tracecontext,baggage", "SW_APM_SERVICE_KEY": "valid:key", }) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -350,6 +536,19 @@ def test_calculate_agent_enabled_no_tracecontext_propagator(self, mocker): "OTEL_PROPAGATORS": "solarwinds_propagator", "SW_APM_SERVICE_KEY": "valid:key", }) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -359,6 +558,19 @@ def test_calculate_agent_enabled_sw_before_tracecontext_propagator(self, mocker) "OTEL_PROPAGATORS": "solarwinds_propagator,tracecontext", "SW_APM_SERVICE_KEY": "valid:key", }) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -368,6 +580,19 @@ def test_calculate_agent_enabled_sw_before_baggage_propagator(self, mocker): "OTEL_PROPAGATORS": "tracecontext,solarwinds_propagator,baggage", "SW_APM_SERVICE_KEY": "valid:key", }) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -377,6 +602,19 @@ def test_calculate_agent_enabled_sw_after_baggage_propagator(self, mocker): "OTEL_PROPAGATORS": "tracecontext,baggage,solarwinds_propagator", "SW_APM_SERVICE_KEY": "valid:key", }) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "key" @@ -392,6 +630,19 @@ def test_calculate_agent_enabled_sw_but_no_such_other_exporter(self, mocker): mock_iter_entry_points.configure_mock( side_effect=StopIteration("mock error") ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert not resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "" @@ -409,6 +660,19 @@ def test_calculate_agent_enabled_sw_and_two_other_valid_exporters(self, mocker): mock_iter_entry_points.configure_mock( return_value=mock_points ) + mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } + ) resulting_config = apm_config.SolarWindsApmConfig() assert resulting_config._calculate_agent_enabled() assert resulting_config.service_name == "key" diff --git a/tests/unit/test_apm_config/test_apm_config_cnf_file.py b/tests/unit/test_apm_config/test_apm_config_cnf_file.py index 8cd3bc03..1d9b2628 100644 --- a/tests/unit/test_apm_config/test_apm_config_cnf_file.py +++ b/tests/unit/test_apm_config/test_apm_config_cnf_file.py @@ -104,7 +104,17 @@ def test_update_with_cnf_file_all_valid( return_value=fixture_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } ) # use key from env var (Python APM only uses key from here), @@ -120,7 +130,6 @@ def test_update_with_cnf_file_all_valid( assert resulting_config.get("collector") == "foo-bar" assert resulting_config.get("reporter") == "udp" assert resulting_config.get("debug_level") == 6 - assert resulting_config.get("log_type") == 2 # because logname not none assert resulting_config.get("logname") == "foo-bar_ext" assert resulting_config.get("hostname_alias") == "foo-bar" assert resulting_config.get("trustedpath") == "foo-bar" @@ -189,7 +198,17 @@ def test_update_with_cnf_file_mostly_invalid( return_value=mostly_invalid_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } ) # use key from env var (Python APM only uses key from here), # agent enabled, nothing has errored @@ -202,7 +221,6 @@ def test_update_with_cnf_file_mostly_invalid( assert resulting_config.get("trigger_trace") == 1 assert resulting_config.get("reporter") == "" assert resulting_config.get("debug_level") == 2 - assert resulting_config.get("log_type") == 2 # because logname not none assert resulting_config.get("events_flush_interval") == -1 assert resulting_config.get("max_request_size_bytes") == -1 assert resulting_config.get("ec2_metadata_timeout") == 1000 @@ -272,7 +290,17 @@ def test_update_with_cnf_file_and_all_validenv_vars( return_value=fixture_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } ) resulting_config = apm_config.SolarWindsApmConfig() # update_transaction_filters was called @@ -288,7 +316,6 @@ def test_update_with_cnf_file_and_all_validenv_vars( assert resulting_config.get("collector") == "other-foo-bar" assert resulting_config.get("reporter") == "file" assert resulting_config.get("debug_level") == 5 - assert resulting_config.get("log_type") == 2 # because logname not none assert resulting_config.get("hostname_alias") == "other-foo-bar" assert resulting_config.get("trustedpath") == "other-foo-bar" assert resulting_config.get("events_flush_interval") == 3 @@ -354,7 +381,17 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( return_value=fixture_cnf_dict ) mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_settings" + ) + mock_apm_logging = mocker.patch( + "solarwinds_apm.apm_config.apm_logging" + ) + mock_apm_logging.configure_mock( + **{ + "set_sw_log_type": mocker.Mock(), + "set_sw_log_level": mocker.Mock(), + "ApmLoggingLevel.default_level": mocker.Mock(return_value=2) + } ) resulting_config = apm_config.SolarWindsApmConfig() # update_transaction_filters was called @@ -370,7 +407,6 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( assert resulting_config.get("trigger_trace") == 1 assert resulting_config.get("reporter") == "udp" assert resulting_config.get("debug_level") == 6 - assert resulting_config.get("log_type") == 2 # because logname not none assert resulting_config.get("events_flush_interval") == 2 assert resulting_config.get("max_request_size_bytes") == 2 assert resulting_config.get("ec2_metadata_timeout") == 1234 From 362221c6c02ccd8d5eab30d52970fd5cfc673bd6 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Tue, 16 Jan 2024 15:25:04 -0800 Subject: [PATCH 8/9] Update comment --- solarwinds_apm/apm_logging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solarwinds_apm/apm_logging.py b/solarwinds_apm/apm_logging.py index 36cbbe3e..3c12ffc4 100644 --- a/solarwinds_apm/apm_logging.py +++ b/solarwinds_apm/apm_logging.py @@ -218,7 +218,7 @@ def set_sw_log_type(log_type, logname=""): # stop logging to stream logger.removeHandler(_stream_handler) except FileNotFoundError: - # path should be checked first by ApmConfig.check_logname + # path should be checked first by ApmConfig.update_logname logger.error( "Could not write logs to file; please check configured SW_APM_LOGNAME." ) From e52366896163ffe2750f345a6fdf35b742fc373b Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Tue, 16 Jan 2024 15:52:05 -0800 Subject: [PATCH 9/9] Rename to SW_APM_LOG_FILEPATH --- solarwinds_apm/apm_config.py | 48 ++++----- solarwinds_apm/apm_logging.py | 12 +-- solarwinds_apm/configurator.py | 2 +- .../unit/test_apm_config/fixtures/cnf_dict.py | 6 +- tests/unit/test_apm_config/test_apm_config.py | 98 +++++++++---------- .../test_apm_config_cnf_file.py | 14 +-- tests/unit/test_apm_logging.py | 4 +- 7 files changed, 94 insertions(+), 90 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 37f787f3..3279aedf 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -97,7 +97,7 @@ def __init__( "reporter": "", # the reporter mode, either 'udp' or 'ssl'. "log_type": apm_logging.ApmLoggingType.default_type(), "debug_level": apm_logging.ApmLoggingLevel.default_level(), - "logname": "", + "log_filepath": "", "service_key": "", "hostname_alias": "", "trustedpath": "", @@ -140,11 +140,11 @@ def __init__( self.update_log_settings() apm_logging.set_sw_log_type( self.__config["log_type"], - self.__config["logname"], + self.__config["log_filepath"], ) apm_logging.set_sw_log_level(self.__config["debug_level"]) - self.update_logname_for_reporter() + self.update_log_filepath_for_reporter() # Calculate c-lib extension usage ( @@ -473,30 +473,30 @@ def _update_service_key_name( def update_log_settings( self, ) -> None: - """Update logname and log type""" - self.update_logname() + """Update log_filepath and log type""" + self.update_log_filepath() self.update_log_type() - def update_logname( + def update_log_filepath( self, ) -> None: - """Checks SW_APM_LOGNAME path to file else fileHandler will fail. + """Checks SW_APM_LOG_FILEPATH path to file else fileHandler will fail. If invalid, create path to match Boost.log behaviour. If not possible, switch to default log settings. """ - logname_path = os.path.dirname(self.__config["logname"]) - if logname_path: - if not os.path.exists(logname_path): + log_filepath = os.path.dirname(self.__config["log_filepath"]) + if log_filepath: + if not os.path.exists(log_filepath): try: - os.makedirs(logname_path) + os.makedirs(log_filepath) logger.debug( - "Created directory path from provided SW_APM_LOGNAME." + "Created directory path from provided SW_APM_LOG_FILEPATH." ) except FileNotFoundError: logger.error( - "Could not create log file directory path from provided SW_APM_LOGNAME. Using default log settings." + "Could not create log file directory path from provided SW_APM_LOG_FILEPATH. Using default log settings." ) - self.__config["logname"] = "" + self.__config["log_filepath"] = "" self.__config[ "log_type" ] = apm_logging.ApmLoggingType.default_type() @@ -507,23 +507,27 @@ def update_log_type( """Updates agent log type depending on other configs. SW_APM_DEBUG_LEVEL -1 will set c-lib log_type to disabled (4). - SW_APM_LOGNAME not None will set c-lib log_type to file (2). + SW_APM_LOG_FILEPATH not None will set c-lib log_type to file (2). """ if self.__config["debug_level"] == -1: self.__config[ "log_type" ] = apm_logging.ApmLoggingType.disabled_type() - elif self.__config["logname"]: + elif self.__config["log_filepath"]: self.__config["log_type"] = apm_logging.ApmLoggingType.file_type() - def update_logname_for_reporter( + def update_log_filepath_for_reporter( self, ) -> None: - """Updates logname for extension Reporter to avoid conflict""" - orig_logname = self.__config["logname"] - if orig_logname: - logname, logname_ext = os.path.splitext(orig_logname) - self.__config["logname"] = f"{logname}_ext{logname_ext}" + """Updates log_filepath for extension Reporter to avoid conflict""" + orig_log_filepath = self.__config["log_filepath"] + if orig_log_filepath: + log_filepath, log_filepath_ext = os.path.splitext( + orig_log_filepath + ) + self.__config[ + "log_filepath" + ] = f"{log_filepath}_ext{log_filepath_ext}" def _calculate_metric_format(self) -> int: """Return one of 1 (TransactionResponseTime only) or 2 (default; ResponseTime only). Note: 0 (both) is no longer supported. Based on collector URL which may have a port e.g. foo-collector.com:443""" diff --git a/solarwinds_apm/apm_logging.py b/solarwinds_apm/apm_logging.py index 3c12ffc4..f90bc184 100644 --- a/solarwinds_apm/apm_logging.py +++ b/solarwinds_apm/apm_logging.py @@ -189,7 +189,7 @@ def disable_logger(disable=True): logger.propagate = True -def set_sw_log_type(log_type, logname=""): +def set_sw_log_type(log_type, log_filepath=""): """Set the logging 'type' of the agent-internal logger. This function expects the log_type to be one of the integer @@ -197,7 +197,7 @@ def set_sw_log_type(log_type, logname=""): If an invalid type has been provided, the logging handler will not be changed but a warning message will be emitted. - If logname is provided and log_type is file_type, logger stream handler + If log_filepath is provided and log_type is file_type, logger stream handler is swapped with a rotating file handler. """ if not ApmLoggingType.is_valid_log_type(log_type): @@ -206,11 +206,11 @@ def set_sw_log_type(log_type, logname=""): ) return - if log_type == ApmLoggingType.file_type() and logname: + if log_type == ApmLoggingType.file_type() and log_filepath: try: # no rollover to match oboe logs file_hander = RotatingFileHandler( - filename=logname, + filename=log_filepath, maxBytes=0, backupCount=0, ) @@ -218,9 +218,9 @@ def set_sw_log_type(log_type, logname=""): # stop logging to stream logger.removeHandler(_stream_handler) except FileNotFoundError: - # path should be checked first by ApmConfig.update_logname + # path should be checked first by ApmConfig.update_log_filepath logger.error( - "Could not write logs to file; please check configured SW_APM_LOGNAME." + "Could not write logs to file; please check configured SW_APM_LOG_FILEPATH." ) diff --git a/solarwinds_apm/configurator.py b/solarwinds_apm/configurator.py index 7fc85178..3c0ac751 100644 --- a/solarwinds_apm/configurator.py +++ b/solarwinds_apm/configurator.py @@ -465,7 +465,7 @@ def _initialize_solarwinds_reporter( "hostname_alias": apm_config.get("hostname_alias"), "log_type": apm_config.get("log_type"), "log_level": apm_config.get("debug_level"), - "log_file_path": apm_config.get("logname"), + "log_file_path": apm_config.get("log_filepath"), "max_transactions": apm_config.get("max_transactions"), "max_flush_wait_time": apm_config.get("max_flush_wait_time"), "events_flush_interval": apm_config.get("events_flush_interval"), diff --git a/tests/unit/test_apm_config/fixtures/cnf_dict.py b/tests/unit/test_apm_config/fixtures/cnf_dict.py index 99506799..b2dc26ff 100644 --- a/tests/unit/test_apm_config/fixtures/cnf_dict.py +++ b/tests/unit/test_apm_config/fixtures/cnf_dict.py @@ -9,7 +9,7 @@ def fixture_cnf_dict(): "collector": "foo-bar", "reporter": "udp", "debugLevel": 6, - "logname": "foo-bar", + "logFilepath": "foo-bar", "serviceKey": "not-good-to-put-here:still-could-be-used", "hostnameAlias": "foo-bar", "trustedpath": "foo-bar", @@ -37,7 +37,7 @@ def fixture_cnf_dict_enabled_false(): "collector": "foo-bar", "reporter": "udp", "debugLevel": 6, - "logname": "foo-bar", + "logFilepath": "foo-bar", "serviceKey": "not-good-to-put-here:still-could-be-used", "hostnameAlias": "foo-bar", "trustedpath": "foo-bar", @@ -65,7 +65,7 @@ def fixture_cnf_dict_enabled_false_mixed_case(): "collector": "foo-bar", "reporter": "udp", "debugLevel": 6, - "logname": "foo-bar", + "logFilepath": "foo-bar", "serviceKey": "not-good-to-put-here:still-could-be-used", "hostnameAlias": "foo-bar", "trustedpath": "foo-bar", diff --git a/tests/unit/test_apm_config/test_apm_config.py b/tests/unit/test_apm_config/test_apm_config.py index 0e2a1b13..258feae7 100644 --- a/tests/unit/test_apm_config/test_apm_config.py +++ b/tests/unit/test_apm_config/test_apm_config.py @@ -767,70 +767,70 @@ def test__update_service_key_name_agent_enabled_and_service_name_ok(self): assert result == "valid_key_with:bar-service" def test_update_log_settings(self, mocker): - mock_logname = mocker.patch( - "solarwinds_apm.apm_config.SolarWindsApmConfig.update_logname" + mock_log_filepath = mocker.patch( + "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_filepath" ) mock_log_type = mocker.patch( "solarwinds_apm.apm_config.SolarWindsApmConfig.update_log_type" ) # init includes update_log_settings() apm_config.SolarWindsApmConfig() - mock_logname.assert_called_once() + mock_log_filepath.assert_called_once() mock_log_type.assert_called_once() - def test_update_logname_none(self, mocker): + def test_update_log_filepath_none(self, mocker): mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists") mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "") + test_config._set_config_value("log_filepath", "") test_config._set_config_value("log_type", 2) - test_config.update_logname() + test_config.update_log_filepath() mock_exists.assert_not_called() mock_makedirs.assert_not_called() - assert test_config.get("logname") == "" + assert test_config.get("log_filepath") == "" assert test_config.get("log_type") == 2 - def test_update_logname_no_parent_path(self, mocker): + def test_update_log_filepath_no_parent_path(self, mocker): mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists") mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "foo") + test_config._set_config_value("log_filepath", "foo") test_config._set_config_value("log_type", 2) - test_config.update_logname() + test_config.update_log_filepath() mock_exists.assert_not_called() mock_makedirs.assert_not_called() - assert test_config.get("logname") == "foo" + assert test_config.get("log_filepath") == "foo" assert test_config.get("log_type") == 2 - def test_update_logname_path_exists(self, mocker): + def test_update_log_filepath_path_exists(self, mocker): mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists", return_value=True) mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "/path/to/foo") + test_config._set_config_value("log_filepath", "/path/to/foo") test_config._set_config_value("log_type", 2) - test_config.update_logname() + test_config.update_log_filepath() mock_exists.assert_called_once_with("/path/to") mock_makedirs.assert_not_called() - assert test_config.get("logname") == "/path/to/foo" + assert test_config.get("log_filepath") == "/path/to/foo" assert test_config.get("log_type") == 2 - def test_update_logname_create_path(self, mocker): + def test_update_log_filepath_create_path(self, mocker): mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists", return_value=False) mock_makedirs = mocker.patch("solarwinds_apm.apm_config.os.makedirs") test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "/path/to/foo") + test_config._set_config_value("log_filepath", "/path/to/foo") test_config._set_config_value("log_type", 2) - test_config.update_logname() + test_config.update_log_filepath() mock_exists.assert_called_once_with("/path/to") mock_makedirs.assert_called_once_with("/path/to") - assert test_config.get("logname") == "/path/to/foo" + assert test_config.get("log_filepath") == "/path/to/foo" assert test_config.get("log_type") == 2 - def test_update_logname_cannot_create_reset_settings(self, mocker): + def test_update_log_filepath_cannot_create_reset_settings(self, mocker): mock_exists = mocker.patch("solarwinds_apm.apm_config.os.path.exists", return_value=False) mock_makedirs = mocker.patch( "solarwinds_apm.apm_config.os.makedirs", @@ -838,77 +838,77 @@ def test_update_logname_cannot_create_reset_settings(self, mocker): ) test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "/path/to/foo") + test_config._set_config_value("log_filepath", "/path/to/foo") test_config._set_config_value("log_type", 2) - test_config.update_logname() + test_config.update_log_filepath() mock_exists.assert_called_once_with("/path/to") mock_makedirs.assert_called_once_with("/path/to") - assert test_config.get("logname") == "" + assert test_config.get("log_filepath") == "" assert test_config.get("log_type") == 0 def test_update_log_type_no_change(self): test_config = apm_config.SolarWindsApmConfig() test_config._set_config_value("debug_level", 2) test_config._set_config_value("log_type", 0) - test_config._set_config_value("logname", "") + test_config._set_config_value("log_filepath", "") test_config.update_log_type() assert test_config.get("debug_level") == 2 assert test_config.get("log_type") == 0 - assert test_config.get("logname") == "" + assert test_config.get("log_filepath") == "" def test_update_log_type_disabled(self): test_config = apm_config.SolarWindsApmConfig() test_config._set_config_value("debug_level", -1) test_config._set_config_value("log_type", 0) - test_config._set_config_value("logname", "") + test_config._set_config_value("log_filepath", "") test_config.update_log_type() assert test_config.get("debug_level") == -1 assert test_config.get("log_type") == 4 - assert test_config.get("logname") == "" + assert test_config.get("log_filepath") == "" - def test_update_log_type_logname(self): + def test_update_log_type_log_filepath(self): test_config = apm_config.SolarWindsApmConfig() test_config._set_config_value("debug_level", 1) test_config._set_config_value("log_type", 0) - test_config._set_config_value("logname", "some-file-path") + test_config._set_config_value("log_filepath", "some-file-path") test_config.update_log_type() assert test_config.get("debug_level") == 1 assert test_config.get("log_type") == 2 - assert test_config.get("logname") == "some-file-path" + assert test_config.get("log_filepath") == "some-file-path" - def test_update_log_type_logname_but_disabled(self): + def test_update_log_type_log_filepath_but_disabled(self): test_config = apm_config.SolarWindsApmConfig() test_config._set_config_value("debug_level", -1) test_config._set_config_value("log_type", 0) - test_config._set_config_value("logname", "some-file-path") + test_config._set_config_value("log_filepath", "some-file-path") test_config.update_log_type() assert test_config.get("debug_level") == -1 assert test_config.get("log_type") == 4 - assert test_config.get("logname") == "some-file-path" + assert test_config.get("log_filepath") == "some-file-path" - def test_update_logname_for_reporter_empty(self): + def test_update_log_filepath_for_reporter_empty(self): test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "") - test_config.update_logname_for_reporter() - assert test_config.get("logname") == "" + test_config._set_config_value("log_filepath", "") + test_config.update_log_filepath_for_reporter() + assert test_config.get("log_filepath") == "" - def test_update_logname_for_reporter_no_ext(self): + def test_update_log_filepath_for_reporter_no_ext(self): test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "/path/to/foo") - test_config.update_logname_for_reporter() - assert test_config.get("logname") == "/path/to/foo_ext" + test_config._set_config_value("log_filepath", "/path/to/foo") + test_config.update_log_filepath_for_reporter() + assert test_config.get("log_filepath") == "/path/to/foo_ext" - def test_update_logname_for_reporter_ext(self): + def test_update_log_filepath_for_reporter_ext(self): test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "/path/to/foo.log") - test_config.update_logname_for_reporter() - assert test_config.get("logname") == "/path/to/foo_ext.log" + test_config._set_config_value("log_filepath", "/path/to/foo.log") + test_config.update_log_filepath_for_reporter() + assert test_config.get("log_filepath") == "/path/to/foo_ext.log" - def test_update_logname_for_reporter_ext_multiple_dots(self): + def test_update_log_filepath_for_reporter_ext_multiple_dots(self): test_config = apm_config.SolarWindsApmConfig() - test_config._set_config_value("logname", "/path/to/foo.abc.def.xyz") - test_config.update_logname_for_reporter() - assert test_config.get("logname") == "/path/to/foo.abc.def_ext.xyz" + test_config._set_config_value("log_filepath", "/path/to/foo.abc.def.xyz") + test_config.update_log_filepath_for_reporter() + assert test_config.get("log_filepath") == "/path/to/foo.abc.def_ext.xyz" def test_convert_to_bool_bool_true(self): test_config = apm_config.SolarWindsApmConfig() diff --git a/tests/unit/test_apm_config/test_apm_config_cnf_file.py b/tests/unit/test_apm_config/test_apm_config_cnf_file.py index 1d9b2628..debc90e2 100644 --- a/tests/unit/test_apm_config/test_apm_config_cnf_file.py +++ b/tests/unit/test_apm_config/test_apm_config_cnf_file.py @@ -130,7 +130,7 @@ def test_update_with_cnf_file_all_valid( assert resulting_config.get("collector") == "foo-bar" assert resulting_config.get("reporter") == "udp" assert resulting_config.get("debug_level") == 6 - assert resulting_config.get("logname") == "foo-bar_ext" + assert resulting_config.get("log_filepath") == "foo-bar_ext" assert resulting_config.get("hostname_alias") == "foo-bar" assert resulting_config.get("trustedpath") == "foo-bar" assert resulting_config.get("events_flush_interval") == 2 @@ -174,7 +174,7 @@ def test_update_with_cnf_file_mostly_invalid( "collector": False, "reporter": "not-ssl-or-anything", "debugLevel": "foo", - "logname": False, + "logFilepath": False, "serviceKey": "not-good-to-put-here-and-wont-be-used", "hostnameAlias": False, "trustedpath": False, @@ -237,7 +237,7 @@ def test_update_with_cnf_file_mostly_invalid( assert resulting_config.get("collector") == "False" assert resulting_config.get("hostname_alias") == "False" assert resulting_config.get("trustedpath") == "False" - assert resulting_config.get("logname") == "False_ext" + assert resulting_config.get("log_filepath") == "False_ext" # update_transaction_filters was called mock_update_txn_filters.assert_called_once_with(mostly_invalid_cnf_dict) @@ -264,7 +264,7 @@ def test_update_with_cnf_file_and_all_validenv_vars( "SW_APM_COLLECTOR": "other-foo-bar", "SW_APM_REPORTER": "file", "SW_APM_DEBUG_LEVEL": "5", - "SW_APM_LOGNAME": "other-foo-bar", + "SW_APM_LOG_FILEPATH": "other-foo-bar", "SW_APM_HOSTNAME_ALIAS": "other-foo-bar", "SW_APM_TRUSTEDPATH": "other-foo-bar", "SW_APM_EVENTS_FLUSH_INTERVAL": "3", @@ -323,7 +323,7 @@ def test_update_with_cnf_file_and_all_validenv_vars( assert resulting_config.get("ec2_metadata_timeout") == 2222 assert resulting_config.get("max_flush_wait_time") == 3 assert resulting_config.get("max_transactions") == 3 - assert resulting_config.get("logname") == "other-foo-bar_ext" + assert resulting_config.get("log_filepath") == "other-foo-bar_ext" assert resulting_config.get("trace_metrics") == 3 assert resulting_config.get("token_bucket_capacity") == 3 assert resulting_config.get("token_bucket_rate") == 3 @@ -355,7 +355,7 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( "SW_APM_COLLECTOR": "False", "SW_APM_REPORTER": "other-foo-bar", "SW_APM_DEBUG_LEVEL": "other-foo-bar", - "SW_APM_LOGNAME": "False", + "SW_APM_LOG_FILEPATH": "False", "SW_APM_HOSTNAME_ALIAS": "False", "SW_APM_TRUSTEDPATH": "False", "SW_APM_EVENTS_FLUSH_INTERVAL": "other-foo-bar", @@ -424,7 +424,7 @@ def test_update_with_cnf_file_and_several_invalid_env_vars( assert resulting_config.get("collector") == "False" assert resulting_config.get("hostname_alias") == "False" assert resulting_config.get("trustedpath") == "False" - assert resulting_config.get("logname") == "False_ext" + assert resulting_config.get("log_filepath") == "False_ext" # Restore old collector if old_collector: diff --git a/tests/unit/test_apm_logging.py b/tests/unit/test_apm_logging.py index 2e6ba422..7510211f 100644 --- a/tests/unit/test_apm_logging.py +++ b/tests/unit/test_apm_logging.py @@ -111,7 +111,7 @@ def test_set_sw_log_type_not_file(self, mocker): mock_apm_logger.error.assert_not_called() mock_apm_logger.warning.assert_not_called() - def test_set_sw_log_type_no_logname(self, mocker): + def test_set_sw_log_type_no_log_filepath(self, mocker): mock_apm_logger, mock_rfhandler = self.get_mock_logger_and_rfhandler(mocker) apm_logging.set_sw_log_type(2, "") @@ -134,7 +134,7 @@ def test_set_sw_log_type_filenotfounderror(self, mocker): mock_apm_logger.addHandler.assert_not_called() mock_apm_logger.removeHandler.assert_not_called() mock_apm_logger.error.assert_called_once_with( - "Could not write logs to file; please check configured SW_APM_LOGNAME." + "Could not write logs to file; please check configured SW_APM_LOG_FILEPATH." ) mock_apm_logger.warning.assert_not_called()