From c18d3061a7965f8e428c12610740d2faea0d890c Mon Sep 17 00:00:00 2001 From: samueltwum1 <samueltwum1@gmail.com> Date: Tue, 15 Sep 2020 07:54:34 +0200 Subject: [PATCH] SAR-149 Add implementation details for TransactionIdGenerator IdProviderService has been replaced with TransactionIdGenerator --- src/ska/logging/transactions.py | 32 ++++++++---- tests/conftest.py | 19 +++---- tests/test_configuration.py | 2 - tests/test_transactions.py | 88 +++++++++++++++++++++++++-------- 4 files changed, 100 insertions(+), 41 deletions(-) diff --git a/src/ska/logging/transactions.py b/src/ska/logging/transactions.py index 3eee962..693171f 100644 --- a/src/ska/logging/transactions.py +++ b/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): diff --git a/tests/conftest.py b/tests/conftest.py index 4c9967f..27a0993 100644 --- a/tests/conftest.py +++ b/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 diff --git a/tests/test_configuration.py b/tests/test_configuration.py index 98a2533..df83b84 100644 --- a/tests/test_configuration.py +++ b/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]}} - - diff --git a/tests/test_transactions.py b/tests/test_transactions.py index 05a31a7..a4032ef 100644 --- a/tests/test_transactions.py +++ b/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)) -- GitLab