address review comments

Author: dimaqq

ops/_tracing/__init__.py

@@ -18,6 +18,7 @@
 
 from __future__ import annotations
 
+import warnings
 from contextlib import contextmanager
 from dataclasses import dataclass
 from typing import Generator
@@ -29,12 +30,14 @@
 
 tracer = opentelemetry.trace.get_tracer('ops', ops.version.version)
 
+
 @dataclass
 class _Config:
     """Tracing destination configuration.
 
     NOTE: that empty string values may be coerced to None.
     """
+
     url: str | None
     """The URL to send tracing data to."""
     ca: str | None
@@ -50,9 +53,19 @@ class _Config:
 except ImportError:
 
     def mark_observed() -> None: ...
-    def set_tracing_destination(config: _Config) -> None: ...
+
+    def set_tracing_destination(config: _Config) -> None:
+        warnings.warn(
+            'Tracing is not enabled, but set_tracing_destination() was called. '
+            "Ensure 'ops[tracing]' is installed.",
+            UserWarning,
+            stacklevel=3,
+        )
+
     @contextmanager
-    def setup_tracing(juju_context: _JujuContext, charm_class_name: str) -> Generator[None, None, None]:
+    def setup_tracing(
+        juju_context: _JujuContext, charm_class_name: str
+    ) -> Generator[None, None, None]:
         yield
 
 

ops/_tracing/api.py

@@ -19,6 +19,7 @@
 
 import ops
 from ops._tracing import _Config
+from ops.charm import RelationRole
 
 from .vendor.charms.certificate_transfer_interface.v1.certificate_transfer import (
     CertificateTransferRequires,
@@ -44,7 +45,7 @@
 # ```
 # {"receivers": ["otlp_http"]}
 # ```
-# 
+#
 # - client reads this from remote_app_data:
 # ```
 # {"receivers": [
@@ -58,7 +59,7 @@
 
 
 class Tracing(ops.Object):
-    """FIXME docstring."""
+    """Tracing service."""
 
     _certificate_transfer: CertificateTransferRequires | None
 
@@ -69,46 +70,70 @@ def __init__(
         ca_relation_name: str | None = None,
         ca_data: str | None = None,
     ):
-        """FIXME docstring.
-
-        Include `ops[tracing]` in your dependencies.
-
-        Declare the relations that the charm supports:
-
-        ```yaml
-        requires:
-            charm-tracing:
-                interface: tracing
-                limit: 1
-                optional: true
-            send-ca-cert:
-                interface: certificate_transfer
-                limit: 1
-                optional: true
-        ```
-
-        Initialise the tracing thing (FIXME) with the names of these relations.
-
-        ```py
-        import ops.tracing
-
-        class SomeCharm(ops.CharmBase):
-            def __init__(self, framework: ops.Framework):
-                ...
-                self._tracing = ops.tracing.Tracing(
-                    tracing_relation_name="charm-tracing",
-                    ca_relation_name="send-ca-cart",
-                )
-        ```
+        """Initialise the tracing service.
+
+        Args:
+            charm: your charm instange
+            tracing_relation_name: the name of the relation that provides the
+                destination to send tracing data to.
+            ca_relation_name: the name of the relation that provides the CA
+                list to validate the tracing destination against.
+            ca_data: a fixed CA list (PEM bundle, a multi-line string).
+
+        If the destination is resolved to an HTTPS URL, a CA list is required
+        to establish a secure connection.
+
+        The CA list can be provided over a relation via ``ca_relation_name=``
+        argument, as a fixed string via ``ca_data`` argument, or the system CA
+        list will be used if the earlier two are both ``None``.
+
+        Usage:
+            - Include ``ops[tracing]`` in your dependencies.
+            - Declare the relations that the charm supports.
+            - Initialise ``Tracing`` with the names of these relations.
+
+        Example::
+            # charmcraft.yaml
+            requires:
+                charm-tracing:
+                    interface: tracing
+                    limit: 1
+                    optional: true
+                send-ca-cert:
+                    interface: certificate_transfer
+                    limit: 1
+                    optional: true
+
+            # src/charm.py
+            import ops.tracing
+
+            class SomeCharm(ops.CharmBase):
+                def __init__(self, framework: ops.Framework):
+                    ...
+                    self.tracing = ops.tracing.Tracing(
+                        tracing_relation_name="charm-tracing",
+                        ca_relation_name="send-ca-cart",
+                    )
         """
         super().__init__(charm, f'{tracing_relation_name}+{ca_relation_name}')
         self.charm = charm
         self.tracing_relation_name = tracing_relation_name
         self.ca_relation_name = ca_relation_name
         self.ca_data = ca_data
 
-        # FIXME: Pietro recommends inspecting charm meta to validate the interface name
-        assert self.charm.meta.relations[tracing_relation_name].interface_name == 'tracing'
+        # NOTE: Pietro recommends inspecting charm meta to validate the relation
+        # that way a badly written charm crashes in early testing.
+        if not (relation := self.charm.meta.relations.get(tracing_relation_name)):
+            raise ValueError(f'{tracing_relation_name} is not declared in charm metadata')
+        if (relation_role := relation.role) is not RelationRole.requires:
+            raise ValueError(
+                f"{tracing_relation_name} {relation_role=} when 'requires' is expected"
+            )
+        if (interface_name := relation.interface_name) != 'tracing':
+            raise ValueError(
+                f"{tracing_relation_name=} {interface_name=} when 'tracing' is expected"
+            )
+
         self._tracing = TracingEndpointRequirer(
             self.charm,
             tracing_relation_name,
@@ -121,12 +146,7 @@ def __init__(self, framework: ops.Framework):
         # RelationRoleMismatchError,
 
         for event in (
-            # FIXME: validate this
-            # - the start event may happen after relation-joined?
-            # - the k8s container died and got restarted
-            # - the vm [smth] crashed and got restarted
             self.charm.on.start,
-            # In case the previous charm version has a relation but didn't save the URL
             self.charm.on.upgrade_charm,
             self._tracing.on.endpoint_changed,
             self._tracing.on.endpoint_removed,
@@ -135,11 +155,20 @@ def __init__(self, framework: ops.Framework):
 
         self._certificate_transfer = None
         if ca_relation_name:
+            if not (relation := self.charm.meta.relations.get(ca_relation_name)):
+                raise ValueError(f'{ca_relation_name} is not declared in charm metadata')
+            if (relation_role := relation.role) is not RelationRole.requires:
+                raise ValueError(
+                    f"{ca_relation_name} {relation_role=} when 'requires' is expected"
+                )
+            if (interface_name := relation.interface_name) != 'certificate_transfer':
+                raise ValueError(
+                    f"{ca_relation_name=} {interface_name=} when 'certificate_transfer' "
+                    'is expected'
+                )
+
             self._certificate_transfer = CertificateTransferRequires(charm, ca_relation_name)
-            assert (
-                self.charm.meta.relations[ca_relation_name].interface_name
-                == 'certificate_transfer'
-            )
+
             for event in (
                 self._certificate_transfer.on.certificate_set_updated,
                 self._certificate_transfer.on.certificates_removed,
@@ -168,8 +197,8 @@ def _get_config(self) -> _Config:
         ca_rel_id = ca_rel.id if ca_rel else None
 
         if ca_rel and self._certificate_transfer.is_ready(ca_rel):
-            return _Config(url, '\n'.join(
-                sorted(self._certificate_transfer.get_all_certificates(ca_rel_id))
-            ))
+            return _Config(
+                url, '\n'.join(sorted(self._certificate_transfer.get_all_certificates(ca_rel_id)))
+            )
         else:
             return _Config(None, None)

ops/_tracing/buffer.py

@@ -149,13 +149,17 @@ def tx(self, *, timeout: float = DB_TIMEOUT, readonly: bool = False):
     def get_destination(self) -> _Config:
         with self.tx(readonly=True) as conn:
             settings = {k: v for k, v in conn.execute("""SELECT key, value FROM settings""")}
-            return _Config(settings.get("url") or None, settings.get("ca") or None)
+            return _Config(settings.get('url') or None, settings.get('ca') or None)
 
     @retry
     def set_destination(self, config: _Config) -> None:
         with self.tx() as conn:
-            conn.execute("""REPLACE INTO settings(key, value) VALUES('url', ?)""", (config.url or "",))
-            conn.execute("""REPLACE INTO settings(key, value) VALUES('ca', ?)""", (config.ca or "",))
+            conn.execute(
+                """REPLACE INTO settings(key, value) VALUES('url', ?)""", (config.url or '',)
+            )
+            conn.execute(
+                """REPLACE INTO settings(key, value) VALUES('ca', ?)""", (config.ca or '',)
+            )
 
     # FIXME: currently unused
     # If most charms observe the CollectStatusEvent, then an event is observed on every dispatch.

ops/_tracing/export.py

@@ -196,7 +196,9 @@ def suppress_juju_log_handler():
 
 
 @contextlib.contextmanager
-def setup_tracing(juju_context: _JujuContext, charm_class_name: str) -> Generator[None, None, None]:
+def setup_tracing(
+    juju_context: _JujuContext, charm_class_name: str
+) -> Generator[None, None, None]:
     global _exporter
     # FIXME is it ever possible for unit_name to be unset (empty)?
     app_name, unit_number = juju_context.unit_name.split('/', 1)
@@ -234,9 +236,9 @@ def set_tracing_destination(config: _Config) -> None:
     """Configure the destination service for tracing data.
 
     Args:
-        url: The URL of the telemetry service to send tracing data to.
-        ca: The PEM formatted CA list.
-            Only in use if the URL is an HTTPS URL.
+        config: destination configuration with two fields: ``.url``, the URL of the
+            telemetry service to send tracing data to and ``.ca``, the CA list (PEM
+            bundle, a multi-line string) if the URL is an HTTPS URL.
     """
     if not _exporter:
         return

test/fixme_charm/test/conftest.py

@@ -27,8 +27,8 @@
     sys.path.insert(0, '')
 
 
-import ops.testing
 import ops._tracing
+import ops.testing
 from ops.jujucontext import _JujuContext
 
 

tracing/ops_tracing/__init__.py

@@ -0,0 +1,14 @@
+# Copyright 2025 Canonical Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# 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.
+"""The tracing facility of the Operator Framework."""