From 7d15c7bff0bea414480b529b15e12e3072784409 Mon Sep 17 00:00:00 2001 From: Dima Tisnek Date: Fri, 18 Jul 2025 22:09:27 +0900 Subject: [PATCH 1/5] feat: instrument the snap charm library --- lib/charms/operator_libs_linux/v2/snap.py | 54 +++++++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/lib/charms/operator_libs_linux/v2/snap.py b/lib/charms/operator_libs_linux/v2/snap.py index b0d65017..c0627390 100644 --- a/lib/charms/operator_libs_linux/v2/snap.py +++ b/lib/charms/operator_libs_linux/v2/snap.py @@ -54,6 +54,10 @@ except snap.SnapError as e: logger.error("An exception occurred when installing snaps. Reason: %s" % e.message) ``` + +Dependencies: +Note that this module requires `opentelemetry-api`, which is already included into +your charm's virtual environment via `ops >= 2.21`. """ from __future__ import annotations @@ -85,6 +89,8 @@ TypeVar, ) +import opentelemetry.trace + if typing.TYPE_CHECKING: # avoid typing_extensions import at runtime from typing_extensions import NotRequired, ParamSpec, Required, Self, TypeAlias, Unpack @@ -93,6 +99,7 @@ _T = TypeVar("_T") logger = logging.getLogger(__name__) +tracer = opentelemetry.trace.get_tracer(__name__) # The unique Charmhub library identifier, never change it LIBID = "05394e5893f94f2d90feb7cbe6b633cd" @@ -102,7 +109,9 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 12 +LIBPATCH = 13 + +PYDEPS = ["opentelemetry-api"] # Regex to locate 7-bit C1 ANSI sequences @@ -277,7 +286,9 @@ def _from_called_process_error(cls, msg: str, error: CalledProcessError) -> Self lines.extend(['Stderr:', error.stderr]) try: cmd = ['journalctl', '--unit', 'snapd', '--lines', '20'] - logs = subprocess.check_output(cmd, text=True) + with tracer.start_as_current_span(cmd[0]) as span: + span.set_attribute("argv", cmd) + logs = subprocess.check_output(cmd, text=True) except Exception as e: lines.extend(['Error fetching logs:', str(e)]) else: @@ -356,7 +367,9 @@ def _snap(self, command: str, optargs: Iterable[str] | None = None) -> str: optargs = optargs or [] args = ["snap", command, self._name, *optargs] try: - return subprocess.check_output(args, text=True, stderr=subprocess.PIPE) + with tracer.start_as_current_span(args[0]) as span: + span.set_attribute("argv", args) + return subprocess.check_output(args, text=True, stderr=subprocess.PIPE) except CalledProcessError as e: msg = f'Snap: {self._name!r} -- command {args!r} failed!' raise SnapError._from_called_process_error(msg=msg, error=e) from e @@ -384,7 +397,9 @@ def _snap_daemons( args = ["snap", *command, *services] try: - return subprocess.run(args, text=True, check=True, capture_output=True) + with tracer.start_as_current_span(args[0]) as span: + span.set_attribute("argv", args) + return subprocess.run(args, text=True, check=True, capture_output=True) except CalledProcessError as e: msg = f'Snap: {self._name!r} -- command {args!r} failed!' raise SnapError._from_called_process_error(msg=msg, error=e) from e @@ -491,7 +506,9 @@ def connect(self, plug: str, service: str | None = None, slot: str | None = None args = ["snap", *command] try: - subprocess.run(args, text=True, check=True, capture_output=True) + with tracer.start_as_current_span(args[0]) as span: + span.set_attribute("argv", args) + subprocess.run(args, text=True, check=True, capture_output=True) except CalledProcessError as e: msg = f'Snap: {self._name!r} -- command {args!r} failed!' raise SnapError._from_called_process_error(msg=msg, error=e) from e @@ -523,7 +540,9 @@ def alias(self, application: str, alias: str | None = None) -> None: alias = application args = ["snap", "alias", f"{self.name}.{application}", alias] try: - subprocess.run(args, text=True, check=True, capture_output=True) + with tracer.start_as_current_span(args[0]) as span: + span.set_attribute("argv", args) + subprocess.run(args, text=True, check=True, capture_output=True) except CalledProcessError as e: msg = f'Snap: {self._name!r} -- command {args!r} failed!' raise SnapError._from_called_process_error(msg=msg, error=e) from e @@ -913,7 +932,16 @@ def _request_raw( request = urllib.request.Request(url, method=method, data=data, headers=headers) # noqa: S310 try: - response = self.opener.open(request, timeout=self.timeout) + with tracer.start_as_current_span(method) as span: + span.set_attributes({"url.full": url, "http.request.method": method}) + try: + response: http.client.HTTPResponse = self.opener.open( + request, timeout=self.timeout + ) + span.set_attribute("http.response.status_code", response.status) + except urllib.error.HTTPError as e: + span.set_attribute("http.response.status_code", e.code) + raise except urllib.error.HTTPError as e: code = e.code status = e.reason @@ -1280,7 +1308,13 @@ def install_local( if dangerous: args.append("--dangerous") try: - result = subprocess.check_output(args, text=True, stderr=subprocess.PIPE).splitlines()[-1] + with tracer.start_as_current_span(args[0]) as span: + span.set_attribute("argv", args) + result = subprocess.check_output( + args, + text=True, + stderr=subprocess.PIPE, + ).splitlines()[-1] snap_name, _ = result.split(" ", 1) snap_name = ansi_filter.sub("", snap_name) @@ -1309,7 +1343,9 @@ def _system_set(config_item: str, value: str) -> None: """ args = ["snap", "set", "system", f"{config_item}={value}"] try: - subprocess.run(args, text=True, check=True, capture_output=True) + with tracer.start_as_current_span(args[0]) as span: + span.set_attribute("argv", args) + subprocess.run(args, text=True, check=True, capture_output=True) except CalledProcessError as e: msg = f"Failed setting system config '{config_item}' to '{value}'" raise SnapError._from_called_process_error(msg=msg, error=e) from e From a21f0a43c6bf3e123a9ac0002b8465c8505ff516 Mon Sep 17 00:00:00 2001 From: Dima Tisnek Date: Sat, 19 Jul 2025 07:25:24 +0900 Subject: [PATCH 2/5] maybe a better way --- lib/charms/operator_libs_linux/v2/snap.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/charms/operator_libs_linux/v2/snap.py b/lib/charms/operator_libs_linux/v2/snap.py index c0627390..4370eb12 100644 --- a/lib/charms/operator_libs_linux/v2/snap.py +++ b/lib/charms/operator_libs_linux/v2/snap.py @@ -932,16 +932,7 @@ def _request_raw( request = urllib.request.Request(url, method=method, data=data, headers=headers) # noqa: S310 try: - with tracer.start_as_current_span(method) as span: - span.set_attributes({"url.full": url, "http.request.method": method}) - try: - response: http.client.HTTPResponse = self.opener.open( - request, timeout=self.timeout - ) - span.set_attribute("http.response.status_code", response.status) - except urllib.error.HTTPError as e: - span.set_attribute("http.response.status_code", e.code) - raise + response = self.opener.open(request, timeout=self.timeout) except urllib.error.HTTPError as e: code = e.code status = e.reason @@ -960,15 +951,20 @@ def _request_raw( def get_installed_snaps(self) -> list[dict[str, JSONType]]: """Get information about currently installed snaps.""" - return self._request("GET", "snaps") # type: ignore + with tracer.start_as_current_span("get_installed_snaps"): + return self._request("GET", "snaps") # type: ignore def get_snap_information(self, name: str) -> dict[str, JSONType]: """Query the snap server for information about single snap.""" - return self._request("GET", "find", {"name": name})[0] # type: ignore + with tracer.start_as_current_span("get_snap_information") as span: + span.set_attribute("name", name) + return self._request("GET", "find", {"name": name})[0] # type: ignore def get_installed_snap_apps(self, name: str) -> list[dict[str, JSONType]]: """Query the snap server for apps belonging to a named, currently installed snap.""" - return self._request("GET", "apps", {"names": name, "select": "service"}) # type: ignore + with tracer.start_as_current_span("get_installed_snap_apps") as span: + span.set_attribute("name", name) + return self._request("GET", "apps", {"names": name, "select": "service"}) # type: ignore def _put_snap_conf(self, name: str, conf: dict[str, JSONAble]) -> None: """Set the configuration details for an installed snap.""" From daed0a17c9f30925a25304db209f0fda097c1f96 Mon Sep 17 00:00:00 2001 From: Dima Tisnek Date: Fri, 25 Jul 2025 17:40:04 +0900 Subject: [PATCH 3/5] chore: add test vector for get_snap_apps --- tests/unit/test_snap.py | 50 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_snap.py b/tests/unit/test_snap.py index 5247d4ce..c1ef3556 100644 --- a/tests/unit/test_snap.py +++ b/tests/unit/test_snap.py @@ -22,7 +22,7 @@ patch("charms.operator_libs_linux.v2.snap._cache_init", lambda x: x).start() -lazy_load_result = r""" +snap_information_result = r""" { "type": "sync", "status-code": 200, @@ -195,6 +195,19 @@ } """ +snap_apps_result = { + "type": "sync", + "result": [ + { + 'snap': 'juju', + 'name': 'fetch-oci', + 'daemon': 'oneshot', + 'daemon-scope': 'system', + 'enabled': True, + }, + ], +} + class SnapCacheTester(snap.SnapCache): def __init__(self): @@ -266,9 +279,9 @@ def test_can_lazy_load_snap_info(self, mock_exists, m): m.return_value.__next__ = lambda self: next(iter(self.readline, "")) mock_exists.return_value = True s = SnapCacheTester() - s._snap_client.get_snap_information.return_value = json.loads(lazy_load_result)["result"][ - 0 - ] + s._snap_client.get_snap_information.return_value = json.loads(snap_information_result)[ + "result" + ][0] s._load_available_snaps() self.assertIn("curl", s._snap_map) @@ -929,7 +942,7 @@ def setUp(self, mock_exists, m): installed_result )["result"] snap._Cache.cache._snap_client.get_snap_information.return_value = json.loads( - lazy_load_result + snap_information_result )["result"][0] snap._Cache.cache._load_installed_snaps() snap._Cache.cache._load_available_snaps() @@ -1338,3 +1351,30 @@ def test_held(self, mock_subprocess: MagicMock): self.assertEqual(foo.held, False) mock_subprocess.return_value = {"hold:": "key isn't checked"} self.assertEqual(foo.held, True) + + +@pytest.fixture +def fake_request(monkeypatch: pytest.MonkeyPatch): + request = MagicMock() + monkeypatch.setattr("charms.operator_libs_linux.v2.snap.SnapClient._request", request) + return request + + +@pytest.fixture +def snap_client(): + return snap.SnapClient(socket_path="/does/not/exist") + + +def test_get_installed_snaps(snap_client: snap.SnapClient, fake_request: MagicMock): + fake_request.return_value = json.loads(installed_result)["result"] + snap_client.get_installed_snaps() + + +def test_get_installed_snap_apps(snap_client: snap.SnapClient, fake_request: MagicMock): + fake_request.return_value = snap_apps_result["result"] + snap_client.get_installed_snap_apps("some-snap") + + +def test_get_snap_information(snap_client: snap.SnapClient, fake_request: MagicMock): + fake_request.return_value = json.loads(snap_information_result)["result"] + snap_client.get_snap_information("some-snap") From bae33805c9b12037f966208a64f8d70186b6454f Mon Sep 17 00:00:00 2001 From: Dima Tisnek Date: Fri, 25 Jul 2025 17:49:39 +0900 Subject: [PATCH 4/5] better names --- tests/unit/test_snap.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/tests/unit/test_snap.py b/tests/unit/test_snap.py index c1ef3556..5ec0d8d5 100644 --- a/tests/unit/test_snap.py +++ b/tests/unit/test_snap.py @@ -22,7 +22,7 @@ patch("charms.operator_libs_linux.v2.snap._cache_init", lambda x: x).start() -snap_information_result = r""" +snap_information_response = r""" { "type": "sync", "status-code": 200, @@ -103,7 +103,7 @@ } """ -installed_result = r""" +installed_snaps_response = r""" { "type": "sync", "status-code": 200, @@ -195,7 +195,7 @@ } """ -snap_apps_result = { +installed_snap_apps_response = { "type": "sync", "result": [ { @@ -279,7 +279,7 @@ def test_can_lazy_load_snap_info(self, mock_exists, m): m.return_value.__next__ = lambda self: next(iter(self.readline, "")) mock_exists.return_value = True s = SnapCacheTester() - s._snap_client.get_snap_information.return_value = json.loads(snap_information_result)[ + s._snap_client.get_snap_information.return_value = json.loads(snap_information_response)[ "result" ][0] s._load_available_snaps() @@ -296,7 +296,9 @@ def test_can_lazy_load_snap_info(self, mock_exists, m): def test_can_load_installed_snap_info(self, mock_exists): mock_exists.return_value = True s = SnapCacheTester() - s._snap_client.get_installed_snaps.return_value = json.loads(installed_result)["result"] + s._snap_client.get_installed_snaps.return_value = json.loads(installed_snaps_response)[ + "result" + ] s._load_installed_snaps() @@ -628,20 +630,24 @@ def test_snap_unhold(self, mock_subprocess: MagicMock): @patch("charms.operator_libs_linux.v2.snap.SnapClient.get_installed_snap_apps") def test_apps_property(self, patched): s = SnapCacheTester() - s._snap_client.get_installed_snaps.return_value = json.loads(installed_result)["result"] + s._snap_client.get_installed_snaps.return_value = json.loads(installed_snaps_response)[ + "result" + ] s._load_installed_snaps() - patched.return_value = json.loads(installed_result)["result"][0]["apps"] + patched.return_value = json.loads(installed_snaps_response)["result"][0]["apps"] self.assertEqual(len(s["charmcraft"].apps), 2) self.assertIn({"snap": "charmcraft", "name": "charmcraft"}, s["charmcraft"].apps) @patch("charms.operator_libs_linux.v2.snap.SnapClient.get_installed_snap_apps") def test_services_property(self, patched): s = SnapCacheTester() - s._snap_client.get_installed_snaps.return_value = json.loads(installed_result)["result"] + s._snap_client.get_installed_snaps.return_value = json.loads(installed_snaps_response)[ + "result" + ] s._load_installed_snaps() - patched.return_value = json.loads(installed_result)["result"][0]["apps"] + patched.return_value = json.loads(installed_snaps_response)["result"][0]["apps"] self.assertEqual(len(s["charmcraft"].services), 1) self.assertDictEqual( s["charmcraft"].services, @@ -939,10 +945,10 @@ def setUp(self, mock_exists, m): mock_exists.return_value = True snap._Cache.cache = SnapCacheTester() snap._Cache.cache._snap_client.get_installed_snaps.return_value = json.loads( - installed_result + installed_snaps_response )["result"] snap._Cache.cache._snap_client.get_snap_information.return_value = json.loads( - snap_information_result + snap_information_response )["result"][0] snap._Cache.cache._load_installed_snaps() snap._Cache.cache._load_available_snaps() @@ -1366,15 +1372,15 @@ def snap_client(): def test_get_installed_snaps(snap_client: snap.SnapClient, fake_request: MagicMock): - fake_request.return_value = json.loads(installed_result)["result"] + fake_request.return_value = json.loads(installed_snaps_response)["result"] snap_client.get_installed_snaps() def test_get_installed_snap_apps(snap_client: snap.SnapClient, fake_request: MagicMock): - fake_request.return_value = snap_apps_result["result"] + fake_request.return_value = installed_snap_apps_response["result"] snap_client.get_installed_snap_apps("some-snap") def test_get_snap_information(snap_client: snap.SnapClient, fake_request: MagicMock): - fake_request.return_value = json.loads(snap_information_result)["result"] + fake_request.return_value = json.loads(snap_information_response)["result"] snap_client.get_snap_information("some-snap") From cba253ce54fdf5061bd6c8cdb329574a0b75e496 Mon Sep 17 00:00:00 2001 From: Dima Tisnek Date: Tue, 29 Jul 2025 13:51:46 +0900 Subject: [PATCH 5/5] add basic assertions to the new tests --- tests/unit/test_snap.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_snap.py b/tests/unit/test_snap.py index 5ec0d8d5..88e3fe12 100644 --- a/tests/unit/test_snap.py +++ b/tests/unit/test_snap.py @@ -14,7 +14,7 @@ import unittest from subprocess import CalledProcessError from typing import Any, Iterable -from unittest.mock import MagicMock, mock_open, patch +from unittest.mock import ANY, MagicMock, mock_open, patch import fake_snapd as fake_snapd import pytest @@ -1373,14 +1373,26 @@ def snap_client(): def test_get_installed_snaps(snap_client: snap.SnapClient, fake_request: MagicMock): fake_request.return_value = json.loads(installed_snaps_response)["result"] - snap_client.get_installed_snaps() + rv = snap_client.get_installed_snaps() + charmcraft = next(snap for snap in rv if snap["name"] == "charmcraft") + assert charmcraft["version"] == "1.2.1" def test_get_installed_snap_apps(snap_client: snap.SnapClient, fake_request: MagicMock): fake_request.return_value = installed_snap_apps_response["result"] - snap_client.get_installed_snap_apps("some-snap") + rv = snap_client.get_installed_snap_apps("juju") + assert rv == [ + { + "name": "fetch-oci", + "snap": "juju", + "daemon": "oneshot", + "daemon-scope": ANY, + "enabled": ANY, + } + ] def test_get_snap_information(snap_client: snap.SnapClient, fake_request: MagicMock): fake_request.return_value = json.loads(snap_information_response)["result"] - snap_client.get_snap_information("some-snap") + rv = snap_client.get_snap_information("curl") + assert rv["version"] == "7.78.0"