Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NH-69217 sw.transaction attribute on otlp traces, metrics #307

Merged
merged 7 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions solarwinds_apm/apm_constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
INTL_SWO_OTEL_SCOPE_NAME = "otel.scope.name"
INTL_SWO_OTEL_SCOPE_VERSION = "otel.scope.version"
INTL_SWO_TRACESTATE_KEY = "sw"
INTL_SWO_TRANSACTION_ATTR_KEY = "sw.transaction"
INTL_SWO_TRANSACTION_ATTR_MAX = 255
INTL_SWO_X_OPTIONS_KEY = "sw_xtraceoptions"
INTL_SWO_X_OPTIONS_RESPONSE_KEY = "xtrace_options_response"
INTL_SWO_DEFAULT_TRACES_EXPORTER = "solarwinds_exporter"
Expand Down
46 changes: 46 additions & 0 deletions solarwinds_apm/sampler.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
INTL_SWO_COMMA_W3C_SANITIZED,
INTL_SWO_EQUALS_W3C_SANITIZED,
INTL_SWO_TRACESTATE_KEY,
INTL_SWO_TRANSACTION_ATTR_KEY,
INTL_SWO_TRANSACTION_ATTR_MAX,
INTL_SWO_X_OPTIONS_KEY,
INTL_SWO_X_OPTIONS_RESPONSE_KEY,
)
Expand Down Expand Up @@ -69,6 +71,9 @@ def __init__(
oboe_api: "OboeAPI",
):
self.apm_config = apm_config
# SW_APM_TRANSACTION_NAME and AWS_LAMBDA_FUNCTION_NAME
self.env_transaction_name = apm_config.get("transaction_name")
self.lambda_function_name = apm_config.lambda_function_name
self.context = apm_config.extension.Context

if self.apm_config.get("tracing_mode") is not None:
Expand Down Expand Up @@ -415,6 +420,35 @@ def add_tracestate_capture_to_attributes_dict(
)
return attributes_dict

def calculate_otlp_transaction_name(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new sampler function duplicates what's in the otlp metrics processor. If this should be split it out into a shared place, I'm not sure where to put it. Maybe a top-level util like W3CTransformer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yes good point, it would be nice to have it shared, although W3C... is very much an HTTP header concern while transaction naming is our SWO-specific concept, so I wouldn't put it there.

There is the top-level apm_txnname_manager, and some other transaction name classes under trace/ that might fit the bill. But you may want to just defer the refactoring until you've had a chance to investigate the idea of subclassing the OTel exporter in order to support custom SDK naming for lambda (which can definitely be a future iteration).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I couldn't remember that exact piece. Made a ticket for subclassing: https://swicloud.atlassian.net/browse/NH-72398

self,
span_name: str,
) -> str:
"""Calculate transaction name for OTLP trace following this order
of decreasing precedence, truncated to 255 char:

1. SW_APM_TRANSACTION_NAME
2. AWS_LAMBDA_FUNCTION_NAME
3. automated naming from span name
4. "unknown" backup, to match core lib

Notes:
* Spans at time of sampling decision/on_start will not have access
to SDK-set names, nor all span attributes from instrumentors
* But they should have access to span `name`
* Spans at on_end are immutable
"""
if self.env_transaction_name:
return self.env_transaction_name[:INTL_SWO_TRANSACTION_ATTR_MAX]

if self.lambda_function_name:
return self.lambda_function_name[:INTL_SWO_TRANSACTION_ATTR_MAX]

if span_name:
return span_name[:INTL_SWO_TRANSACTION_ATTR_MAX]

return "unknown"

def calculate_attributes(
self,
span_name: str,
Expand Down Expand Up @@ -462,6 +496,18 @@ def calculate_attributes(
f"{decision['bucket_rate']}"
)

# If `experimental` and `otel_collector`,
# always (root or is_remote) set sw.transaction
# TODO Clean up use of experimental trace flag
# https://swicloud.atlassian.net/browse/NH-65067
if self.apm_config.get("experimental").get("otel_collector") is True:
logger.debug(
"Experimental otel_collector flag configured. Setting sw.transaction on span."
)
new_attributes[INTL_SWO_TRANSACTION_ATTR_KEY] = (
self.calculate_otlp_transaction_name(span_name)
)

# sw.tracestate_parent_id is set if:
# 1. the future span is the entry span of a service
# 2. there exists a (remote) parent span
Expand Down
44 changes: 21 additions & 23 deletions solarwinds_apm/trace/otlp_metrics_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
import logging
from typing import TYPE_CHECKING

from solarwinds_apm.apm_constants import (
INTL_SWO_TRANSACTION_ATTR_KEY,
INTL_SWO_TRANSACTION_ATTR_MAX,
)
from solarwinds_apm.apm_meter_manager import SolarWindsMeterManager
from solarwinds_apm.apm_noop import SolarWindsMeterManager as NoopMeterManager
from solarwinds_apm.trace.base_metrics_processor import _SwBaseMetricsProcessor
Expand All @@ -17,7 +21,6 @@
from solarwinds_apm.apm_config import SolarWindsApmConfig
from solarwinds_apm.apm_txname_manager import SolarWindsTxnNameManager
from solarwinds_apm.extension.oboe import OboeAPI
from solarwinds_apm.trace.tnames import TransactionNames


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -56,26 +59,28 @@ def __init__(

def calculate_otlp_transaction_name(
self,
tnames: "TransactionNames",
span_name: str,
) -> str:
"""Calculate transaction name for OTLP metrics following this order
of decreasing precedence:
of decreasing precedence, truncated to 255 char:

1. custom SDK name
2. SW_APM_TRANSACTION_NAME
3. AWS_LAMBDA_FUNCTION_NAME
4. automated naming from span name, attributes
"""
if tnames.custom_name:
return tnames.custom_name
1. SW_APM_TRANSACTION_NAME
2. AWS_LAMBDA_FUNCTION_NAME
3. automated naming from span name
4. "unknown" backup, to match core lib

See also _SwSampler.calculate_otlp_transaction_name
"""
if self.env_transaction_name:
return self.env_transaction_name
return self.env_transaction_name[:INTL_SWO_TRANSACTION_ATTR_MAX]

if self.lambda_function_name:
return self.lambda_function_name
return self.lambda_function_name[:INTL_SWO_TRANSACTION_ATTR_MAX]

if span_name:
return span_name[:INTL_SWO_TRANSACTION_ATTR_MAX]

return tnames.trans_name
return "unknown"

def on_end(self, span: "ReadableSpan") -> None:
"""Calculates and reports OTLP trace metrics"""
Expand All @@ -88,14 +93,7 @@ def on_end(self, span: "ReadableSpan") -> None:
):
return

tnames = self.get_tnames(span)
if not tnames:
logger.error(
"Could not get transaction name. Not recording otlp metrics."
)
return

trans_name = self.calculate_otlp_transaction_name(tnames)
trans_name = self.calculate_otlp_transaction_name(span.name)

meter_attrs = {}
has_error = self.has_error(span)
Expand All @@ -119,11 +117,11 @@ def on_end(self, span: "ReadableSpan") -> None:
{
self._HTTP_STATUS_CODE: status_code,
self._HTTP_METHOD: request_method,
"sw.transaction": trans_name,
INTL_SWO_TRANSACTION_ATTR_KEY: trans_name,
}
)
else:
meter_attrs.update({"sw.transaction": trans_name})
meter_attrs.update({INTL_SWO_TRANSACTION_ATTR_KEY: trans_name})
self.apm_meters.response_time.record(
amount=span_time,
attributes=meter_attrs,
Expand Down
77 changes: 47 additions & 30 deletions tests/unit/test_processors/test_otlp_metrics_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,70 +88,87 @@ def test__init_not_experimental(self, mocker, mock_meter_manager):
assert processor.lambda_function_name == "foo-lambda-name"
assert isinstance(processor.apm_meters, NoopMeterManager)

def test_calculate_otlp_transaction_name_custom(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(mocker)
tnames = TransactionNames(
"unused",
"unused",
"foo-custom-name",
def test_calculate_otlp_transaction_name_env_trans(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
"foo-env-trans-name",
)
assert "foo-custom-name" == SolarWindsOTLPMetricsSpanProcessor(
assert "foo-env-trans-name" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name(tnames)
).calculate_otlp_transaction_name("foo-span")

def test_calculate_otlp_transaction_name_env_trans(self, mocker, mock_meter_manager):
def test_calculate_otlp_transaction_name_env_trans_truncated(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
"foo-env-trans-name",
)
tnames = TransactionNames(
"unused",
"unused",
None,
"foo-txn-ffoooofofooooooofooofooooofofofofoooooofoooooooooffoffooooooffffofooooofffooooooofoooooffoofofoooooofffofooofoffoooofooofoooooooooooooofooffoooofofooofoooofoofooffooooofoofooooofoooooffoofffoffoooooofoooofoooffooffooofofooooooffffooofoooooofoooooofooofoooofoo",
)
assert "foo-env-trans-name" == SolarWindsOTLPMetricsSpanProcessor(
assert "foo-txn-ffoooofofooooooofooofooooofofofofoooooofoooooooooffoffooooooffffofooooofffooooooofoooooffoofofoooooofffofooofoffoooofooofoooooooooooooofooffoooofofooofoooofoofooffooooofoofooooofoooooffoofffoffoooooofoooofoooffooffooofofooooooffffooofoooooofoooooo" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name(tnames)
).calculate_otlp_transaction_name("foo-span")

def test_calculate_otlp_transaction_name_env_lambda(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
outer_txn_retval=None,
lambda_function_name="foo-lambda-name",
lambda_function_name="foo-lambda-ffoooofofooooooofooofooooofofofofoooooofoooooooooffoffooooooffffofooooofffooooooofoooooffoofofoooooofffofooofoffoooofooofoooooooooooooofooffoooofofooofoooofoofooffooooofoofooooofoooooffoofffoffoooooofoooofoooffooffooofofooooooffffooofoooooofoooooofooofoooofoo",
)
tnames = TransactionNames(
"unused",
"unused",
None,
assert "foo-lambda-ffoooofofooooooofooofooooofofofofoooooofoooooooooffoffooooooffffofooooofffooooooofoooooffoofofoooooofffofooofoffoooofooofoooooooooooooofooffoooofofooofoooofoofooffooooofoofooooofoooooffoofffoffoooooofoooofoooffooffooofofooooooffffooofoooooofooo" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name("foo-span")

def test_calculate_otlp_transaction_name_env_lambda_truncated(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
outer_txn_retval=None,
lambda_function_name="foo-lambda-name",
)
assert "foo-lambda-name" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name(tnames)
).calculate_otlp_transaction_name("foo-span")

def test_calculate_otlp_transaction_name_default(self, mocker, mock_meter_manager):
def test_calculate_otlp_transaction_name_span_name(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
outer_txn_retval=None,
lambda_function_name=None,
)
assert "foo-span" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name("foo-span")

tnames = TransactionNames(
"foo-trans-name",
"unused",
None,
def test_calculate_otlp_transaction_name_span_name_truncated(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
outer_txn_retval=None,
lambda_function_name=None,
)
assert "foo-span-ffoooofofooooooofooofooooofofofofoooooofoooooooooffoffooooooffffofooooofffooooooofoooooffoofofoooooofffofooofoffoooofooofoooooooooooooofooffoooofofooofoooofoofooffooooofoofooooofoooooffoofffoffoooooofoooofoooffooffooofofooooooffffooofoooooofooooo" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name("foo-span-ffoooofofooooooofooofooooofofofofoooooofoooooooooffoffooooooffffofooooofffooooooofoooooffoofofoooooofffofooofoffoooofooofoooooooooooooofooffoooofofooofoooofoofooffooooofoofooooofoooooffoofffoffoooooofoooofoooffooffooofofooooooffffooofoooooofoooooofooofoooofoo")

assert "foo-trans-name" == SolarWindsOTLPMetricsSpanProcessor(
def test_calculate_otlp_transaction_name_empty(self, mocker, mock_meter_manager):
mock_apm_config = self.get_mock_apm_config(
mocker,
outer_txn_retval=None,
lambda_function_name=None,
)
assert "unknown" == SolarWindsOTLPMetricsSpanProcessor(
mocker.Mock(),
mock_apm_config,
mocker.Mock()
).calculate_otlp_transaction_name(tnames)
).calculate_otlp_transaction_name("")

def patch_for_on_end(
self,
Expand Down
30 changes: 27 additions & 3 deletions tests/unit/test_sampler/fixtures/sampler.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,38 @@ def config_get(param) -> Any:
return -1
elif param == "transaction_filters":
return []
elif param == "transaction_name":
return "foo-txn"
else:
return None

@pytest.fixture(name="fixture_swsampler")
def fixture_swsampler(mocker):
# mock `experimental` `otel_collector` when checked
mock_get_inner = mocker.Mock(return_value=True)
mock_inner = mocker.Mock()
mock_inner.configure_mock(
**{
"get": mock_get_inner,
}
)
def outer_side_effect(param) -> Any:
if param == "tracing_mode":
return -1
elif param == "transaction_filters":
return []
elif param == "transaction_name":
return "foo-txn"
elif param == "experimental":
return mock_inner
else:
return None

mock_apm_config = mocker.Mock()
mock_get = mocker.Mock(
side_effect=config_get
mock_get_outer = mocker.Mock(
side_effect=outer_side_effect
)

mock_get_decisions = mocker.patch(
"solarwinds_apm.extension.oboe.Context.getDecisions"
)
Expand All @@ -43,9 +66,10 @@ def fixture_swsampler(mocker):
mock_apm_config.configure_mock(
**{
"agent_enabled": True,
"get": mock_get,
"get": mock_get_outer,
"extension": mock_extension,
"is_lambda": False,
"lambda_function_name": "foo-lambda",
}
)
return _SwSampler(mock_apm_config, mocker.Mock())
Expand Down
19 changes: 19 additions & 0 deletions tests/unit/test_sampler/test_sampler.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.

from typing import Any

import pytest

from opentelemetry.sdk.trace.sampling import (
Expand Down Expand Up @@ -199,11 +201,28 @@ def fixture_parent_span_context_valid_remote_no_tracestate():

class Test_SwSampler():
def test_init(self, mocker):
def config_get(param) -> Any:
if param == "transaction_name":
return "foo-txn"
else:
return "foo"

mock_get = mocker.Mock(
side_effect=config_get
)
mock_apm_config = mocker.Mock()
mock_apm_config.configure_mock(
**{
"get": mock_get,
"lambda_function_name": "foo-lambda",
}
)
mock_oboe_api = mocker.Mock()
sampler = _SwSampler(mock_apm_config, mock_oboe_api)
assert sampler.apm_config == mock_apm_config
assert sampler.oboe_settings_api == mock_oboe_api
assert sampler.env_transaction_name == "foo-txn"
assert sampler.lambda_function_name == "foo-lambda"

def test_calculate_liboboe_decision_is_lambda(
self,
Expand Down
Loading