From df2d094dc65e042b75ceb2a0a999136a7eae1720 Mon Sep 17 00:00:00 2001 From: mtakaki Date: Tue, 28 Jan 2020 01:42:49 -0800 Subject: [PATCH] Fixing push status that has been broken since moving to a client (#80) * Fixing push status that has been broken since moving to a client * Adding unit test to cover the bug --- cachet_url_monitor/client.py | 11 +++-- cachet_url_monitor/configuration.py | 53 ++++++++++++--------- cachet_url_monitor/latency_unit.py | 5 +- cachet_url_monitor/scheduler.py | 15 +++--- tests/test_client.py | 8 ++++ tests/test_configuration.py | 73 ++++++++++++++++------------- 6 files changed, 99 insertions(+), 66 deletions(-) diff --git a/cachet_url_monitor/client.py b/cachet_url_monitor/client.py index d4a0849..c27ade6 100644 --- a/cachet_url_monitor/client.py +++ b/cachet_url_monitor/client.py @@ -1,5 +1,6 @@ #!/usr/bin/env python from typing import Dict +from typing import Optional import click import requests @@ -81,7 +82,7 @@ class CachetClient(object): else: raise exceptions.MetricNonexistentError(metric_id) - def get_component_status(self, component_id): + def get_component_status(self, component_id: int) -> Optional[status.ComponentStatus]: """Retrieves the current status of the given component. It will fail if the component does not exist or doesn't respond with the expected data. :return component status. @@ -94,13 +95,13 @@ class CachetClient(object): else: raise exceptions.ComponentNonexistentError(component_id) - def push_status(self, component_id, component_status): + def push_status(self, component_id: int, component_status: status.ComponentStatus): """Pushes the status of the component to the cachet server. """ - params = {'id': component_id, 'status': component_status} + params = {'id': component_id, 'status': component_status.value} return requests.put(f"{self.url}/components/{component_id}", params=params, headers=self.headers) - def push_metrics(self, metric_id, latency_time_unit, elapsed_time_in_seconds, timestamp): + def push_metrics(self, metric_id: int, latency_time_unit: str, elapsed_time_in_seconds: int, timestamp: int): """Pushes the total amount of seconds the request took to get a response from the URL. """ value = latency_unit.convert_to_unit(latency_time_unit, elapsed_time_in_seconds) @@ -122,7 +123,7 @@ class CachetClient(object): # This is the first time the incident is being created. params = {'name': 'URL unavailable', 'message': message, 'status': status.IncidentStatus.INVESTIGATING.value, - 'visible': is_public_incident, 'component_id': component_id, 'component_status': status_value, + 'visible': is_public_incident, 'component_id': component_id, 'component_status': status_value.value, 'notify': True} return requests.post(f'{self.url}/incidents', params=params, headers=self.headers) diff --git a/cachet_url_monitor/configuration.py b/cachet_url_monitor/configuration.py index 307f3bf..7a94ea7 100644 --- a/cachet_url_monitor/configuration.py +++ b/cachet_url_monitor/configuration.py @@ -2,16 +2,15 @@ import abc import copy import logging -import os import re import time +from typing import Dict import requests from yaml import dump import cachet_url_monitor.status as st from cachet_url_monitor.client import CachetClient, normalize_url -from cachet_url_monitor.exceptions import MetricNonexistentError from cachet_url_monitor.status import ComponentStatus # This is the mandatory fields that must be in the configuration file in this @@ -33,13 +32,37 @@ class Configuration(object): """Represents a configuration file, but it also includes the functionality of assessing the API and pushing the results to cachet. """ + endpoint_index: int + endpoint: str + client: CachetClient + token: str + current_fails: int + trigger_update: bool + headers: Dict[str, str] - def __init__(self, config, endpoint_index: int): - self.endpoint_index: int = endpoint_index + endpoint_method: str + endpoint_url: str + endpoint_timeout: int + endpoint_header: Dict[str, str] + + allowed_fails: int + component_id: int + metric_id: int + default_metric_value: int + latency_unit: str + + status: ComponentStatus + previous_status: ComponentStatus + + def __init__(self, config, endpoint_index: int, client: CachetClient, token: str): + self.endpoint_index = endpoint_index self.data = config self.endpoint = self.data['endpoints'][endpoint_index] - self.current_fails: int = 0 - self.trigger_update: bool = True + self.client = client + self.token = token + + self.current_fails = 0 + self.trigger_update = True if 'name' not in self.endpoint: # We have to make this mandatory, otherwise the logs are confusing when there are multiple URLs. @@ -54,7 +77,7 @@ class Configuration(object): self.validate() # We store the main information from the configuration file, so we don't keep reading from the data dictionary. - self.token = os.environ.get('CACHET_TOKEN') or self.data['cachet']['token'] + self.headers = {'X-Cachet-Token': self.token} self.endpoint_method = self.endpoint['method'] @@ -63,14 +86,11 @@ class Configuration(object): self.endpoint_header = self.endpoint.get('header') or None self.allowed_fails = self.endpoint.get('allowed_fails') or 0 - self.api_url = os.environ.get('CACHET_API_URL') or self.data['cachet']['api_url'] self.component_id = self.endpoint['component_id'] self.metric_id = self.endpoint.get('metric_id') - self.client = CachetClient(self.api_url, self.token) - if self.metric_id is not None: - self.default_metric_value = self.get_default_metric_value(self.metric_id) + self.default_metric_value = self.client.get_default_metric_value(self.metric_id) # The latency_unit configuration is not mandatory and we fallback to seconds, by default. self.latency_unit = self.data['cachet'].get('latency_unit') or 's' @@ -88,15 +108,6 @@ class Configuration(object): for expectation in self.expectations: self.logger.info('Registered expectation: %s' % (expectation,)) - def get_default_metric_value(self, metric_id): - """Returns default value for configured metric.""" - get_metric_request = requests.get('%s/metrics/%s' % (self.api_url, metric_id), headers=self.headers) - - if get_metric_request.ok: - return get_metric_request.json()['data']['default_value'] - else: - raise MetricNonexistentError(metric_id) - def get_action(self): """Retrieves the action list from the configuration. If it's empty, returns an empty list. :return: The list of actions, which can be an empty list. @@ -162,7 +173,7 @@ class Configuration(object): status: ComponentStatus = expectation.get_status(self.request) # The greater the status is, the worse the state of the API is. - if status.value > self.status.value: + if status.value >= self.status.value: self.status = status self.message = expectation.get_message(self.request) self.logger.info(self.message) diff --git a/cachet_url_monitor/latency_unit.py b/cachet_url_monitor/latency_unit.py index c5708ba..de24fc3 100644 --- a/cachet_url_monitor/latency_unit.py +++ b/cachet_url_monitor/latency_unit.py @@ -1,10 +1,11 @@ #!/usr/bin/env python +from typing import Dict -seconds_per_unit = {"ms": 1000, "milliseconds": 1000, "s": 1, "seconds": 1, "m": float(1) / 60, +seconds_per_unit: Dict[str, float] = {"ms": 1000, "milliseconds": 1000, "s": 1, "seconds": 1, "m": float(1) / 60, "minutes": float(1) / 60, "h": float(1) / 3600, "hours": float(1) / 3600} -def convert_to_unit(time_unit, value): +def convert_to_unit(time_unit: str, value: float): """ Will convert the given value from seconds to the given time_unit. diff --git a/cachet_url_monitor/scheduler.py b/cachet_url_monitor/scheduler.py index 8171922..bedcaa2 100644 --- a/cachet_url_monitor/scheduler.py +++ b/cachet_url_monitor/scheduler.py @@ -7,6 +7,7 @@ import time import schedule from yaml import load, SafeLoader +from cachet_url_monitor.client import CachetClient from cachet_url_monitor.configuration import Configuration cachet_mandatory_fields = ['api_url', 'token'] @@ -105,14 +106,14 @@ def build_agent(configuration, logger): def validate_config(): - if 'endpoints' not in config_file.keys(): + if 'endpoints' not in config_data.keys(): fatal_error('Endpoints is a mandatory field') - if config_file['endpoints'] is None: + if config_data['endpoints'] is None: fatal_error('Endpoints array can not be empty') for key in cachet_mandatory_fields: - if key not in config_file['cachet']: + if key not in config_data['cachet']: fatal_error('Missing cachet mandatory fields') @@ -132,14 +133,16 @@ if __name__ == "__main__": sys.exit(1) try: - config_file = load(open(sys.argv[1], 'r'), SafeLoader) + config_data = load(open(sys.argv[1], 'r'), SafeLoader) except FileNotFoundError: logging.getLogger('cachet_url_monitor.scheduler').fatal(f'File not found: {sys.argv[1]}') sys.exit(1) validate_config() - for endpoint_index in range(len(config_file['endpoints'])): - configuration = Configuration(config_file, endpoint_index) + for endpoint_index in range(len(config_data['endpoints'])): + token = os.environ.get('CACHET_TOKEN') or config_data['cachet']['token'] + api_url = os.environ.get('CACHET_API_URL') or config_data['cachet']['api_url'] + configuration = Configuration(config_data, endpoint_index, CachetClient(api_url, token), token) NewThread(Scheduler(configuration, build_agent(configuration, logging.getLogger('cachet_url_monitor.scheduler')))).start() diff --git a/tests/test_client.py b/tests/test_client.py index 4611c7b..3736aa2 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -145,3 +145,11 @@ class ClientTest(unittest.TestCase): self.assertEqual(status, ComponentStatus.OPERATIONAL, 'Getting component status value is incorrect.') + + @requests_mock.mock() + def test_push_status(self, m): + m.put(f'{CACHET_URL}/components/123?id=123&status={ComponentStatus.PARTIAL_OUTAGE.value}', + headers={'X-Cachet-Token': TOKEN}) + response = self.client.push_status(123, ComponentStatus.PARTIAL_OUTAGE) + + self.assertTrue(response.ok, 'Pushing status value is failed.') diff --git a/tests/test_configuration.py b/tests/test_configuration.py index 2888fa8..c4ca09a 100644 --- a/tests/test_configuration.py +++ b/tests/test_configuration.py @@ -9,6 +9,8 @@ import requests_mock from yaml import load, SafeLoader import cachet_url_monitor.status +from cachet_url_monitor.client import CachetClient +import cachet_url_monitor.exceptions sys.modules['logging'] = mock.Mock() from cachet_url_monitor.configuration import Configuration @@ -16,35 +18,26 @@ import os class ConfigurationTest(unittest.TestCase): - @mock.patch.dict(os.environ, {'CACHET_TOKEN': 'token2'}) + client: CachetClient + configuration: Configuration + def setUp(self): def getLogger(name): self.mock_logger = mock.Mock() return self.mock_logger sys.modules['logging'].getLogger = getLogger - - # def get(url, headers): - # get_return = mock.Mock() - # get_return.ok = True - # get_return.json = mock.Mock() - # get_return.json.return_value = {'data': {'status': 1, 'default_value': 0.5}} - # return get_return - # - # sys.modules['requests'].get = get - + self.client = mock.Mock() + # We set the initial status to OPERATIONAL. + self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL self.configuration = Configuration( - load(open(os.path.join(os.path.dirname(__file__), 'configs/config.yml'), 'rt'), SafeLoader), 0) - # sys.modules['requests'].Timeout = Timeout - # sys.modules['requests'].ConnectionError = ConnectionError - # sys.modules['requests'].HTTPError = HTTPError + load(open(os.path.join(os.path.dirname(__file__), 'configs/config.yml'), 'rt'), SafeLoader), 0, self.client, + 'token2') def test_init(self): self.assertEqual(len(self.configuration.data), 2, 'Number of root elements in config.yml is incorrect') self.assertEqual(len(self.configuration.expectations), 3, 'Number of expectations read from file is incorrect') self.assertDictEqual(self.configuration.headers, {'X-Cachet-Token': 'token2'}, 'Header was not set correctly') - self.assertEqual(self.configuration.api_url, 'https://demo.cachethq.io/api/v1', - 'Cachet API URL was set incorrectly') self.assertDictEqual(self.configuration.endpoint_header, {'SOME-HEADER': 'SOME-VALUE'}, 'Header is incorrect') @requests_mock.mock() @@ -98,31 +91,49 @@ class ConfigurationTest(unittest.TestCase): 'Component status set incorrectly') self.mock_logger.exception.assert_called_with('Unexpected HTTP response') - @requests_mock.mock() - def test_push_status(self, m): - m.put('https://demo.cachethq.io/api/v1/components/1?id=1&status=1', headers={'X-Cachet-Token': 'token2'}) - self.assertEqual(self.configuration.status, cachet_url_monitor.status.ComponentStatus.OPERATIONAL, - 'Incorrect component update parameters') + def test_push_status(self): + self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL + push_status_response = mock.Mock() + self.client.push_status.return_value = push_status_response + push_status_response.ok = True + self.configuration.status = cachet_url_monitor.status.ComponentStatus.PARTIAL_OUTAGE + self.configuration.push_status() - @requests_mock.mock() - def test_push_status_with_failure(self, m): - m.put('https://demo.cachethq.io/api/v1/components/1?id=1&status=1', headers={'X-Cachet-Token': 'token2'}, - status_code=400) - self.assertEqual(self.configuration.status, cachet_url_monitor.status.ComponentStatus.OPERATIONAL, - 'Incorrect component update parameters') + self.client.push_status.assert_called_once_with(1, cachet_url_monitor.status.ComponentStatus.OPERATIONAL) + + def test_push_status_with_failure(self): + self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL + push_status_response = mock.Mock() + self.client.push_status.return_value = push_status_response + push_status_response.ok = False + self.configuration.status = cachet_url_monitor.status.ComponentStatus.PARTIAL_OUTAGE + self.configuration.push_status() + self.client.push_status.assert_called_once_with(1, cachet_url_monitor.status.ComponentStatus.OPERATIONAL) + + def test_push_status_same_status(self): + self.client.get_component_status.return_value = cachet_url_monitor.status.ComponentStatus.OPERATIONAL + self.configuration.status = cachet_url_monitor.status.ComponentStatus.OPERATIONAL + + self.configuration.push_status() + + self.client.push_status.assert_not_called() + class ConfigurationMultipleUrlTest(unittest.TestCase): @mock.patch.dict(os.environ, {'CACHET_TOKEN': 'token2'}) def setUp(self): config_yaml = load(open(os.path.join(os.path.dirname(__file__), 'configs/config_multiple_urls.yml'), 'rt'), SafeLoader) + self.client = [] self.configuration = [] for index in range(len(config_yaml['endpoints'])): - self.configuration.append(Configuration(config_yaml, index)) + client = mock.Mock() + self.client.append(client) + self.configuration.append(Configuration(config_yaml, index, client, 'token2')) def test_init(self): expected_method = ['GET', 'POST'] @@ -133,8 +144,6 @@ class ConfigurationMultipleUrlTest(unittest.TestCase): self.assertEqual(len(config.data), 2, 'Number of root elements in config.yml is incorrect') self.assertEqual(len(config.expectations), 1, 'Number of expectations read from file is incorrect') self.assertDictEqual(config.headers, {'X-Cachet-Token': 'token2'}, 'Header was not set correctly') - self.assertEqual(config.api_url, 'https://demo.cachethq.io/api/v1', - 'Cachet API URL was set incorrectly') self.assertEqual(expected_method[index], config.endpoint_method) self.assertEqual(expected_url[index], config.endpoint_url) @@ -146,4 +155,4 @@ class ConfigurationNegativeTest(unittest.TestCase): with pytest.raises(cachet_url_monitor.configuration.ConfigurationValidationError): self.configuration = Configuration( load(open(os.path.join(os.path.dirname(__file__), 'configs/config_invalid_type.yml'), 'rt'), - SafeLoader), 0) + SafeLoader), 0, mock.Mock(), 'token2')