diff --git a/dont-merge/fake-charm.py b/dont-merge/fake-charm.py index 5e6427484..2ef138876 100755 --- a/dont-merge/fake-charm.py +++ b/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: diff --git a/ops/__init__.py b/ops/__init__.py index 3d954b286..cbfc58246 100644 --- a/ops/__init__.py +++ b/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__ diff --git a/ops/_main.py b/ops/_main.py index 2406430c0..0032230d7 100644 --- a/ops/_main.py +++ b/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(). diff --git a/ops/_private/yaml.py b/ops/_private/yaml.py index 294bfe2f0..b6b6f4d82 100644 --- a/ops/_private/yaml.py +++ b/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 diff --git a/ops/_tracing/__init__.py b/ops/_tracing/__init__.py index b5bd52b14..b6ada1beb 100644 --- a/ops/_tracing/__init__.py +++ b/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) diff --git a/ops/charm.py b/ops/charm.py index 300fb453b..e0922d033 100644 --- a/ops/charm.py +++ b/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. diff --git a/ops/model.py b/ops/model.py index 065820354..ee1d6c876 100644 --- a/ops/model.py +++ b/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