SAR-149 Add implementation details for TransactionIdGenerator

IdProviderService has been replaced with TransactionIdGenerator

src/ska/logging/transactions.py

@@ -3,9 +3,11 @@
 """This module provides the transaction logging mechanism."""
 import json
 import logging
+import os
 
 from typing import Mapping, Text
 
+from skuid.client import SkuidClient, get_local_transaction_id
 
 class transaction:
     """Transaction context handler.
@@ -29,21 +31,25 @@ class transaction:
 
      """
 
-    def __init__(self, name, params):
+    def __init__(self, name, params, logger=None):
         if not isinstance(params, Mapping):
             raise TransactionParamsError("params must be dict-like (Mapping)")
+        self.logger = logger
+        if not logger:
+            self.logger = logging
         self._name = name
         self._params = params
 
     def __enter__(self):
         transaction_id = self._get_new_or_existing_transaction_id()
         params_json = json.dumps(self._params)
-        # Question:  should we have a user provided logger instead of using root?
-        logging.info(f"Start transaction {self._name}, {transaction_id}, {params_json}")
+        self.logger.info(f"Start transaction {self._name}, {transaction_id}, {params_json}")
         return transaction_id
 
     def __exit__(self, exc_type, exc_val, exc_tb):
-        pass
+        # TODO: more to be done with func arguments
+
+        self.logger.info(f"Exit transaction {self._name}, {transaction_id}, {params_json}")
 
     def _get_new_or_existing_transaction_id(self):
         transaction_id = self._params.get("transaction_id")
@@ -57,13 +63,21 @@ class transaction:
         return False
 
     def _generate_new_id(self):
-        id_provider = IdProviderService()
-        return id_provider.generate_new_id()
+        id_generator = TransactionIdGenerator()
+        return id_generator.next()
+
+
+class TransactionIdGenerator:
 
+    def __init__(self):
+        if os.environ.get("SKUID_URL"):
+            client = SkuidClient(os.environ["SKUID_URL"])
+            self._get_id = client.fetch_transaction_id
+        else:
+            self._get_id = get_local_transaction_id
 
-class IdProviderService:
-    def generate_new_id(self):
-        return 'xyz-333'
+    def next(self):
+        return self._get_id()
 
 
 class TransactionParamsError(TypeError):

tests/conftest.py

@@ -106,17 +106,18 @@ def get_named_handler(logger, name):
 
 
 @pytest.fixture
-def id_provider_stub(mocker):
-    """Replace the standard transactions.IdProviderService with a simple stub implementation."""
+def id_generator_stub(mocker):
+    """Replace the standard transactions.TransactionIdGenerator with a simple stub implementation."""
 
-    class IdProviderStub:
+    class TransactionIdGeneratorStub:
+        # TODO: change stub implementation to account for local and remote ids
         last_id = "NOT SET"
         call_count = 0
 
-        def generate_new_id(self):
-            IdProviderStub.last_id = "XYZ-789"
-            IdProviderStub.call_count += 1
-            return IdProviderStub.last_id
+        def next(self):
+            TransactionIdGeneratorStub.last_id = "XYZ-789"
+            TransactionIdGeneratorStub.call_count += 1
+            return TransactionIdGeneratorStub.last_id
 
-    mocker.patch('ska.logging.transactions.IdProviderService', IdProviderStub)
-    yield IdProviderStub
+    mocker.patch('ska.logging.transactions.TransactionIdGenerator', TransactionIdGeneratorStub)
+    yield TransactionIdGeneratorStub

tests/test_configuration.py

@@ -133,5 +133,3 @@ class TestOverride:
         override = {"a": {"aa": [], "ab": None, "ac": 3}, "b": {"bb": [1, 2]}}
         out = configuration._override(orig, override)
         assert out == {"a": {"aa": [], "ac": 3}, "b": {"ba": {"c": 10}, "bb": [1, 2]}}
-
-

tests/test_transactions.py

@@ -2,61 +2,65 @@
 
 """Tests for the logging transactions module."""
 import json
+import os
 
 import pytest
+import concurrent.futures
+
+from unittest.mock import patch, MagicMock
 
 from ska.logging import transaction
-from ska.logging.transactions import TransactionParamsError
+from ska.logging.transactions import TransactionParamsError, TransactionIdGenerator
 from tests.conftest import get_first_record_and_log_message
 
 
 class TestTransactionIdGeneration:
     """Tests for :class:`~ska.logging.transaction` related to ID generation."""
 
-    def test_existing_valid_id_is_reused(self, id_provider_stub):
+    def test_existing_valid_id_is_reused(self, id_generator_stub):
         parameters = {"transaction_id": "abc1234", "other": "config"}
         with transaction("name", parameters) as transaction_id:
             assert transaction_id == "abc1234"
 
-    def test_new_id_generated_if_id_is_empty(self, id_provider_stub):
+    def test_new_id_generated_if_id_is_empty(self, id_generator_stub):
         parameters = {"transaction_id": "", "other": "config"}
         with transaction("name", parameters) as transaction_id:
-            assert transaction_id == id_provider_stub.last_id
+            assert transaction_id == id_generator_stub.last_id
 
-    def test_new_id_generated_if_id_is_none(self, id_provider_stub):
+    def test_new_id_generated_if_id_is_none(self, id_generator_stub):
         parameters = {"transaction_id": None, "other": "config"}
         with transaction("name", parameters) as transaction_id:
-            assert transaction_id == id_provider_stub.last_id
+            assert transaction_id == id_generator_stub.last_id
 
-    def test_new_id_generated_if_id_is_only_white_space(self, id_provider_stub):
+    def test_new_id_generated_if_id_is_only_white_space(self, id_generator_stub):
         parameters = {"transaction_id": "\t\n \r\n", "other": "config"}
         with transaction("name", parameters) as transaction_id:
-            assert transaction_id == id_provider_stub.last_id
+            assert transaction_id == id_generator_stub.last_id
 
-    def test_new_id_generated_if_id_is_not_string_type(self, id_provider_stub):
+    def test_new_id_generated_if_id_is_not_string_type(self, id_generator_stub):
         parameters = {"transaction_id": 1234.5, "other": "config"}
         with transaction("name", parameters) as transaction_id:
-            assert transaction_id == id_provider_stub.last_id
+            assert transaction_id == id_generator_stub.last_id
 
-    def test_new_id_generated_if_id_is_not_present(self, id_provider_stub):
+    def test_new_id_generated_if_id_is_not_present(self, id_generator_stub):
         parameters = {"other": "config"}
         with transaction("name", parameters) as transaction_id:
-            assert transaction_id == id_provider_stub.last_id
+            assert transaction_id == id_generator_stub.last_id
 
-    def test_new_id_generated_if_params_empty(self, id_provider_stub):
+    def test_new_id_generated_if_params_empty(self, id_generator_stub):
         parameters = {}
         with transaction("name", parameters) as transaction_id:
-            assert transaction_id == id_provider_stub.last_id
+            assert transaction_id == id_generator_stub.last_id
 
-    def test_id_provider_only_used_once_for_one_new_id(self, id_provider_stub):
+    def test_id_provider_only_used_once_for_one_new_id(self, id_generator_stub):
         parameters = {}
         with transaction("name", parameters):
-            assert id_provider_stub.call_count == 1
+            assert id_generator_stub.call_count == 1
 
-    def test_id_provider_not_used_for_existing_valid_id(self, id_provider_stub):
+    def test_id_provider_not_used_for_existing_valid_id(self, id_generator_stub):
         parameters = {"transaction_id": "abc1234"}
         with transaction("name", parameters):
-            assert id_provider_stub.call_count == 0
+            assert id_generator_stub.call_count == 0
 
     def test_error_if_params_type_is_not_mapping(self):
         parameters = []
@@ -68,7 +72,7 @@ class TestTransactionIdGeneration:
 class TestTransactionLogging:
     """Tests for :class:`~ska.logging.transaction` related to logging."""
 
-    def test_name_and_id_and_params_json_logged_on_entry(self, id_provider_stub, recording_logger):
+    def test_name_and_id_and_params_json_logged_on_entry(self, id_generator_stub, recording_logger):
         params = {"other": ["config", 1, 2, 3.0]}
         with transaction("name", params) as transaction_id:
             pass
@@ -78,5 +82,47 @@ class TestTransactionLogging:
         assert json.dumps(params) in log_message
 
 
-class TestIdProviderService:
-    """Tests for :class:`~ska.logging.transactions.IdProviderService`."""
+class TestTransactionIdGenerator:
+    """Tests for :class:`~ska.logging.transactions.TransactionIdGenerator`."""
+
+    def test_local_id_generator_increments_on_next(self):
+        assert not os.environ.get("SKUID_URL")
+        generator = TransactionIdGenerator()
+
+        assert generator.next()
+        assert generator.next() != generator.next()
+
+    def test_remote_id_generator_increments_on_next(self, monkeypatch):
+        monkeypatch.setenv("SKUID_URL", "endpoint/to/skuid-client")
+
+        with patch("skuid.client.requests.get") as mocked_req:
+            response = MagicMock()
+            response.json.side_effect = [
+                json.dumps({"transaction_id": 1}),
+                json.dumps({"transaction_id": 2}),
+                json.dumps({"transaction_id": 3}),
+            ]
+            mocked_req.return_value = response
+            generator = TransactionIdGenerator()
+
+            assert generator.next() == 1
+            assert generator.next() == 2
+            assert generator.next() == 3
+
+    def test_remote_id_generation_is_thread_safe(self):
+
+        def get_100_ids():
+            for j in range(100):
+                self.test_remote_id_generator_increments_on_next()
+
+        with concurrent.futures.ThreadPoolExecutor(max_workers=10) as executor:
+            executor.map(get_100_ids, range(10))
+
+    def test_local_id_generation_is_thread_safe(self):
+
+        def get_100_ids():
+            for j in range(100):
+                self.test_local_id_generator_increments_on_next()
+
+        with concurrent.futures.ThreadPoolExecutor(max_workers=10) as executor:
+            executor.map(get_100_ids, range(10))