From 199c52e3ae7c8b87afc89c969d0c4490c6f5c01e Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Jul 2023 16:07:25 -0700 Subject: [PATCH 1/5] load_instrumentor sets commenter_options in kwargs --- solarwinds_apm/distro.py | 43 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/solarwinds_apm/distro.py b/solarwinds_apm/distro.py index f76526cb..92414c64 100644 --- a/solarwinds_apm/distro.py +++ b/solarwinds_apm/distro.py @@ -6,6 +6,7 @@ """Module to configure OpenTelemetry to work with SolarWinds backend""" +import logging from os import environ from opentelemetry.environment_variables import ( @@ -24,6 +25,8 @@ INTL_SWO_DEFAULT_TRACES_EXPORTER, ) +logger = logging.getLogger(__name__) + class SolarWindsDistro(BaseDistro): """OpenTelemetry Distro for SolarWinds reporting environment""" @@ -54,6 +57,12 @@ def load_instrumentor(self, entry_point: EntryPoint, **kwargs): kwargs["enable_commenter"] = True # instrumentation for Django ORM kwargs["is_sql_commentor_enabled"] = True + + # Assumes can be empty and any KVs not used + # by current library are ignored + # Note: Django ORM accepts options in settings.py + # https://opentelemetry-python-contrib.readthedocs.io/en/latest/instrumentation/django/django.html + kwargs["commenter_options"] = self.detect_commenter_options() instrumentor: BaseInstrumentor = entry_point.load() instrumentor().instrument(**kwargs) @@ -65,3 +74,37 @@ def enable_commenter(self, entry_point: EntryPoint, **kwargs) -> bool: if enable_commenter.lower() == "true": return True return False + + def detect_commenter_options(self): + """Returns commenter options dict parsed from environment, if any""" + commenter_opts = {} + commenter_opts_env = environ.get("OTEL_SQLCOMMENTER_OPTIONS") + if commenter_opts_env: + for opt_item in commenter_opts_env.split(","): + opt_k = "" + opt_v = "" + try: + opt_k, opt_v = opt_item.split("=", maxsplit=1) + except ValueError as exc: + logger.warning( + "Invalid key-value pair for sqlcommenter option %s: %s", + opt_item, + exc, + ) + opt_v_bool = self._convert_to_bool(opt_v.strip()) + if opt_v_bool is not None: + commenter_opts[opt_k.strip()] = opt_v_bool + + return commenter_opts + + # TODO Refactor as this also exists in ApmConfig + def _convert_to_bool(self, val): + """Converts given value to boolean value if bool or str representation, else None""" + if isinstance(val, bool): + return val + if isinstance(val, str): + if val.lower() == "true": + return True + if val.lower() == "false": + return False + return None From 98f9582e4e32d07b72f0b8dc4953502968b44eb4 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Jul 2023 15:34:18 -0700 Subject: [PATCH 2/5] _convert_to_bool to classmethod --- solarwinds_apm/apm_config.py | 3 ++- solarwinds_apm/distro.py | 15 ++------------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index bfb35d0e..1cf9b50b 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -608,7 +608,8 @@ def update_with_kwargs(self, kwargs): """Update the configuration settings with (in-code) keyword arguments""" # TODO Implement in-code config with kwargs after alpha - def _convert_to_bool(self, val): + @classmethod + def _convert_to_bool(cls, val): """Converts given value to boolean value if bool or str representation, else None""" if isinstance(val, bool): return val diff --git a/solarwinds_apm/distro.py b/solarwinds_apm/distro.py index 92414c64..f52b39a8 100644 --- a/solarwinds_apm/distro.py +++ b/solarwinds_apm/distro.py @@ -24,6 +24,7 @@ INTL_SWO_DEFAULT_PROPAGATORS, INTL_SWO_DEFAULT_TRACES_EXPORTER, ) +from solarwinds_apm.apm_config import SolarWindsApmConfig logger = logging.getLogger(__name__) @@ -91,20 +92,8 @@ def detect_commenter_options(self): opt_item, exc, ) - opt_v_bool = self._convert_to_bool(opt_v.strip()) + opt_v_bool = SolarWindsApmConfig._convert_to_bool(opt_v.strip()) if opt_v_bool is not None: commenter_opts[opt_k.strip()] = opt_v_bool return commenter_opts - - # TODO Refactor as this also exists in ApmConfig - def _convert_to_bool(self, val): - """Converts given value to boolean value if bool or str representation, else None""" - if isinstance(val, bool): - return val - if isinstance(val, str): - if val.lower() == "true": - return True - if val.lower() == "false": - return False - return None From 130a1c188bba237f52484ea92cba251e8ca8ddb8 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Jul 2023 15:40:12 -0700 Subject: [PATCH 3/5] Rename convert_to_bool --- solarwinds_apm/apm_config.py | 8 ++--- solarwinds_apm/distro.py | 4 +-- tests/unit/test_apm_config/test_apm_config.py | 32 +++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 1cf9b50b..c82db43c 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -506,7 +506,7 @@ def _snake_to_camel_case(key): return # agent_enabled is special - cnf_agent_enabled = self._convert_to_bool( + cnf_agent_enabled = self.convert_to_bool( cnf_dict.get(_snake_to_camel_case("agent_enabled")) ) if cnf_agent_enabled is not None: @@ -587,7 +587,7 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: def update_with_env_var(self) -> None: """Update the settings with environment variables.""" # agent_enabled is special - env_agent_enabled = self._convert_to_bool( + env_agent_enabled = self.convert_to_bool( os.environ.get("SW_APM_AGENT_ENABLED") ) if env_agent_enabled is not None: @@ -609,7 +609,7 @@ def update_with_kwargs(self, kwargs): # TODO Implement in-code config with kwargs after alpha @classmethod - def _convert_to_bool(cls, val): + def convert_to_bool(cls, val): """Converts given value to boolean value if bool or str representation, else None""" if isinstance(val, bool): return val @@ -698,7 +698,7 @@ def _set_config_value(self, keys_str: str, val: Any) -> Any: apm_logging.set_sw_log_level(val) elif isinstance(sub_dict, dict) and keys[-1] in sub_dict: if isinstance(sub_dict[keys[-1]], bool): - val = self._convert_to_bool(val) + val = self.convert_to_bool(val) else: val = type(sub_dict[keys[-1]])(val) sub_dict[keys[-1]] = val diff --git a/solarwinds_apm/distro.py b/solarwinds_apm/distro.py index f52b39a8..bac8637e 100644 --- a/solarwinds_apm/distro.py +++ b/solarwinds_apm/distro.py @@ -20,11 +20,11 @@ ) from pkg_resources import EntryPoint +from solarwinds_apm.apm_config import SolarWindsApmConfig from solarwinds_apm.apm_constants import ( INTL_SWO_DEFAULT_PROPAGATORS, INTL_SWO_DEFAULT_TRACES_EXPORTER, ) -from solarwinds_apm.apm_config import SolarWindsApmConfig logger = logging.getLogger(__name__) @@ -92,7 +92,7 @@ def detect_commenter_options(self): opt_item, exc, ) - opt_v_bool = SolarWindsApmConfig._convert_to_bool(opt_v.strip()) + opt_v_bool = SolarWindsApmConfig.convert_to_bool(opt_v.strip()) if opt_v_bool is not None: commenter_opts[opt_k.strip()] = opt_v_bool diff --git a/tests/unit/test_apm_config/test_apm_config.py b/tests/unit/test_apm_config/test_apm_config.py index f9b21f8d..6288fb38 100644 --- a/tests/unit/test_apm_config/test_apm_config.py +++ b/tests/unit/test_apm_config/test_apm_config.py @@ -591,34 +591,34 @@ def test__update_service_key_name_agent_enabled_and_service_name_ok(self): ) assert result == "valid_key_with:bar-service" - def test__convert_to_bool_bool_true(self): + def test_convert_to_bool_bool_true(self): test_config = apm_config.SolarWindsApmConfig() - assert test_config._convert_to_bool(True) + assert test_config.convert_to_bool(True) - def test__convert_to_bool_bool_false(self): + def test_convert_to_bool_bool_false(self): test_config = apm_config.SolarWindsApmConfig() - assert not test_config._convert_to_bool(False) + assert not test_config.convert_to_bool(False) - def test__convert_to_bool_int(self): + def test_convert_to_bool_int(self): test_config = apm_config.SolarWindsApmConfig() - assert test_config._convert_to_bool(0) is None + assert test_config.convert_to_bool(0) is None - def test__convert_to_bool_str_invalid(self): + def test_convert_to_bool_str_invalid(self): test_config = apm_config.SolarWindsApmConfig() - assert test_config._convert_to_bool("not-true-nor-false") is None + assert test_config.convert_to_bool("not-true-nor-false") is None - def test__convert_to_bool_str_true(self): + def test_convert_to_bool_str_true(self): test_config = apm_config.SolarWindsApmConfig() - assert test_config._convert_to_bool("true") + assert test_config.convert_to_bool("true") - def test__convert_to_bool_str_true_mixed_case(self): + def test_convert_to_bool_str_true_mixed_case(self): test_config = apm_config.SolarWindsApmConfig() - assert test_config._convert_to_bool("tRuE") + assert test_config.convert_to_bool("tRuE") - def test__convert_to_bool_str_false(self): + def test_convert_to_bool_str_false(self): test_config = apm_config.SolarWindsApmConfig() - assert not test_config._convert_to_bool("false") + assert not test_config.convert_to_bool("false") - def test__convert_to_bool_str_false_mixed_case(self): + def test_convert_to_bool_str_false_mixed_case(self): test_config = apm_config.SolarWindsApmConfig() - assert not test_config._convert_to_bool("fAlSE") + assert not test_config.convert_to_bool("fAlSE") From e62df1a8e17eab5ec27787875eae06baf0805eaa Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Jul 2023 16:02:07 -0700 Subject: [PATCH 4/5] Rm unused args from enable_commenter --- solarwinds_apm/distro.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/solarwinds_apm/distro.py b/solarwinds_apm/distro.py index bac8637e..ce06c285 100644 --- a/solarwinds_apm/distro.py +++ b/solarwinds_apm/distro.py @@ -53,7 +53,7 @@ def load_instrumentor(self, entry_point: EntryPoint, **kwargs): """ # Set enable for sqlcommenter. Assumes kwargs ignored if not # implemented for current instrumentation library - if self.enable_commenter(entry_point, **kwargs): + if self.enable_commenter(): # instrumentation for Flask ORM, sqlalchemy, psycopg2 kwargs["enable_commenter"] = True # instrumentation for Django ORM @@ -67,7 +67,7 @@ def load_instrumentor(self, entry_point: EntryPoint, **kwargs): instrumentor: BaseInstrumentor = entry_point.load() instrumentor().instrument(**kwargs) - def enable_commenter(self, entry_point: EntryPoint, **kwargs) -> bool: + def enable_commenter(self) -> bool: """Enable sqlcommenter feature, if implemented""" # TODO: Update if changed in OTel spec: # https://github.com/open-telemetry/opentelemetry-specification/issues/3560 From b7dfef02fbb63fb4662a3f8fe8b07030f1625cd1 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Jul 2023 16:42:39 -0700 Subject: [PATCH 5/5] Add distro unit tests --- tests/unit/test_distro.py | 115 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/unit/test_distro.py b/tests/unit/test_distro.py index 3c9cadb5..09b2677b 100644 --- a/tests/unit/test_distro.py +++ b/tests/unit/test_distro.py @@ -32,3 +32,118 @@ def test_configure_env_propagators(self, mocker): SolarWindsDistro()._configure() assert os.environ[OTEL_PROPAGATORS] == "tracecontext,solarwinds_propagator,foobar" assert os.environ[OTEL_TRACES_EXPORTER] == "solarwinds_exporter" + + def test_load_instrumentor_no_commenting(self, mocker): + mock_instrument = mocker.Mock() + mock_instrumentor = mocker.Mock() + mock_instrumentor.configure_mock( + return_value=mocker.Mock( + **{ + "instrument": mock_instrument + } + ) + ) + mock_load = mocker.Mock() + mock_load.configure_mock(return_value=mock_instrumentor) + mock_entry_point = mocker.Mock() + mock_entry_point.configure_mock( + **{ + "load": mock_load + } + ) + SolarWindsDistro().load_instrumentor(mock_entry_point, **{"foo": "bar"}) + mock_instrument.assert_called_once_with(**{"foo": "bar"}) + + def test_load_instrumentor_enable_commenting(self, mocker): + mock_instrument = mocker.Mock() + mock_instrumentor = mocker.Mock() + mock_instrumentor.configure_mock( + return_value=mocker.Mock( + **{ + "instrument": mock_instrument + } + ) + ) + mock_load = mocker.Mock() + mock_load.configure_mock(return_value=mock_instrumentor) + mock_entry_point = mocker.Mock() + mock_entry_point.configure_mock( + **{ + "load": mock_load + } + ) + mocker.patch( + "solarwinds_apm.distro.SolarWindsDistro.enable_commenter", + return_value=True + ) + mocker.patch( + "solarwinds_apm.distro.SolarWindsDistro.detect_commenter_options", + return_value="foo-options" + ) + SolarWindsDistro().load_instrumentor(mock_entry_point, **{"foo": "bar"}) + mock_instrument.assert_called_once_with( + commenter_options="foo-options", + enable_commenter=True, + foo="bar", + is_sql_commentor_enabled=True, + ) + + def test_enable_commenter_none(self, mocker): + mocker.patch.dict(os.environ, {}) + assert SolarWindsDistro().enable_commenter() == False + + def test_enable_commenter_non_bool_value(self, mocker): + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "foo"}) + assert SolarWindsDistro().enable_commenter() == False + + def test_enable_commenter_false(self, mocker): + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "false"}) + assert SolarWindsDistro().enable_commenter() == False + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "False"}) + assert SolarWindsDistro().enable_commenter() == False + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "faLsE"}) + assert SolarWindsDistro().enable_commenter() == False + + def test_enable_commenter_true(self, mocker): + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "true"}) + assert SolarWindsDistro().enable_commenter() == True + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "True"}) + assert SolarWindsDistro().enable_commenter() == True + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_ENABLED": "tRuE"}) + assert SolarWindsDistro().enable_commenter() == True + + def test_detect_commenter_options_not_set(self, mocker): + mocker.patch.dict(os.environ, {}) + result = SolarWindsDistro().detect_commenter_options() + assert result == {} + + def test_detect_commenter_options_invalid_kv_ignored(self, mocker): + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_OPTIONS": "invalid-kv,foo=bar"}) + result = SolarWindsDistro().detect_commenter_options() + assert result == {} + + def test_detect_commenter_options_valid_kvs(self, mocker): + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_OPTIONS": "foo=true,bar=FaLSe"}) + result = SolarWindsDistro().detect_commenter_options() + assert result == { + "foo": True, + "bar": False, + } + + def test_detect_commenter_options_strip_whitespace_ok(self, mocker): + mocker.patch.dict( + os.environ, + { + "OTEL_SQLCOMMENTER_OPTIONS": " foo = tRUe , bar = falsE " + } + ) + result = SolarWindsDistro().detect_commenter_options() + assert result.get("foo") == True + assert result.get("bar") == False + + def test_detect_commenter_options_strip_mix(self, mocker): + mocker.patch.dict(os.environ, {"OTEL_SQLCOMMENTER_OPTIONS": "invalid-kv, foo=TrUe ,bar = faLSE, baz=qux "}) + result = SolarWindsDistro().detect_commenter_options() + assert result.get("foo") == True + assert result.get("bar") == False + assert result.get("baz") is None