feat: use an event to configure tracing

dont-merge/fake-charm.py

@@ -56,14 +56,19 @@ class FakeCharm(ops.CharmBase):
     def __init__(self, framework: ops.Framework):
         """Dummy docstring."""
         super().__init__(framework)
+        self.framework.observe(self.on.setup_tracing, self._on_setup_tracing)
         self.framework.observe(self.on.start, self._on_start)
         self.framework.observe(self.on.collect_app_status, self._on_collect_app_status)
         self.db_requirer = DatabaseRequirer(self)
         self.framework.observe(self.db_requirer.on.ready, self._on_db_ready)
 
+    def _on_setup_tracing(self, event: ops.SetupTracingEvent) -> None:
+        """Dummy docstring."""
+        self.dummy_load(event)
+        event.set_destination(url='http://localhost:4318/v1/traces')
+
     def _on_start(self, event: ops.StartEvent) -> None:
         """Dummy docstring."""
-        ops.set_tracing_destination(url='http://localhost:4318/v1/traces')
         self.dummy_load(event, 0.0025)
 
     def _on_db_ready(self, event: DatabaseReadyEvent) -> None:

ops/__init__.py

@@ -54,7 +54,6 @@
 # that those symbols are part of the public API, so we have to add __all__.
 __all__ = [  # noqa: RUF022 `__all__` is not sorted
     '__version__',
-    'set_tracing_destination',
     'main',
     'pebble',
     # From charm.py
@@ -100,6 +99,7 @@
     'SecretExpiredEvent',
     'SecretRemoveEvent',
     'SecretRotateEvent',
+    'SetupTracingEvent',
     'StartEvent',
     'StopEvent',
     'StorageAttachedEvent',
@@ -244,6 +244,7 @@
     SecretExpiredEvent,
     SecretRemoveEvent,
     SecretRotateEvent,
+    SetupTracingEvent,
     StartEvent,
     StopEvent,
     StorageAttachedEvent,
@@ -334,7 +335,6 @@
 # NOTE: don't import testing or Harness here, as that's a test-time concern
 # rather than a runtime concern.
 
-from ._tracing import set_tracing_destination
 from .version import version as __version__
 
 

ops/_main.py

@@ -496,6 +496,7 @@ def _make_framework(self, dispatcher: _Dispatcher):
 
     def _emit(self):
         """Emit the event on the charm."""
+        ops.charm._setup_tracing(self.charm)
         # TODO: Remove the collect_metrics check below as soon as the relevant
         #       Juju changes are made. Also adjust the docstring on
         #       EventBase.defer().

ops/_private/yaml.py

@@ -30,8 +30,8 @@
 def safe_load(stream: Union[str, TextIO]) -> Any:
     """Same as yaml.safe_load, but use fast C loader if available."""
     if not isinstance(stream, TextIO):
-        opentelemetry.trace.get_current_span().set_attribute("len", len(stream))
-    opentelemetry.trace.get_current_span().set_attribute("stream", isinstance(stream, TextIO))
+        opentelemetry.trace.get_current_span().set_attribute('len', len(stream))
+    opentelemetry.trace.get_current_span().set_attribute('stream', isinstance(stream, TextIO))
     return yaml.load(stream, Loader=_safe_loader)  # noqa: S506
 
 
@@ -41,6 +41,6 @@ def safe_dump(data: Any, stream: Optional[TextIO] = None) -> str:
     rv = yaml.dump(data, stream=stream, Dumper=_safe_dumper)
     if stream is None:
         assert rv is not None
-        opentelemetry.trace.get_current_span().set_attribute("len", len(rv))
-    opentelemetry.trace.get_current_span().set_attribute("stream", stream is not None)
+        opentelemetry.trace.get_current_span().set_attribute('len', len(rv))
+    opentelemetry.trace.get_current_span().set_attribute('stream', stream is not None)
     return rv  # type: ignore

ops/_tracing/__init__.py

@@ -20,13 +20,10 @@
 
 import logging
 
-import opentelemetry.trace
-
 from ops._tracing import hacks
 
 # FIXME must this hack be run before OTEL packages are imported?
 hacks.remove_stale_otel_sdk_packages()
-tracer = opentelemetry.trace.get_tracer(__name__)
 
 
 try:
@@ -42,20 +39,12 @@ def setup_tracing(charm_class_name: str) -> None:
     export.setup_tracing(charm_class_name)
 
 
-@tracer.start_as_current_span('ops.set_tracing_destination')  # type: ignore
 def set_tracing_destination(
     *,
     url: str | None,
     ca: str | None = None,
 ) -> None:
-    """Configure the destination service for tracing data.
-
-    Args:
-        url: The URL of the "collector", the destination for tracing data.
-             Example: 'http://localhost:4318/v1/traces'
-        ca: The PEM-formatted server certificate authority list.
-            This argument is in effect only if the ``url`` is an HTTPS URL.
-    """
+    """Configure the destination service for tracing data."""
     if not export:
         return
     export.set_tracing_destination(url=url, ca=ca)

ops/charm.py

@@ -38,6 +38,7 @@
 
 import opentelemetry.trace
 
+import ops._tracing
 from ops import model
 from ops._private import yaml
 from ops.framework import (
@@ -1179,6 +1180,24 @@ def add_status(self, status: model.StatusBase):
             model_.unit._collected_statuses.append(status)
 
 
+class SetupTracingEvent(LifecycleEvent):
+    """FIXME docstring."""
+
+    def set_destination(self, *, url: Optional[str], ca: Optional[str] = None) -> None:
+        """Configure the destination service for tracing data.
+
+        Args:
+            url: The URL of the "collector", the destination for tracing data.
+                 Example: 'http://localhost:4318/v1/traces'
+            ca: The PEM-formatted server certificate authority list.
+                This argument is in effect only if the ``url`` is an HTTPS URL.
+        """
+        with tracer.start_as_current_span('ops.SetupTracingEvent.set_destination') as span:  # type: ignore
+            span.set_attribute('url', url)  # type: ignore
+            span.set_attribute('ca', ca is not None)  # type: ignore
+            ops._tracing.set_tracing_destination(url=url, ca=ca)
+
+
 class CharmEvents(ObjectEvents):
     """Events generated by Juju pertaining to application lifecycle.
 
@@ -1291,6 +1310,11 @@ class CharmEvents(ObjectEvents):
     (see :class:`~ops.CollectStatusEvent`).
     """
 
+    setup_tracing = EventSource(SetupTracingEvent)
+    """Triggered at the beginning of every hook to set up the tracing facility.
+    (see :class:`~ops.SetupTracingEvent`).
+    """
+
 
 class CharmBase(Object):
     """Base class that represents the charm overall.
@@ -1389,6 +1413,14 @@ def config(self) -> model.ConfigData:
         return self.model.config
 
 
+def _setup_tracing(charm: CharmBase) -> None:
+    """FIXME docstring."""
+    # FIXME if the body is so simple, does this function make sense?
+    # would it be better to collect the arguments,
+    # and then set the tracing config out of band?
+    charm.on.setup_tracing.emit()
+
+
 def _evaluate_status(charm: CharmBase):  # pyright: ignore[reportUnusedFunction]
     """Trigger collect-status events and evaluate and set the highest-priority status.
 

ops/model.py

@@ -902,24 +902,24 @@ def _invalidate(self):
 
     # FIXME isntrument all these?
     def __contains__(self, key: str) -> bool:
-        with tracer.start_as_current_span(f"x in ops.{self.__class__.__name__}"):  # type: ignore
+        with tracer.start_as_current_span(f'x in ops.{self.__class__.__name__}'):  # type: ignore
             return key in self._data
 
     def __len__(self) -> int:
-        with tracer.start_as_current_span(f"len(ops.{self.__class__.__name__})"):  # type: ignore
+        with tracer.start_as_current_span(f'len(ops.{self.__class__.__name__})'):  # type: ignore
             return len(self._data)
 
     def __iter__(self):
-        with tracer.start_as_current_span(f"for x in ops.{self.__class__.__name__}"):  # type: ignore
+        with tracer.start_as_current_span(f'for x in ops.{self.__class__.__name__}'):  # type: ignore
             return iter(self._data)
 
     def __getitem__(self, key: str) -> _LazyValueType:
         # FIXME code path also covers .get()
-        with tracer.start_as_current_span(f"ops.{self.__class__.__name__}[x]"):  # type: ignore
+        with tracer.start_as_current_span(f'ops.{self.__class__.__name__}[x]'):  # type: ignore
             return self._data[key]
 
     def __repr__(self) -> str:
-        with tracer.start_as_current_span(f"repr(ops.{self.__class__.__name__})"):  # type: ignore
+        with tracer.start_as_current_span(f'repr(ops.{self.__class__.__name__})'):  # type: ignore
             return repr(self._data)
 
 
@@ -962,7 +962,7 @@ def __iter__(self) -> Iterable[str]:
         return iter(self._data)
 
     # FIXME: called by bass class for .get() too
-    @tracer.start_as_current_span("ops.RelationMapping[x]")  # type: ignore
+    @tracer.start_as_current_span('ops.RelationMapping[x]')  # type: ignore
     def __getitem__(self, relation_name: str) -> List['Relation']:
         is_peer = relation_name in self._peers
         relation_list: Optional[List[Relation]] = self._data[relation_name]
@@ -2172,7 +2172,7 @@ def __init__(self, names: Iterable[str], backend: '_ModelBackend'):
         self._backend = backend
         self._paths: Dict[str, Optional[Path]] = {name: None for name in names}
 
-    @tracer.start_as_current_span("ops.Resources.fetch")  # type: ignore
+    @tracer.start_as_current_span('ops.Resources.fetch')  # type: ignore
     def fetch(self, name: str) -> Path:
         """Fetch the resource from the controller or store.
 
@@ -2203,7 +2203,7 @@ class Pod:
     def __init__(self, backend: '_ModelBackend'):
         self._backend = backend
 
-    @tracer.start_as_current_span("ops.Pod.set_spec")  # type: ignore
+    @tracer.start_as_current_span('ops.Pod.set_spec')  # type: ignore
     def set_spec(self, spec: 'K8sSpec', k8s_resources: Optional['K8sSpec'] = None):
         """Set the specification for pods that Juju should start in kubernetes.
 
@@ -2237,7 +2237,7 @@ def __iter__(self):
         return iter(self._storage_map)
 
     # FIXME: called by bass class for .get() too
-    @tracer.start_as_current_span("ops.StorageMapping[]")  # type: ignore
+    @tracer.start_as_current_span('ops.StorageMapping[]')  # type: ignore
     def __getitem__(self, storage_name: str) -> List['Storage']:
         if storage_name not in self._storage_map:
             meant = ', or '.join(repr(k) for k in self._storage_map)
@@ -2251,7 +2251,7 @@ def __getitem__(self, storage_name: str) -> List['Storage']:
         return storage_list
 
     # FIXME doesn't seem used by any charm?
-    @tracer.start_as_current_span("ops.StorageMapping.request")  # type: ignore
+    @tracer.start_as_current_span('ops.StorageMapping.request')  # type: ignore
     def request(self, storage_name: str, count: int = 1):
         """Requests new storage instances of a given name.
 
@@ -3389,6 +3389,7 @@ def _run(
         # TODO(benhoyt): all the "type: ignore"s below kinda suck, but I've
         #                been fighting with Pyright for half an hour now...
         try:
+            # FIXME instrument and log this
             result = subprocess.run(args, **kwargs)  # type: ignore
         except subprocess.CalledProcessError as e:
             raise ModelError(e.stderr) from e