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-65068 Add is_lambda checks and todos to APM Python #211

Merged
merged 14 commits into from
Nov 7, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased](https://github.com/solarwindscloud/solarwinds-apm-python/compare/rel-0.18.0...HEAD)

### Changed
- Change c-lib usage and `agent_enabled` calculation if `is_lambda` ([#211](https://github.com/solarwindscloud/solarwinds-apm-python/pull/211))
- Updated Makefile for APM Python lambda builds ([#212](https://github.com/solarwindscloud/solarwinds-apm-python/pull/212))

## [0.18.0](https://github.com/solarwindscloud/solarwinds-apm-python/releases/tag/rel-0.18.0) - 2023-10-31
Expand Down
95 changes: 70 additions & 25 deletions solarwinds_apm/apm_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from opentelemetry.sdk.resources import Resource
from pkg_resources import iter_entry_points

import solarwinds_apm.apm_noop as noop_extension
from solarwinds_apm import apm_logging
from solarwinds_apm.apm_constants import (
INTL_SWO_AO_COLLECTOR,
Expand Down Expand Up @@ -115,6 +116,7 @@ def __init__(
"experimental": {},
"transaction_name": None,
}
self.is_lambda = self._is_lambda()
self.agent_enabled = True
self.update_with_cnf_file()
self.update_with_env_var()
Expand All @@ -132,25 +134,14 @@ def __init__(
self.service_name,
)

if self.agent_enabled:
try:
# pylint: disable=import-outside-toplevel
import solarwinds_apm.extension.oboe as c_extension
except ImportError as err:
# At this point, if agent_enabled but cannot import
# extension then something unexpected happened
logger.error(
"Could not import extension. Please contact %s. Tracing disabled: %s",
INTL_SWO_SUPPORT_EMAIL,
err,
)
# pylint: disable=import-outside-toplevel
import solarwinds_apm.apm_noop as c_extension
else:
# pylint: disable-next=import-outside-toplevel
import solarwinds_apm.apm_noop as c_extension
self.extension = c_extension
self.context = self.extension.Context
# Calculate c-lib extension usage
# TODO Used returned OboeAPI
# https://swicloud.atlassian.net/browse/NH-64716
self.extension, self.context = self._get_extension_components(
self.agent_enabled,
self.is_lambda,
)

self.context.setTracingMode(self.__config["tracing_mode"])
self.context.setTriggerMode(self.__config["trigger_trace"])

Expand All @@ -159,13 +150,46 @@ def __init__(

logger.debug("Set ApmConfig as: %s", self)

def _get_extension_components(
self,
agent_enabled: bool,
is_lambda: bool,
) -> None:
"""Returns c-lib extension or noop components based on agent_enabled, is_lambda.

TODO: Return OboeAPI as noop or c-lib version
https://swicloud.atlassian.net/browse/NH-64716

agent_enabled T, is_lambda F -> c-lib extension, c-lib Context
agent_enabled T, is_lambda T -> no-op extension, no-op Context
agent_enabled F -> all no-op
"""
if not agent_enabled:
return noop_extension, noop_extension.Context
try:
# pylint: disable=import-outside-toplevel
import solarwinds_apm.extension.oboe as c_extension
except ImportError as err:
# At this point, if agent_enabled but cannot import
# extension then something unexpected happened
logger.error(
"Could not import extension. Please contact %s. Tracing disabled: %s",
INTL_SWO_SUPPORT_EMAIL,
err,
)
return noop_extension, noop_extension.Context

if is_lambda:
return noop_extension, noop_extension.Context
return c_extension, c_extension.Context

def _is_lambda(self) -> bool:
"""Checks if agent is running in an AWS Lambda environment."""
if os.environ.get("AWS_LAMBDA_FUNCTION_NAME") and os.environ.get(
"LAMBDA_TASK_ROOT"
):
logger.warning(
"AWS Lambda is not yet supported by Python SolarWinds APM."
"AWS Lambda is experimental in Python SolarWinds APM."
)
return True
return False
Expand All @@ -185,6 +209,19 @@ def _calculate_agent_enabled_platform(self) -> bool:
return False
return True

def _calculate_agent_enabled_config_lambda(self) -> bool:
"""Checks if agent is enabled/disabled based on config in lambda environment:
- SW_APM_AGENT_ENABLED (optional) (env var or cnf file)
"""
if not self.agent_enabled:
logger.info(
"SolarWinds APM is disabled and will not report any traces because the environment variable "
"SW_APM_AGENT_ENABLED or the config file agentEnabled field is set to 'false'! If this is not intended either unset the variable or set it to "
"a value other than false. Note that SW_APM_AGENT_ENABLED/agentEnabled is case-insensitive."
)
return False
return True

# TODO: Account for in-code config with kwargs after alpha
# pylint: disable=too-many-branches,too-many-return-statements
def _calculate_agent_enabled_config(self) -> bool:
Expand All @@ -194,8 +231,11 @@ def _calculate_agent_enabled_config(self) -> bool:
- OTEL_PROPAGATORS (optional) (env var only)
- OTEL_TRACES_EXPORTER (optional) (env var only)
"""
if self.is_lambda:
return self._calculate_agent_enabled_config_lambda()

# (1) SW_APM_SERVICE_KEY
if not self.__config.get("service_key") and not self._is_lambda():
if not self.__config.get("service_key"):
logger.error("Missing service key. Tracing disabled.")
return False
# Key must be at least one char + ":" + at least one other char
Expand Down Expand Up @@ -267,14 +307,16 @@ def _calculate_agent_enabled_config(self) -> bool:
return False

# (4) OTEL_TRACES_EXPORTER
# TODO Relax traces exporter requirements outside lambda
# https://swicloud.atlassian.net/browse/NH-65713
try:
# SolarWindsDistro._configure does setdefault so this shouldn't
# be None, but safer and more explicit this way
environ_exporters = os.environ.get(
OTEL_TRACES_EXPORTER,
INTL_SWO_DEFAULT_TRACES_EXPORTER,
).split(",")
# If not using the default propagators,
# If not using the default exporters,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixing an old typo

# can any arbitrary list BUT
# (1) must include solarwinds_exporter
# (2) other exporters must be loadable by OTel
Expand Down Expand Up @@ -530,7 +572,6 @@ def _snake_to_camel_case(key):
self.agent_enabled = cnf_agent_enabled

available_cnf = set(self.__config.keys())
# TODO after alpha: is_lambda
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per Slack chat starting here, we won't configure token_* in APM Python like we did in AO. So no longer needed.

for key in available_cnf:
# Use internal snake_case config keys to check JSON config file camelCase keys
val = cnf_dict.get(_snake_to_camel_case(key))
Expand Down Expand Up @@ -611,11 +652,12 @@ def update_with_env_var(self) -> None:
self.agent_enabled = env_agent_enabled

available_envvs = set(self.__config.keys())
# TODO after alpha: is_lambda
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for key in available_envvs:
if key == "transaction":
# we do not allow complex config options to be set via environment variables
continue
# TODO Add experimental trace flag, clean up
# https://swicloud.atlassian.net/browse/NH-65067
if key == "experimental":
# but we do allow flat SW_APM_EXPERIMENTAL_OTEL_COLLECTOR setting to match js
key = self._EXP_PREFIX + "otel_collector"
Expand Down Expand Up @@ -700,7 +742,6 @@ def _set_config_value(self, keys_str: str, val: Any) -> Any:
)
self.__config[key] = oboe_trigger_trace
elif keys == ["reporter"]:
# TODO: support 'lambda'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if not isinstance(val, str) or val.lower() not in (
"udp",
"ssl",
Expand All @@ -716,6 +757,8 @@ def _set_config_value(self, keys_str: str, val: Any) -> Any:
self.__config[key] = val
# update logging level of agent logger
apm_logging.set_sw_log_level(val)
# TODO Add experimental trace flag, clean up
# https://swicloud.atlassian.net/browse/NH-65067
elif keys == ["experimental"]:
for exp_k, exp_v in val.items():
if exp_k in self._EXP_KEYS:
Expand All @@ -727,6 +770,8 @@ def _set_config_value(self, keys_str: str, val: Any) -> Any:
)
else:
self.__config["experimental"][exp_k] = exp_v
# TODO Add experimental trace flag, clean up
# https://swicloud.atlassian.net/browse/NH-65067
elif keys == ["experimental_otel_collector"]:
val = self.convert_to_bool(val)
if val is None:
Expand Down
34 changes: 26 additions & 8 deletions solarwinds_apm/configurator.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import os
import sys
import time
from typing import TYPE_CHECKING

from opentelemetry import metrics, trace
from opentelemetry.environment_variables import (
Expand Down Expand Up @@ -46,7 +47,6 @@
)
from solarwinds_apm.apm_fwkv_manager import SolarWindsFrameworkKvManager
from solarwinds_apm.apm_meter_manager import SolarWindsMeterManager
from solarwinds_apm.apm_noop import Reporter
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 was actually the wrong import used for type checking before. Removed this and added solarwinds_apm.extension.oboe.Reporter below.

from solarwinds_apm.apm_noop import SolarWindsMeterManager as NoopMeterManager
from solarwinds_apm.apm_oboe_codes import OboeReporterCode
from solarwinds_apm.apm_txname_manager import SolarWindsTxnNameManager
Expand All @@ -61,6 +61,9 @@
)
from solarwinds_apm.version import __version__

if TYPE_CHECKING:
from solarwinds_apm.extension.oboe import Reporter

solarwinds_apm_logger = apm_logging.logger
logger = logging.getLogger(__name__)

Expand All @@ -80,6 +83,8 @@ def _configure(self, **kwargs: int) -> None:
apm_fwkv_manager = SolarWindsFrameworkKvManager()
apm_config = SolarWindsApmConfig()

# TODO Add experimental trace flag, clean up
# https://swicloud.atlassian.net/browse/NH-65067
if not apm_config.get("experimental").get("otel_collector") is True:
logger.debug(
"Experimental otel_collector flag not configured. Creating meter manager as no-op."
Expand Down Expand Up @@ -110,7 +115,7 @@ def _configure_otel_components(
"""Configure OTel sampler, exporter, propagator, response propagator"""
self._configure_sampler(apm_config)
if apm_config.agent_enabled:
self._configure_metrics_span_processor(
self._configure_inbound_metrics_span_processor(
apm_txname_manager,
apm_config,
)
Expand Down Expand Up @@ -165,12 +170,16 @@ def _configure_sampler(
),
)

def _configure_metrics_span_processor(
def _configure_inbound_metrics_span_processor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming for clarity

self,
apm_txname_manager: SolarWindsTxnNameManager,
apm_config: SolarWindsApmConfig,
) -> None:
"""Configure SolarWindsInboundMetricsSpanProcessor"""
"""Configure SolarWindsInboundMetricsSpanProcessor.
Note: if config's extension is no-op, the processor will run
but will not export inbound metrics."""
# TODO Refactor span processors
# https://swicloud.atlassian.net/browse/NH-65061
trace.get_tracer_provider().add_span_processor(
SolarWindsInboundMetricsSpanProcessor(
apm_txname_manager,
Expand All @@ -184,7 +193,11 @@ def _configure_otlp_metrics_span_processor(
apm_config: SolarWindsApmConfig,
apm_meters: SolarWindsMeterManager,
) -> None:
"""Configure SolarWindsOTLPMetricsSpanProcessor"""
"""Configure SolarWindsOTLPMetricsSpanProcessor.
If no meters/instruments are initialized, the processor will
run but will not collect/flush OTLP metrics."""
# TODO Add experimental trace flag, clean up
# https://swicloud.atlassian.net/browse/NH-65067
if not apm_config.get("experimental").get("otel_collector") is True:
logger.debug(
"Experimental otel_collector flag not configured. Not configuring OTLP metrics span processor."
Expand All @@ -207,8 +220,10 @@ def _configure_exporter(
apm_config: SolarWindsApmConfig,
) -> None:
"""Configure SolarWinds OTel span exporters, defaults or environment
configured, or none if agent disabled. Initialization of SolarWinds
exporter requires a liboboe reporter and agent_enabled flag."""
configured, or none if agent disabled.

Initialization of SolarWinds exporter requires a liboboe reporter
Note: if reporter is no-op, the SW exporter will not export spans."""
if not apm_config.agent_enabled:
logger.error("Tracing disabled. Cannot set span_processor.")
return
Expand Down Expand Up @@ -370,7 +385,10 @@ def _initialize_solarwinds_reporter(
self,
apm_config: SolarWindsApmConfig,
) -> "Reporter":
"""Initialize SolarWinds reporter used by sampler and exporter, using SolarWindsApmConfig. This establishes collector and sampling settings in a background thread."""
"""Initialize SolarWinds reporter used by sampler and exporter, using SolarWindsApmConfig.
This establishes collector and sampling settings in a background thread.

Note: if config's extension is no-op, this has no effect."""
reporter_kwargs = {
"hostname_alias": apm_config.get("hostname_alias"),
"log_level": apm_config.get("debug_level"),
Expand Down
3 changes: 3 additions & 0 deletions solarwinds_apm/inbound_metrics_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@


class SolarWindsInboundMetricsSpanProcessor(SpanProcessor):
# TODO Refactor span processors
# https://swicloud.atlassian.net/browse/NH-65061

_TRANSACTION_NAME = "transaction_name"
_HTTP_METHOD = SpanAttributes.HTTP_METHOD # "http.method"
_HTTP_ROUTE = SpanAttributes.HTTP_ROUTE # "http.route"
Expand Down
Loading