From ba5791262425c26c9730cf24cd76ab5d2d88e4a3 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 12:10:02 -0700 Subject: [PATCH 01/12] Log warning instead error --- solarwinds_apm/apm_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 85b6135f..1ea97caf 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -532,7 +532,7 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: """Update configured transaction_filters using config dict""" txn_settings = cnf_dict.get("transactionSettings") if not txn_settings or not isinstance(txn_settings, list): - logger.error( + logger.warning( "Transaction filters must be a non-empty list of filters. Ignoring." ) return @@ -540,7 +540,7 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: if set(filter) != set(["regex", "tracing"]) or filter[ "tracing" ] not in ["enabled", "disabled"]: - logger.error( + logger.warning( "Invalid transaction filter rule. Ignoring: %s", filter ) continue From a2f414c06eaa3a950fd22797530b3b84dd1b7733 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 12:11:07 -0700 Subject: [PATCH 02/12] Txn filters checked if valid regex, stored with oboe tracing_mode --- solarwinds_apm/apm_config.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 1ea97caf..f6d2c0ca 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -7,6 +7,7 @@ import json import logging import os +import re import sys from collections import defaultdict from functools import reduce @@ -549,7 +550,31 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: cfilter["regex"] for cfilter in self.__config["transaction_filters"] ]: - self.__config["transaction_filters"].append(filter) + txn_filter = {} + txn_filter[ + "tracing_mode" + ] = OboeTracingMode.get_oboe_trace_mode(filter["tracing"]) + + if not isinstance(filter["regex"], str): + logger.warning( + "Transaction filter regex must be string or regex. Ignoring: %s", + filter, + ) + continue + try: + re.compile(filter["regex"]) + except re.error: + logger.warning( + "Transaction filter regex invalid. Ignoring: %s", + filter, + ) + txn_filter["regex"] = filter["regex"] + self.__config["transaction_filters"].append(txn_filter) + + # TODO (NH-34752) Confirm handling web request filtering after instrumentation + # libraries updated so http attributes available at should_sample + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/936 + logger.debug( "Set up transaction filters: %s", self.__config["transaction_filters"], From 8539bfb631b457ddd1187a858ac073a9d046546b Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 14:14:14 -0700 Subject: [PATCH 03/12] Add stub for calculate_tracing_mode --- solarwinds_apm/sampler.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/solarwinds_apm/sampler.py b/solarwinds_apm/sampler.py index e4eba36e..e1444ab0 100644 --- a/solarwinds_apm/sampler.py +++ b/solarwinds_apm/sampler.py @@ -75,10 +75,23 @@ def __init__(self, apm_config: "SolarWindsApmConfig"): def get_description(self) -> str: return "SolarWinds custom opentelemetry sampler" + def calculate_tracing_mode( + self, + name: str, + kind: SpanKind = None, + attributes: Attributes = None, + ) -> int: + """Calculates tracing mode per-request or global mode, if set""" + # TODO implement this + return self._UNSET + # pylint: disable=too-many-locals def calculate_liboboe_decision( self, parent_span_context: SpanContext, + name: str, + kind: SpanKind = None, + attributes: Attributes = None, xtraceoptions: Optional[XTraceOptions] = None, ) -> dict: """Calculates oboe trace decision based on parent span context and APM config.""" @@ -91,8 +104,11 @@ def calculate_liboboe_decision( INTL_SWO_TRACESTATE_KEY ) - # 'tracing_mode' is not supported in NH Python, so give as unset - tracing_mode = self._UNSET + tracing_mode = self.calculate_tracing_mode( + name, + kind, + attributes, + ) trigger_trace_mode = OboeTracingMode.get_oboe_trigger_trace_mode( self.apm_config.get("trigger_trace") @@ -486,7 +502,7 @@ def should_sample( xtraceoptions = XTraceOptions(parent_context) liboboe_decision = self.calculate_liboboe_decision( - parent_span_context, xtraceoptions + parent_span_context, name, kind, attributes, xtraceoptions ) # Always calculate trace_state for propagation From 54ca418e761ebda704dada18b3f09a8cc04dfa33 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 15:13:46 -0700 Subject: [PATCH 04/12] Add regex len check --- solarwinds_apm/apm_config.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index f6d2c0ca..a5d091e7 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -561,6 +561,14 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: filter, ) continue + + if not len(filter["regex"]) > 0: + logger.warning( + "Transaction filter regex must not be empty. Ignoring: %s", + filter, + ) + continue + try: re.compile(filter["regex"]) except re.error: From 17ae7a3e78d6028af95e4daa1a381966d430fa3c Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 15:14:38 -0700 Subject: [PATCH 05/12] calculate_tracing_mode checks span kind,name --- solarwinds_apm/sampler.py | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/solarwinds_apm/sampler.py b/solarwinds_apm/sampler.py index e1444ab0..5fbf6c53 100644 --- a/solarwinds_apm/sampler.py +++ b/solarwinds_apm/sampler.py @@ -11,6 +11,7 @@ import enum import logging +import re from types import MappingProxyType from typing import TYPE_CHECKING, Optional, Sequence @@ -67,11 +68,17 @@ class _SwSampler(Sampler): def __init__(self, apm_config: "SolarWindsApmConfig"): self.apm_config = apm_config self.context = None + if self.apm_config.agent_enabled: self.context = Context else: self.context = NoopContext + if self.apm_config.get("tracing_mode") is not None: + self.tracing_mode = self.apm_config.get("tracing_mode") + else: + self.tracing_mode = self._UNSET + def get_description(self) -> str: return "SolarWinds custom opentelemetry sampler" @@ -81,9 +88,28 @@ def calculate_tracing_mode( kind: SpanKind = None, attributes: Attributes = None, ) -> int: - """Calculates tracing mode per-request or global mode, if set""" - # TODO implement this - return self._UNSET + """Calculates tracing mode per-request or global tracing mode, if set. + Can still be overridden by remote settings.""" + # If future span matches txn filter, use filter's tracing mode + if self.apm_config.get("transaction_filters"): + for txn_filter in self.apm_config.get("transaction_filters"): + # TODO (NH-34752) Check `attributes` for http.* then filter web requests + # after OTel instrumentation library updates released + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/936 + + # Only matches span kind and name at this time + identifier = f"{kind.name}:{name}" + if re.search(txn_filter.get("regex"), identifier): + logger.debug("Got a match for identifier %s", identifier) + logger.debug( + "Setting tracing_mode as %s", + txn_filter.get("tracing_mode"), + ) + return txn_filter.get("tracing_mode") + + # Else use global 'tracing_mode' + logger.debug("Using global tracing_mode as %s", self.tracing_mode) + return self.tracing_mode # pylint: disable=too-many-locals def calculate_liboboe_decision( From 1b987736090b673284dffbf85e579c1752de5a6a Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 15:15:59 -0700 Subject: [PATCH 06/12] Rm comment --- solarwinds_apm/apm_config.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index a5d091e7..e8e4d3a1 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -579,10 +579,6 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: txn_filter["regex"] = filter["regex"] self.__config["transaction_filters"].append(txn_filter) - # TODO (NH-34752) Confirm handling web request filtering after instrumentation - # libraries updated so http attributes available at should_sample - # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/936 - logger.debug( "Set up transaction filters: %s", self.__config["transaction_filters"], From 4489a344bcd52c642684124fa2b0ca89fec08ddd Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 21 Apr 2023 16:12:18 -0700 Subject: [PATCH 07/12] Fix unit tests --- tests/unit/test_sampler/fixtures/sampler.py | 9 +++++++-- tests/unit/test_sampler/test_sampler.py | 9 +++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_sampler/fixtures/sampler.py b/tests/unit/test_sampler/fixtures/sampler.py index eb29d4ed..5edbd3db 100644 --- a/tests/unit/test_sampler/fixtures/sampler.py +++ b/tests/unit/test_sampler/fixtures/sampler.py @@ -2,17 +2,22 @@ from solarwinds_apm.sampler import _SwSampler +def side_effect_fn(param): + if param == "tracing_mode": + return -1 + elif param == "transaction_filters": + return [] + @pytest.fixture(name="sw_sampler") def fixture_swsampler(mocker): mock_apm_config = mocker.Mock() mock_get = mocker.Mock( - return_value=1 # enabled + side_effect=side_effect_fn ) mock_apm_config.configure_mock( **{ "agent_enabled": True, "get": mock_get, - "tracing_mode": None, # mapped to -1 } ) return _SwSampler(mock_apm_config) diff --git a/tests/unit/test_sampler/test_sampler.py b/tests/unit/test_sampler/test_sampler.py index 7d2670c8..bf53100d 100644 --- a/tests/unit/test_sampler/test_sampler.py +++ b/tests/unit/test_sampler/test_sampler.py @@ -196,6 +196,9 @@ def test_calculate_liboboe_decision_root_span( ): sw_sampler.calculate_liboboe_decision( parent_span_context_invalid, + 'foo', + None, + {'foo': 'bar'}, mock_xtraceoptions_signed_tt, ) solarwinds_apm.extension.oboe.Context.getDecisions.assert_called_once_with( @@ -219,6 +222,9 @@ def test_calculate_liboboe_decision_parent_valid_remote( ): sw_sampler.calculate_liboboe_decision( parent_span_context_valid_remote, + 'foo', + None, + {'foo': 'bar'}, ) solarwinds_apm.extension.oboe.Context.getDecisions.assert_called_once_with( "foo-bar", @@ -574,6 +580,9 @@ def test_should_sample( _SwSampler.calculate_liboboe_decision.assert_called_once_with( "my_span_context", + 'foo', + None, + {'foo': 'bar'}, mock_xtraceoptions ) _SwSampler.calculate_trace_state.assert_called_once_with( From 5fb502ea54113c97aa29759131d56ad6e6fd42f3 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Apr 2023 14:32:07 -0700 Subject: [PATCH 08/12] Fix: config tracing_mode should be int not str --- solarwinds_apm/apm_config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index e8e4d3a1..a6dffe89 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -658,9 +658,10 @@ def _convert_to_bool(val): val = "enabled" if val == "always" else "disabled" if val not in ["enabled", "disabled"]: raise ValueError - self.__config[key] = val + oboe_trace_mode = OboeTracingMode.get_oboe_trace_mode(val) + self.__config[key] = oboe_trace_mode self.context.setTracingMode( - OboeTracingMode.get_oboe_trace_mode(val) + oboe_trace_mode ) elif keys == ["trigger_trace"]: if not isinstance(val, str): From 8fb429dbd20e0cb78d931e76c1491e9b4091422b Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Apr 2023 14:34:58 -0700 Subject: [PATCH 09/12] tracing_mode default a bit more clear --- solarwinds_apm/apm_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index a6dffe89..47ffe13d 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -89,8 +89,8 @@ def __init__( self.__config = {} # Update the config with default values self.__config = { - # 'tracing_mode' is unset by default and not supported in NH Python - "tracing_mode": None, + # 'tracing_mode' is unset by default + "tracing_mode": OboeTracingMode.get_oboe_trace_mode("unset"), # 'trigger_trace' is enabled by default "trigger_trace": "enabled", "collector": "", # the collector address in host:port format. From 0cf4b508c982691fe674a00c3df0f9b1327de82b Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Apr 2023 14:39:47 -0700 Subject: [PATCH 10/12] Fix invalid filter regex ignore --- solarwinds_apm/apm_config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 47ffe13d..40e20f0f 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -576,6 +576,7 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: "Transaction filter regex invalid. Ignoring: %s", filter, ) + continue txn_filter["regex"] = filter["regex"] self.__config["transaction_filters"].append(txn_filter) From d28c040a158f4349bb918285dd92e04fda31cc80 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Apr 2023 14:53:52 -0700 Subject: [PATCH 11/12] Store txn filter regex compiled --- solarwinds_apm/apm_config.py | 5 +++-- solarwinds_apm/sampler.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 40e20f0f..7385aa91 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -569,15 +569,16 @@ def update_transaction_filters(self, cnf_dict: dict) -> None: ) continue + txn_filter_re = None try: - re.compile(filter["regex"]) + txn_filter_re = re.compile(filter["regex"]) except re.error: logger.warning( "Transaction filter regex invalid. Ignoring: %s", filter, ) continue - txn_filter["regex"] = filter["regex"] + txn_filter["regex"] = txn_filter_re self.__config["transaction_filters"].append(txn_filter) logger.debug( diff --git a/solarwinds_apm/sampler.py b/solarwinds_apm/sampler.py index 5fbf6c53..64b2fbdf 100644 --- a/solarwinds_apm/sampler.py +++ b/solarwinds_apm/sampler.py @@ -99,7 +99,7 @@ def calculate_tracing_mode( # Only matches span kind and name at this time identifier = f"{kind.name}:{name}" - if re.search(txn_filter.get("regex"), identifier): + if txn_filter.get("regex").search(identifier): logger.debug("Got a match for identifier %s", identifier) logger.debug( "Setting tracing_mode as %s", From 336b89a799bc634cd53a25e53e43a347d8a8f658 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 24 Apr 2023 14:58:15 -0700 Subject: [PATCH 12/12] Lint and unit test --- solarwinds_apm/apm_config.py | 4 +--- solarwinds_apm/sampler.py | 1 - tests/unit/test_apm_config.py | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/solarwinds_apm/apm_config.py b/solarwinds_apm/apm_config.py index 7385aa91..ee3289fe 100644 --- a/solarwinds_apm/apm_config.py +++ b/solarwinds_apm/apm_config.py @@ -662,9 +662,7 @@ def _convert_to_bool(val): raise ValueError oboe_trace_mode = OboeTracingMode.get_oboe_trace_mode(val) self.__config[key] = oboe_trace_mode - self.context.setTracingMode( - oboe_trace_mode - ) + self.context.setTracingMode(oboe_trace_mode) elif keys == ["trigger_trace"]: if not isinstance(val, str): raise ValueError diff --git a/solarwinds_apm/sampler.py b/solarwinds_apm/sampler.py index 64b2fbdf..ced9e098 100644 --- a/solarwinds_apm/sampler.py +++ b/solarwinds_apm/sampler.py @@ -11,7 +11,6 @@ import enum import logging -import re from types import MappingProxyType from typing import TYPE_CHECKING, Optional, Sequence diff --git a/tests/unit/test_apm_config.py b/tests/unit/test_apm_config.py index ac6ee680..0585b6ec 100644 --- a/tests/unit/test_apm_config.py +++ b/tests/unit/test_apm_config.py @@ -650,7 +650,7 @@ def test_set_config_value_default_proxy(self, caplog, mock_env_vars): def test_set_config_value_default_tracing_mode(self, caplog, mock_env_vars): test_config = apm_config.SolarWindsApmConfig() test_config._set_config_value("tracing_mode", "not-valid-mode") - assert test_config.get("tracing_mode") == None + assert test_config.get("tracing_mode") == -1 assert "Ignore config option" in caplog.text # pylint:disable=unused-argument