fix: remove stale opentelemetry_sdk packages at lib module import time (

#158)

* fix: remove stale opentelemetry_sdk packages at lib module import

fixes #157 by moving the stale package culling of charm_tracing.py ahead of all package imports and widening its scope to any opentelemetry package directories.  Also fixes as a drive-by some linting rules/code format.

lib/charms/tempo_k8s/v1/charm_tracing.py

@@ -172,14 +172,65 @@ def my_tracing_endpoint(self) -> Optional[str]:
 provide an *absolute* path to the certificate file instead.
 """
 
+
+def _remove_stale_otel_sdk_packages():
+    """Hack to remove stale opentelemetry sdk packages from the charm's python venv.
+
+    See https://github.com/canonical/grafana-agent-operator/issues/146 and
+    https://bugs.launchpad.net/juju/+bug/2058335 for more context. This patch can be removed after
+    this juju issue is resolved and sufficient time has passed to expect most users of this library
+    have migrated to the patched version of juju.  When this patch is removed, un-ignore rule E402 for this file in the pyproject.toml (see setting
+    [tool.ruff.lint.per-file-ignores] in pyproject.toml).
+
+    This only has an effect if executed on an upgrade-charm event.
+    """
+    # all imports are local to keep this function standalone, side-effect-free, and easy to revert later
+    import os
+
+    if os.getenv("JUJU_DISPATCH_PATH") != "hooks/upgrade-charm":
+        return
+
+    import logging
+    import shutil
+    from collections import defaultdict
+
+    from importlib_metadata import distributions
+
+    otel_logger = logging.getLogger("charm_tracing_otel_patcher")
+    otel_logger.debug("Applying _remove_stale_otel_sdk_packages patch on charm upgrade")
+    # group by name all distributions starting with "opentelemetry_"
+    otel_distributions = defaultdict(list)
+    for distribution in distributions():
+        name = distribution._normalized_name  # type: ignore
+        if name.startswith("opentelemetry_"):
+            otel_distributions[name].append(distribution)
+
+    otel_logger.debug(f"Found {len(otel_distributions)} opentelemetry distributions")
+
+    # If we have multiple distributions with the same name, remove any that have 0 associated files
+    for name, distributions_ in otel_distributions.items():
+        if len(distributions_) <= 1:
+            continue
+
+        otel_logger.debug(f"Package {name} has multiple ({len(distributions_)}) distributions.")
+        for distribution in distributions_:
+            if not distribution.files:  # Not None or empty list
+                path = distribution._path  # type: ignore
+                otel_logger.info(f"Removing empty distribution of {name} at {path}.")
+                shutil.rmtree(path)
+
+    otel_logger.debug("Successfully applied _remove_stale_otel_sdk_packages patch. ")
+
+
+_remove_stale_otel_sdk_packages()
+
+
 import functools
 import inspect
 import logging
 import os
-import shutil
 from contextlib import contextmanager
 from contextvars import Context, ContextVar, copy_context
-from importlib.metadata import distributions
 from pathlib import Path
 from typing import (
     Any,
@@ -199,14 +250,15 @@ def my_tracing_endpoint(self) -> Optional[str]:
 from opentelemetry.sdk.resources import Resource
 from opentelemetry.sdk.trace import Span, TracerProvider
 from opentelemetry.sdk.trace.export import BatchSpanProcessor
-from opentelemetry.trace import INVALID_SPAN, Tracer
-from opentelemetry.trace import get_current_span as otlp_get_current_span
 from opentelemetry.trace import (
+    INVALID_SPAN,
+    Tracer,
     get_tracer,
     get_tracer_provider,
     set_span_in_context,
     set_tracer_provider,
 )
+from opentelemetry.trace import get_current_span as otlp_get_current_span
 from ops.charm import CharmBase
 from ops.framework import Framework
 
@@ -219,7 +271,7 @@ def my_tracing_endpoint(self) -> Optional[str]:
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
 
-LIBPATCH = 13
+LIBPATCH = 14
 
 PYDEPS = ["opentelemetry-exporter-otlp-proto-http==1.21.0"]
 
@@ -361,30 +413,6 @@ def _get_server_cert(
     return server_cert
 
 
-def _remove_stale_otel_sdk_packages():
-    """Hack to remove stale opentelemetry sdk packages from the charm's python venv.
-
-    See https://github.com/canonical/grafana-agent-operator/issues/146 and
-    https://bugs.launchpad.net/juju/+bug/2058335 for more context. This patch can be removed after
-    this juju issue is resolved and sufficient time has passed to expect most users of this library
-    have migrated to the patched version of juju.
-
-    This only does something if executed on an upgrade-charm event.
-    """
-    if os.getenv("JUJU_DISPATCH_PATH") == "hooks/upgrade-charm":
-        logger.debug("Executing _remove_stale_otel_sdk_packages patch on charm upgrade")
-        # Find any opentelemetry_sdk distributions
-        otel_sdk_distributions = list(distributions(name="opentelemetry_sdk"))
-        # If there is more than 1, inspect each and if it has 0 entrypoints, infer that it is stale
-        if len(otel_sdk_distributions) > 1:
-            for distribution in otel_sdk_distributions:
-                if len(distribution.entry_points) == 0:
-                    # Distribution appears to be empty. Remove it
-                    path = distribution._path  # type: ignore
-                    logger.debug(f"Removing empty opentelemetry_sdk distribution at: {path}")
-                    shutil.rmtree(path)
-
-
 def _setup_root_span_initializer(
     charm_type: _CharmType,
     tracing_endpoint_attr: str,
@@ -420,7 +448,6 @@ def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs):
         # apply hacky patch to remove stale opentelemetry sdk packages on upgrade-charm.
         # it could be trouble if someone ever decides to implement their own tracer parallel to
         # ours and before the charm has inited. We assume they won't.
-        _remove_stale_otel_sdk_packages()
         resource = Resource.create(
             attributes={
                 "service.name": _service_name,

lib/charms/tempo_k8s/v2/tracing.py

@@ -925,7 +925,7 @@ def get_endpoint(
 def charm_tracing_config(
     endpoint_requirer: TracingEndpointRequirer, cert_path: Optional[Union[Path, str]]
 ) -> Tuple[Optional[str], Optional[str]]:
-    """Utility function to determine the charm_tracing config you will likely want.
+    """Return the charm_tracing config you likely want.
 
     If no endpoint is provided:
      disable charm tracing.

pyproject.toml

@@ -40,8 +40,11 @@ line-length = 99
 target-version = ["py38"]
 
 # Linting tools configuration
-[lint]
+[tool.ruff]
 line-length = 99
+extend-exclude = ["__pycache__", "*.egg_info", "*integration/tester*"]
+
+[tool.ruff.lint]
 select = ["E", "W", "F", "C", "N", "D", "I001"]
 extend-ignore = [
     "D203",
@@ -57,8 +60,12 @@ extend-ignore = [
     "D413",
 ]
 ignore = ["E501", "D107"]
-extend-exclude = ["__pycache__", "*.egg_info", "*integration/tester*"]
-per-file-ignores = {"tests/*" = ["D100","D101","D102","D103","D104"]}
+
+[tool.ruff.lint.per-file-ignores]
+"tests/*" = ["D100","D101","D102","D103","D104"]
+# Remove charm_tracing.py E402 when _remove_stale_otel_sdk_packages() is removed
+# from the library
+"lib/charms/tempo_k8s/v1/charm_tracing.py" = ["E402"]
 
 [lint.mccabe]
 max-complexity = 10

src/charm.py

@@ -35,7 +35,6 @@
 from ops.main import main
 from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus
 from ops.pebble import APIError
-
 from tempo import Tempo
 
 logger = logging.getLogger(__name__)
@@ -488,6 +487,7 @@ def is_scaled(self) -> bool:
         return bool(relation.units)
 
     def peers(self) -> Optional[Set[ops.model.Unit]]:
+        """Return charm's peer units."""
         relation = self.model.get_relation("tempo-peers")
         if not relation:
             return None

src/tempo.py

@@ -239,6 +239,7 @@ def restart(self) -> bool:
 
     @property
     def is_running(self) -> bool:
+        """Return whether the container service for tempo is running."""
         return self.container.get_service("tempo").is_running()
 
     def shutdown(self):

tests/integration/test_ingressed_tls.py

@@ -10,9 +10,9 @@
 import requests
 import yaml
 from pytest_operator.plugin import OpsTest
+from tempo import Tempo
 from tenacity import retry, stop_after_attempt, wait_exponential
 
-from tempo import Tempo
 from tests.integration.helpers import get_relation_data
 
 METADATA = yaml.safe_load(Path("./charmcraft.yaml").read_text())

tests/integration/test_tls.py

@@ -10,9 +10,9 @@
 import requests
 import yaml
 from pytest_operator.plugin import OpsTest
+from tempo import Tempo
 from tenacity import retry, stop_after_attempt, wait_exponential
 
-from tempo import Tempo
 from tests.integration.helpers import get_relation_data
 
 METADATA = yaml.safe_load(Path("./charmcraft.yaml").read_text())

tests/interface/conftest.py

@@ -3,13 +3,12 @@
 from unittest.mock import patch
 
 import pytest
+from charm import TempoCharm
 from charms.tempo_k8s.v1.charm_tracing import charm_tracing_disabled
 from interface_tester import InterfaceTester
 from ops.pebble import Layer
 from scenario.state import Container, State
 
-from charm import TempoCharm
-
 
 # Interface tests are centrally hosted at https://github.com/canonical/charm-relation-interfaces.
 # this fixture is used by the test runner of charm-relation-interfaces to test tempo's compliance

tests/scenario/conftest.py

@@ -1,9 +1,8 @@
 from unittest.mock import patch
 
 import pytest
-from scenario import Context
-
 from charm import TempoCharm
+from scenario import Context
 
 
 @pytest.fixture

tests/scenario/helpers.py

@@ -2,7 +2,6 @@
 
 import scenario
 import yaml
-
 from tempo import Tempo
 
 

tests/scenario/test_charm.py

@@ -9,8 +9,8 @@
 from scenario import Container, Mount, Relation, State
 from scenario.sequences import check_builtin_sequences
 from scenario.state import Notice, _BoundNotice
-
 from tempo import Tempo
+
 from tests.scenario.helpers import get_tempo_config
 
 TEMPO_CHARM_ROOT = Path(__file__).parent.parent.parent

tests/scenario/test_ingressed_tracing.py

@@ -4,7 +4,6 @@
 import yaml
 from charms.tempo_k8s.v1.charm_tracing import charm_tracing_disabled
 from scenario import Container, Relation, State
-
 from tempo import Tempo
 
 

tests/scenario/test_tls.py

@@ -2,12 +2,11 @@
 from unittest.mock import patch
 
 import pytest
+from charm import Tempo
 from charms.tempo_k8s.v1.charm_tracing import charm_tracing_disabled
 from charms.tempo_k8s.v2.tracing import TracingProviderAppData, TracingRequirerAppData
 from scenario import Container, Relation, State
 
-from charm import Tempo
-
 
 @pytest.fixture
 def base_state():

tests/scenario/test_tracing_requirer.py

@@ -10,7 +10,6 @@
 )
 from ops import CharmBase, Framework, RelationBrokenEvent, RelationChangedEvent
 from scenario import Context, Relation, State
-
 from tempo import Tempo
 
 

tests/unit/test_charm.py

@@ -6,11 +6,10 @@
 import unittest
 from unittest.mock import patch
 
+from charm import TempoCharm
 from ops.model import ActiveStatus
 from ops.testing import Harness
 
-from charm import TempoCharm
-
 CONTAINER_NAME = "tempo"
 
 

tests/unit/test_tempo.py

@@ -1,7 +1,6 @@
 from unittest.mock import patch
 
 import pytest
-
 from tempo import Tempo