NH-46327 APM Python accepts OTEL_SQLCOMMENTER_OPTIONS

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:
@@ -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
@@ -697,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

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 (
@@ -19,11 +20,14 @@
 )
 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,
 )
 
+logger = logging.getLogger(__name__)
+
 
 class SolarWindsDistro(BaseDistro):
     """OpenTelemetry Distro for SolarWinds reporting environment"""
@@ -49,19 +53,47 @@ 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
             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)
 
-    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
         enable_commenter = environ.get("OTEL_SQLCOMMENTER_ENABLED", "")
         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 = 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

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")

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