From 252d835575a5016a08f7e53f0401d4e05c94651e Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Fri, 18 Jun 2021 13:33:35 -0700 Subject: [PATCH 1/9] Adding support for metric publishing, manually tested and tests pending. --- samtranslator/metrics/__init__.py | 0 samtranslator/metrics/metrics.py | 140 +++++++++++++++++++++++++ samtranslator/translator/translator.py | 7 +- 3 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 samtranslator/metrics/__init__.py create mode 100644 samtranslator/metrics/metrics.py diff --git a/samtranslator/metrics/__init__.py b/samtranslator/metrics/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/samtranslator/metrics/metrics.py b/samtranslator/metrics/metrics.py new file mode 100644 index 0000000000..2c17072b6c --- /dev/null +++ b/samtranslator/metrics/metrics.py @@ -0,0 +1,140 @@ +from botocore.exceptions import ClientError +from collections import namedtuple +import logging + +LOG = logging.getLogger(__name__) + + +class MetricsPublisher: + """Interface for all MetricPublishers""" + + def publish(self, namespace, metrics): + raise NotImplementedError + + +class CWMetricsPublisher(MetricsPublisher): + def __init__(self, cloudwatch_client): + """ + Constructor + + :param cloudwatch_client: cloudwatch client required to publish metrics to cloudwatch + """ + + self.cloudwatch_client = cloudwatch_client + + def publish(self, namespace, metrics): + """ + Method to publish all metrics to Cloudwatch. + + :param namespace: namespace applied to all metrics published. + :param metrics: list of metrics to be published + """ + batch = [] + for metric in metrics: + batch.append(metric) + # Cloudwatch recommends not to send more than 20 metrics at a time + if len(batch) == 20: + self._flush_metrics(namespace, batch) + batch = [] + self._flush_metrics(namespace, batch) + + def _flush_metrics(self, namespace, metrics): + """ + Internal method to publish all provided metrics to cloudwatch, please make sure that array size of metrics is <= 20.""" + metric_data = list(map(lambda m: m.get_metric_data(), metrics)) + try: + self.cloudwatch_client.put_metric_data(Namespace=namespace, MetricData=metric_data) + except ClientError: + LOG.exception("Failed to report {} metrics".format(len(metric_data)), metrics) + + +class DummyMetricsPublisher(MetricsPublisher): + def publish(self, namespace, metrics): + """Method to actually publish them metric""" + + +class Unit: + Seconds = "Seconds" + Microseconds = "Microseconds" + Milliseconds = "Milliseconds" + Bytes = "Bytes" + Kilobytes = "Kilobytes" + Megabytes = "Megabytes" + Bits = "Bits" + Kilobits = "Kilobits" + Megabits = "Megabits" + Percent = "Percent" + Count = "Count" + + +class MetricDatum: + """ + Class to hold Metric data. + """ + + def __init__(self, name, value, unit, dimensions=None): + """ + Constructor + + :param name: metric name + :param value: value of metric + :param unit: unit of metric (try using values from Unit class) + :param dimensions: array of dimensions applied to the metric + """ + self.name = name + self.value = value + self.unit = unit + self.dimensions = dimensions if dimensions else [] + + def get_metric_data(self): + return {"MetricName": self.name, "Value": self.value, "Unit": self.unit, "Dimensions": self.dimensions} + + +class Metrics: + def __init__(self, namespace="ServerlessTransform", metrics_publisher=None): + """ + Constructor + + :param namespace: namespace under which all metrics will be published + :param metrics_publisher: publisher to publish all metrics + """ + self.metrics_publisher = metrics_publisher if metrics_publisher else DummyMetricsPublisher() + self.metrics_cache = [] + self.namespace = namespace + + def _record_metric(self, name, value, unit, dimensions=[]): + """ + Create and save metric objects to an array. + + :param name: metric name + :param value: value of metric + :param unit: unit of metric (try using values from Unit class) + :param dimensions: array of dimensions applied to the metric + """ + self.metrics_cache.append(MetricDatum(name, value, unit, dimensions)) + + def record_count(self, name, value, unit=Unit.Count, dimensions=[]): + """ + Create metric with unit Count. + + :param name: metric name + :param value: value of metric + :param unit: unit of metric (try using values from Unit class) + :param dimensions: array of dimensions applied to the metric + """ + self._record_metric(name, value, unit, dimensions) + + def record_latency(self, name, value, unit=Unit.Milliseconds, dimensions=[]): + """ + Create metric with unit Milliseconds. + + :param name: metric name + :param value: value of metric + :param unit: unit of metric (try using values from Unit class) + :param dimensions: array of dimensions applied to the metric + """ + self._record_metric(name, value, unit, dimensions) + + def publish(self): + """Method to publish all metrics, do not forget to call this method.""" + self.metrics_publisher.publish(self.namespace, self.metrics_cache) diff --git a/samtranslator/translator/translator.py b/samtranslator/translator/translator.py index a1c903b23c..a34aea113b 100644 --- a/samtranslator/translator/translator.py +++ b/samtranslator/translator/translator.py @@ -1,4 +1,6 @@ import copy +import boto3 +from samtranslator.metrics.metrics import CWMetricsPublisher, DummyMetricsPublisher, Metrics from samtranslator.feature_toggle.feature_toggle import ( FeatureToggle, @@ -32,7 +34,7 @@ class Translator: """Translates SAM templates into CloudFormation templates""" - def __init__(self, managed_policy_map, sam_parser, plugins=None, boto_session=None): + def __init__(self, managed_policy_map, sam_parser, plugins=None, boto_session=None, metrics=None): """ :param dict managed_policy_map: Map of managed policy names to the ARNs :param sam_parser: Instance of a SAM Parser @@ -44,6 +46,7 @@ def __init__(self, managed_policy_map, sam_parser, plugins=None, boto_session=No self.sam_parser = sam_parser self.feature_toggle = None self.boto_session = boto_session + self.metrics = metrics if metrics else Metrics("ServerlessTransform", DummyMetricsPublisher()) if self.boto_session: ArnGenerator.BOTO_SESSION_REGION_NAME = self.boto_session.region_name @@ -78,7 +81,7 @@ def _get_function_names(self, resource_dict, intrinsics_resolver): ) return self.function_names - def translate(self, sam_template, parameter_values, feature_toggle=None): + def translate(self, sam_template, parameter_values, feature_toggle=None, **kwargs): """Loads the SAM resources from the given SAM manifest, replaces them with their corresponding CloudFormation resources, and returns the resulting CloudFormation template. From a6891b18fdbe1e182f4ec8d1164e701530af0bc8 Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Sun, 20 Jun 2021 11:08:21 -0700 Subject: [PATCH 2/9] Adding make action for running test with html coverage. --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 09dce252d5..9675b8ccb0 100755 --- a/Makefile +++ b/Makefile @@ -8,6 +8,9 @@ init: test: pytest --cov samtranslator --cov-report term-missing --cov-fail-under 95 tests/* +test-cov-report: + pytest --cov samtranslator --cov-report term-missing --cov-report html --cov-fail-under 95 tests/* + integ-test: pytest --no-cov integration/* From acd322f1a3146211272e3a1b351f487214d8a540 Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Sun, 20 Jun 2021 11:09:12 -0700 Subject: [PATCH 3/9] Adding unit tests for metrics. --- samtranslator/metrics/metrics.py | 28 ++++- samtranslator/translator/translator.py | 6 +- tests/metrics/__init__.py | 0 tests/metrics/test_metrics.py | 149 +++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 10 deletions(-) create mode 100644 tests/metrics/__init__.py create mode 100644 tests/metrics/test_metrics.py diff --git a/samtranslator/metrics/metrics.py b/samtranslator/metrics/metrics.py index 2c17072b6c..b70277bbfd 100644 --- a/samtranslator/metrics/metrics.py +++ b/samtranslator/metrics/metrics.py @@ -1,5 +1,4 @@ from botocore.exceptions import ClientError -from collections import namedtuple import logging LOG = logging.getLogger(__name__) @@ -8,18 +7,23 @@ class MetricsPublisher: """Interface for all MetricPublishers""" + def __init__(self): + pass + def publish(self, namespace, metrics): raise NotImplementedError class CWMetricsPublisher(MetricsPublisher): + BATCH_SIZE = 20 + def __init__(self, cloudwatch_client): """ Constructor :param cloudwatch_client: cloudwatch client required to publish metrics to cloudwatch """ - + MetricsPublisher.__init__(self) self.cloudwatch_client = cloudwatch_client def publish(self, namespace, metrics): @@ -33,24 +37,29 @@ def publish(self, namespace, metrics): for metric in metrics: batch.append(metric) # Cloudwatch recommends not to send more than 20 metrics at a time - if len(batch) == 20: + if len(batch) == self.BATCH_SIZE: self._flush_metrics(namespace, batch) batch = [] self._flush_metrics(namespace, batch) def _flush_metrics(self, namespace, metrics): """ - Internal method to publish all provided metrics to cloudwatch, please make sure that array size of metrics is <= 20.""" + Internal method to publish all provided metrics to cloudwatch, please make sure that array size of metrics is <= 20. + """ metric_data = list(map(lambda m: m.get_metric_data(), metrics)) try: self.cloudwatch_client.put_metric_data(Namespace=namespace, MetricData=metric_data) - except ClientError: - LOG.exception("Failed to report {} metrics".format(len(metric_data)), metrics) + except Exception as e: + LOG.exception("Failed to report {} metrics".format(len(metric_data)), metrics, e) class DummyMetricsPublisher(MetricsPublisher): + def __init__(self): + MetricsPublisher.__init__(self) + def publish(self, namespace, metrics): """Method to actually publish them metric""" + LOG.info("Dummy publisher ignoring {} metrices".format(len(metrics))) class Unit: @@ -102,6 +111,12 @@ def __init__(self, namespace="ServerlessTransform", metrics_publisher=None): self.metrics_cache = [] self.namespace = namespace + def __del__(self): + if len(self.metrics_cache) > 0: + # attempting to publish if user forgot to call publish in code + LOG.warn("There are unpublished metrics. Please make sure you call publish after you record all metrics.") + self.publish() + def _record_metric(self, name, value, unit, dimensions=[]): """ Create and save metric objects to an array. @@ -138,3 +153,4 @@ def record_latency(self, name, value, unit=Unit.Milliseconds, dimensions=[]): def publish(self): """Method to publish all metrics, do not forget to call this method.""" self.metrics_publisher.publish(self.namespace, self.metrics_cache) + self.metrics_cache = [] diff --git a/samtranslator/translator/translator.py b/samtranslator/translator/translator.py index a34aea113b..22ea67006d 100644 --- a/samtranslator/translator/translator.py +++ b/samtranslator/translator/translator.py @@ -1,10 +1,8 @@ import copy -import boto3 -from samtranslator.metrics.metrics import CWMetricsPublisher, DummyMetricsPublisher, Metrics +from samtranslator.metrics.metrics import DummyMetricsPublisher, Metrics from samtranslator.feature_toggle.feature_toggle import ( FeatureToggle, - FeatureToggleLocalConfigProvider, FeatureToggleDefaultConfigProvider, ) from samtranslator.model import ResourceTypeResolver, sam_resources @@ -81,7 +79,7 @@ def _get_function_names(self, resource_dict, intrinsics_resolver): ) return self.function_names - def translate(self, sam_template, parameter_values, feature_toggle=None, **kwargs): + def translate(self, sam_template, parameter_values, feature_toggle=None): """Loads the SAM resources from the given SAM manifest, replaces them with their corresponding CloudFormation resources, and returns the resulting CloudFormation template. diff --git a/tests/metrics/__init__.py b/tests/metrics/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/metrics/test_metrics.py b/tests/metrics/test_metrics.py new file mode 100644 index 0000000000..b62ca280cd --- /dev/null +++ b/tests/metrics/test_metrics.py @@ -0,0 +1,149 @@ +from mock import patch, Mock +from botocore.exceptions import ClientError +from parameterized import parameterized, param +from unittest import TestCase +from mock import MagicMock, patch +from samtranslator.metrics.metrics import ( + Metrics, + MetricsPublisher, + CWMetricsPublisher, + DummyMetricsPublisher, + Unit, + MetricDatum +) + + +class MetricPublisherTestHelper(MetricsPublisher): + def __init__(self): + MetricsPublisher.__init__(self) + self.metrics_cache = [] + self.namespace = "" + + def publish(self, namespace, metrics): + self.namespace = namespace + self.metrics_cache = metrics + + +class TestMetrics(TestCase): + @parameterized.expand( + [ + param("DummyNamespace", "CountMetric", 12, Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param("DummyNamespace", "IAMError", 59, Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + ] + ) + def test_publishing_count_metric(self, namespace, name, value, unit, dimensions): + mock_metrics_publisher = MetricPublisherTestHelper() + metrics = Metrics(namespace, mock_metrics_publisher) + metrics.record_count(name, value, unit, dimensions) + metrics.publish() + self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) + published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() + self.assertEqual(published_metric['MetricName'], name) + self.assertEqual(published_metric['Dimensions'], dimensions) + self.assertEqual(published_metric['Value'], value) + + @parameterized.expand( + [ + param("DummyNamespace", "SARLatency", 1200, Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param("DummyNamespace", "IAMLatency", 400, Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + ] + ) + def test_publishing_latency_metric(self, namespace, name, value, unit, dimensions): + mock_metrics_publisher = MetricPublisherTestHelper() + metrics = Metrics(namespace, mock_metrics_publisher) + metrics.record_latency(name, value, unit, dimensions) + metrics.publish() + self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) + published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() + self.assertEqual(published_metric['MetricName'], name) + self.assertEqual(published_metric['Dimensions'], dimensions) + self.assertEqual(published_metric['Value'], value) + + @parameterized.expand( + [ + param("DummyNamespace", "CountMetric", 12, Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param("DummyNamespace", "LatencyMetric", 1200, Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + ] + ) + def test_publishing_metric_without_calling_publish(self, namespace, name, value, unit, dimensions): + mock_metrics_publisher = MetricPublisherTestHelper() + metrics = Metrics(namespace, mock_metrics_publisher) + metrics.record_count(name, value, unit, dimensions) + del metrics + self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) + published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() + self.assertEqual(published_metric['MetricName'], name) + self.assertEqual(published_metric['Dimensions'], dimensions) + self.assertEqual(published_metric['Value'], value) + + +class TestCWMetricPublisher(TestCase): + @parameterized.expand( + [ + param("DummyNamespace", "CountMetric", 12, Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param("DummyNamespace", "IAMError", 59, Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param("DummyNamespace", "SARLatency", 1200, Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param("DummyNamespace", "IAMLatency", 400, Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + ] + ) + def test_publish_metric(self, namespace, name, value, unit, dimensions): + mock_cw_client = MagicMock() + metric_publisher = CWMetricsPublisher(mock_cw_client) + metric_datum = MetricDatum(name, value, unit, dimensions) + metrics = [metric_datum] + metric_publisher.publish(namespace, metrics) + call_kwargs = mock_cw_client.put_metric_data.call_args.kwargs + published_metric_data = call_kwargs['MetricData'][0] + self.assertEqual(call_kwargs['Namespace'], namespace) + self.assertEqual(published_metric_data['MetricName'], name) + self.assertEqual(published_metric_data['Unit'], unit) + self.assertEqual(published_metric_data['Value'], value) + self.assertEqual(published_metric_data['Dimensions'], dimensions) + + @parameterized.expand( + [ + param("DummyNamespace", "CountMetric", 12, Unit.Count, []), + ] + ) + def test_publish_more_than_20_metrics(self, namespace, name, value, unit, dimensions): + mock_cw_client = MagicMock() + metric_publisher = CWMetricsPublisher(mock_cw_client) + single_metric = MetricDatum(name, value, unit, dimensions) + metrics_list = [] + total_metrics = 45 + for i in range(total_metrics): + metrics_list.append(single_metric) + metric_publisher.publish(namespace, metrics_list) + call_args_list = mock_cw_client.put_metric_data.call_args_list + + self.assertEqual(mock_cw_client.put_metric_data.call_count, 3) + # Validating that metrics are published in batches of 20 + self.assertEqual(len(call_args_list[0].kwargs['MetricData']), min(total_metrics, metric_publisher.BATCH_SIZE)) + total_metrics -= metric_publisher.BATCH_SIZE + self.assertEqual(len(call_args_list[1].kwargs['MetricData']), min(total_metrics, metric_publisher.BATCH_SIZE)) + total_metrics -= metric_publisher.BATCH_SIZE + self.assertEqual(len(call_args_list[2].kwargs['MetricData']), min(total_metrics, metric_publisher.BATCH_SIZE)) + + def test_do_not_fail_on_cloudwatch_any_exception(self): + mock_cw_client = MagicMock() + mock_cw_client.put_metric_data = MagicMock() + mock_cw_client.put_metric_data.side_effect = Exception("BOOM!", "FAILED!!") + metric_publisher = CWMetricsPublisher(mock_cw_client) + single_metric = MetricDatum('Name', 20, Unit.Count, []) + metric_publisher.publish('SomeNamespace', [single_metric]) + self.assertTrue(True) + + def test_for_code_coverage(self): + dummy_publisher = DummyMetricsPublisher() + dummy_publisher.publish('NS', [None]) + self.assertTrue(True) From 51f8c394d558d1031eb8e8e7e53f402225696f4b Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Mon, 21 Jun 2021 10:04:27 -0700 Subject: [PATCH 4/9] Writing integration tests for metrics publishing. --- integration/metrics/__init__.py | 0 .../metrics/test_metrics_integration.py | 73 +++++++++++++++++++ samtranslator/metrics/metrics.py | 1 - tests/metrics/test_metrics.py | 4 +- 4 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 integration/metrics/__init__.py create mode 100644 integration/metrics/test_metrics_integration.py diff --git a/integration/metrics/__init__.py b/integration/metrics/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration/metrics/test_metrics_integration.py b/integration/metrics/test_metrics_integration.py new file mode 100644 index 0000000000..70790ade5f --- /dev/null +++ b/integration/metrics/test_metrics_integration.py @@ -0,0 +1,73 @@ +import boto3 +import time +import uuid +from datetime import datetime, timedelta +from unittest import TestCase +from samtranslator.metrics.metrics import ( + Metrics, + CWMetricsPublisher, +) + + +class MetricsIntegrationTest(TestCase): + """ + This class will use a unique metric namespace to create metrics. There is no cleanup done here + because if a particular namespace is unsed for 2 weeks it'll be cleanedup by cloudwatch. + """ + + @classmethod + def setUpClass(cls): + cls.cw_client = boto3.client('cloudwatch') + cls.cw_metric_publisher = CWMetricsPublisher(cls.cw_client) + + def test_publish_single_metric(self): + now = datetime.now() + tomorrow = now + timedelta(days=1) + namespace = self.get_unique_namespace() + metrics = Metrics(namespace, CWMetricsPublisher(self.cw_client)) + dimensions = [{"Name": "Dim1", "Value": "Val1"}, {"Name": "Dim2", "Value": "Val2"}] + metrics.record_count('TestCountMetric', 1, dimensions=dimensions) + metrics.record_count('TestCountMetric', 3, dimensions=dimensions) + metrics.record_latency('TestLatencyMetric', 1200, dimensions=dimensions) + metrics.record_latency('TestLatencyMetric', 1600, dimensions=dimensions) + metrics.publish() + total_count = self.get_metric_data(namespace, 'TestCountMetric', dimensions, + datetime(now.year, now.month, now.day), + datetime(tomorrow.year, tomorrow.month, tomorrow.day)) + latency_avg = self.get_metric_data(namespace, 'TestLatencyMetric', dimensions, + datetime(now.year, now.month, now.day), + datetime(tomorrow.year, tomorrow.month, tomorrow.day), stat='Average') + + self.assertEqual(total_count[0], 1 + 3) + self.assertEqual(latency_avg[0], 1400) + + def get_unique_namespace(self): + namespace = "SinglePublishTest-{}".format(uuid.uuid1()) + while True: + response = self.cw_client.list_metrics(Namespace=namespace) + if not response['Metrics']: + return namespace + namespace = "SinglePublishTest-{}".format(uuid.uuid1()) + + def get_metric_data(self, namespace, metric_name, dimensions, start_time, end_time, stat='Sum'): + retries = 3 + while retries > 0: + retries -= 1 + response = self.cw_client.get_metric_data(MetricDataQueries=[{ + 'Id': namespace.replace('-', '_').lower(), + 'MetricStat': { + 'Metric': { + 'Namespace': namespace, + 'MetricName': metric_name, + 'Dimensions': dimensions + }, + 'Period': 60, + 'Stat': stat + } + }], StartTime=start_time, EndTime=end_time) + values = response['MetricDataResults'][0]['Values'] + if values: + return values + print("No values found by for metric: {}. Waiting for 5 seconds...".format(metric_name)) + time.sleep(5) + return [0] diff --git a/samtranslator/metrics/metrics.py b/samtranslator/metrics/metrics.py index b70277bbfd..aa12d203e6 100644 --- a/samtranslator/metrics/metrics.py +++ b/samtranslator/metrics/metrics.py @@ -1,4 +1,3 @@ -from botocore.exceptions import ClientError import logging LOG = logging.getLogger(__name__) diff --git a/tests/metrics/test_metrics.py b/tests/metrics/test_metrics.py index b62ca280cd..09c5ff93c6 100644 --- a/tests/metrics/test_metrics.py +++ b/tests/metrics/test_metrics.py @@ -1,8 +1,6 @@ -from mock import patch, Mock -from botocore.exceptions import ClientError from parameterized import parameterized, param from unittest import TestCase -from mock import MagicMock, patch +from mock import MagicMock from samtranslator.metrics.metrics import ( Metrics, MetricsPublisher, From 89b620b9021d61da9e7fe9ab2e7372291bef1857 Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Mon, 21 Jun 2021 10:06:03 -0700 Subject: [PATCH 5/9] Black reformatting --- .../metrics/test_metrics_integration.py | 63 ++++---- tests/metrics/test_metrics.py | 134 ++++++++++++------ 2 files changed, 129 insertions(+), 68 deletions(-) diff --git a/integration/metrics/test_metrics_integration.py b/integration/metrics/test_metrics_integration.py index 70790ade5f..e2a69b757a 100644 --- a/integration/metrics/test_metrics_integration.py +++ b/integration/metrics/test_metrics_integration.py @@ -17,7 +17,7 @@ class MetricsIntegrationTest(TestCase): @classmethod def setUpClass(cls): - cls.cw_client = boto3.client('cloudwatch') + cls.cw_client = boto3.client("cloudwatch") cls.cw_metric_publisher = CWMetricsPublisher(cls.cw_client) def test_publish_single_metric(self): @@ -26,17 +26,26 @@ def test_publish_single_metric(self): namespace = self.get_unique_namespace() metrics = Metrics(namespace, CWMetricsPublisher(self.cw_client)) dimensions = [{"Name": "Dim1", "Value": "Val1"}, {"Name": "Dim2", "Value": "Val2"}] - metrics.record_count('TestCountMetric', 1, dimensions=dimensions) - metrics.record_count('TestCountMetric', 3, dimensions=dimensions) - metrics.record_latency('TestLatencyMetric', 1200, dimensions=dimensions) - metrics.record_latency('TestLatencyMetric', 1600, dimensions=dimensions) + metrics.record_count("TestCountMetric", 1, dimensions=dimensions) + metrics.record_count("TestCountMetric", 3, dimensions=dimensions) + metrics.record_latency("TestLatencyMetric", 1200, dimensions=dimensions) + metrics.record_latency("TestLatencyMetric", 1600, dimensions=dimensions) metrics.publish() - total_count = self.get_metric_data(namespace, 'TestCountMetric', dimensions, - datetime(now.year, now.month, now.day), - datetime(tomorrow.year, tomorrow.month, tomorrow.day)) - latency_avg = self.get_metric_data(namespace, 'TestLatencyMetric', dimensions, - datetime(now.year, now.month, now.day), - datetime(tomorrow.year, tomorrow.month, tomorrow.day), stat='Average') + total_count = self.get_metric_data( + namespace, + "TestCountMetric", + dimensions, + datetime(now.year, now.month, now.day), + datetime(tomorrow.year, tomorrow.month, tomorrow.day), + ) + latency_avg = self.get_metric_data( + namespace, + "TestLatencyMetric", + dimensions, + datetime(now.year, now.month, now.day), + datetime(tomorrow.year, tomorrow.month, tomorrow.day), + stat="Average", + ) self.assertEqual(total_count[0], 1 + 3) self.assertEqual(latency_avg[0], 1400) @@ -45,27 +54,29 @@ def get_unique_namespace(self): namespace = "SinglePublishTest-{}".format(uuid.uuid1()) while True: response = self.cw_client.list_metrics(Namespace=namespace) - if not response['Metrics']: + if not response["Metrics"]: return namespace namespace = "SinglePublishTest-{}".format(uuid.uuid1()) - def get_metric_data(self, namespace, metric_name, dimensions, start_time, end_time, stat='Sum'): + def get_metric_data(self, namespace, metric_name, dimensions, start_time, end_time, stat="Sum"): retries = 3 while retries > 0: retries -= 1 - response = self.cw_client.get_metric_data(MetricDataQueries=[{ - 'Id': namespace.replace('-', '_').lower(), - 'MetricStat': { - 'Metric': { - 'Namespace': namespace, - 'MetricName': metric_name, - 'Dimensions': dimensions - }, - 'Period': 60, - 'Stat': stat - } - }], StartTime=start_time, EndTime=end_time) - values = response['MetricDataResults'][0]['Values'] + response = self.cw_client.get_metric_data( + MetricDataQueries=[ + { + "Id": namespace.replace("-", "_").lower(), + "MetricStat": { + "Metric": {"Namespace": namespace, "MetricName": metric_name, "Dimensions": dimensions}, + "Period": 60, + "Stat": stat, + }, + } + ], + StartTime=start_time, + EndTime=end_time, + ) + values = response["MetricDataResults"][0]["Values"] if values: return values print("No values found by for metric: {}. Waiting for 5 seconds...".format(metric_name)) diff --git a/tests/metrics/test_metrics.py b/tests/metrics/test_metrics.py index 09c5ff93c6..d963db0035 100644 --- a/tests/metrics/test_metrics.py +++ b/tests/metrics/test_metrics.py @@ -7,7 +7,7 @@ CWMetricsPublisher, DummyMetricsPublisher, Unit, - MetricDatum + MetricDatum, ) @@ -25,10 +25,20 @@ def publish(self, namespace, metrics): class TestMetrics(TestCase): @parameterized.expand( [ - param("DummyNamespace", "CountMetric", 12, Unit.Count, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), - param("DummyNamespace", "IAMError", 59, Unit.Count, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param( + "DummyNamespace", + "CountMetric", + 12, + Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), + param( + "DummyNamespace", + "IAMError", + 59, + Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), ] ) def test_publishing_count_metric(self, namespace, name, value, unit, dimensions): @@ -38,16 +48,26 @@ def test_publishing_count_metric(self, namespace, name, value, unit, dimensions) metrics.publish() self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() - self.assertEqual(published_metric['MetricName'], name) - self.assertEqual(published_metric['Dimensions'], dimensions) - self.assertEqual(published_metric['Value'], value) + self.assertEqual(published_metric["MetricName"], name) + self.assertEqual(published_metric["Dimensions"], dimensions) + self.assertEqual(published_metric["Value"], value) @parameterized.expand( [ - param("DummyNamespace", "SARLatency", 1200, Unit.Milliseconds, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), - param("DummyNamespace", "IAMLatency", 400, Unit.Milliseconds, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param( + "DummyNamespace", + "SARLatency", + 1200, + Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), + param( + "DummyNamespace", + "IAMLatency", + 400, + Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), ] ) def test_publishing_latency_metric(self, namespace, name, value, unit, dimensions): @@ -57,16 +77,26 @@ def test_publishing_latency_metric(self, namespace, name, value, unit, dimension metrics.publish() self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() - self.assertEqual(published_metric['MetricName'], name) - self.assertEqual(published_metric['Dimensions'], dimensions) - self.assertEqual(published_metric['Value'], value) + self.assertEqual(published_metric["MetricName"], name) + self.assertEqual(published_metric["Dimensions"], dimensions) + self.assertEqual(published_metric["Value"], value) @parameterized.expand( [ - param("DummyNamespace", "CountMetric", 12, Unit.Count, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), - param("DummyNamespace", "LatencyMetric", 1200, Unit.Milliseconds, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param( + "DummyNamespace", + "CountMetric", + 12, + Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), + param( + "DummyNamespace", + "LatencyMetric", + 1200, + Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), ] ) def test_publishing_metric_without_calling_publish(self, namespace, name, value, unit, dimensions): @@ -76,22 +106,42 @@ def test_publishing_metric_without_calling_publish(self, namespace, name, value, del metrics self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() - self.assertEqual(published_metric['MetricName'], name) - self.assertEqual(published_metric['Dimensions'], dimensions) - self.assertEqual(published_metric['Value'], value) + self.assertEqual(published_metric["MetricName"], name) + self.assertEqual(published_metric["Dimensions"], dimensions) + self.assertEqual(published_metric["Value"], value) class TestCWMetricPublisher(TestCase): @parameterized.expand( [ - param("DummyNamespace", "CountMetric", 12, Unit.Count, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), - param("DummyNamespace", "IAMError", 59, Unit.Count, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), - param("DummyNamespace", "SARLatency", 1200, Unit.Milliseconds, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), - param("DummyNamespace", "IAMLatency", 400, Unit.Milliseconds, - [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}]), + param( + "DummyNamespace", + "CountMetric", + 12, + Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), + param( + "DummyNamespace", + "IAMError", + 59, + Unit.Count, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), + param( + "DummyNamespace", + "SARLatency", + 1200, + Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), + param( + "DummyNamespace", + "IAMLatency", + 400, + Unit.Milliseconds, + [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], + ), ] ) def test_publish_metric(self, namespace, name, value, unit, dimensions): @@ -101,12 +151,12 @@ def test_publish_metric(self, namespace, name, value, unit, dimensions): metrics = [metric_datum] metric_publisher.publish(namespace, metrics) call_kwargs = mock_cw_client.put_metric_data.call_args.kwargs - published_metric_data = call_kwargs['MetricData'][0] - self.assertEqual(call_kwargs['Namespace'], namespace) - self.assertEqual(published_metric_data['MetricName'], name) - self.assertEqual(published_metric_data['Unit'], unit) - self.assertEqual(published_metric_data['Value'], value) - self.assertEqual(published_metric_data['Dimensions'], dimensions) + published_metric_data = call_kwargs["MetricData"][0] + self.assertEqual(call_kwargs["Namespace"], namespace) + self.assertEqual(published_metric_data["MetricName"], name) + self.assertEqual(published_metric_data["Unit"], unit) + self.assertEqual(published_metric_data["Value"], value) + self.assertEqual(published_metric_data["Dimensions"], dimensions) @parameterized.expand( [ @@ -126,22 +176,22 @@ def test_publish_more_than_20_metrics(self, namespace, name, value, unit, dimens self.assertEqual(mock_cw_client.put_metric_data.call_count, 3) # Validating that metrics are published in batches of 20 - self.assertEqual(len(call_args_list[0].kwargs['MetricData']), min(total_metrics, metric_publisher.BATCH_SIZE)) + self.assertEqual(len(call_args_list[0].kwargs["MetricData"]), min(total_metrics, metric_publisher.BATCH_SIZE)) total_metrics -= metric_publisher.BATCH_SIZE - self.assertEqual(len(call_args_list[1].kwargs['MetricData']), min(total_metrics, metric_publisher.BATCH_SIZE)) + self.assertEqual(len(call_args_list[1].kwargs["MetricData"]), min(total_metrics, metric_publisher.BATCH_SIZE)) total_metrics -= metric_publisher.BATCH_SIZE - self.assertEqual(len(call_args_list[2].kwargs['MetricData']), min(total_metrics, metric_publisher.BATCH_SIZE)) + self.assertEqual(len(call_args_list[2].kwargs["MetricData"]), min(total_metrics, metric_publisher.BATCH_SIZE)) def test_do_not_fail_on_cloudwatch_any_exception(self): mock_cw_client = MagicMock() mock_cw_client.put_metric_data = MagicMock() mock_cw_client.put_metric_data.side_effect = Exception("BOOM!", "FAILED!!") metric_publisher = CWMetricsPublisher(mock_cw_client) - single_metric = MetricDatum('Name', 20, Unit.Count, []) - metric_publisher.publish('SomeNamespace', [single_metric]) + single_metric = MetricDatum("Name", 20, Unit.Count, []) + metric_publisher.publish("SomeNamespace", [single_metric]) self.assertTrue(True) def test_for_code_coverage(self): dummy_publisher = DummyMetricsPublisher() - dummy_publisher.publish('NS', [None]) + dummy_publisher.publish("NS", [None]) self.assertTrue(True) From f2d1236c34bd8356c7caa5282b734ab548175440 Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Mon, 21 Jun 2021 10:14:53 -0700 Subject: [PATCH 6/9] Fixing unit test. --- samtranslator/metrics/metrics.py | 3 ++- tests/metrics/test_metrics.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/samtranslator/metrics/metrics.py b/samtranslator/metrics/metrics.py index aa12d203e6..9ef0920b29 100644 --- a/samtranslator/metrics/metrics.py +++ b/samtranslator/metrics/metrics.py @@ -49,7 +49,8 @@ def _flush_metrics(self, namespace, metrics): try: self.cloudwatch_client.put_metric_data(Namespace=namespace, MetricData=metric_data) except Exception as e: - LOG.exception("Failed to report {} metrics".format(len(metric_data)), metrics, e) + LOG.error("Failed to report {} metrics".format(len(metric_data))) + LOG.exception(e) class DummyMetricsPublisher(MetricsPublisher): diff --git a/tests/metrics/test_metrics.py b/tests/metrics/test_metrics.py index d963db0035..608a63a9c6 100644 --- a/tests/metrics/test_metrics.py +++ b/tests/metrics/test_metrics.py @@ -185,7 +185,7 @@ def test_publish_more_than_20_metrics(self, namespace, name, value, unit, dimens def test_do_not_fail_on_cloudwatch_any_exception(self): mock_cw_client = MagicMock() mock_cw_client.put_metric_data = MagicMock() - mock_cw_client.put_metric_data.side_effect = Exception("BOOM!", "FAILED!!") + mock_cw_client.put_metric_data.side_effect = Exception("BOOM FAILED!!") metric_publisher = CWMetricsPublisher(mock_cw_client) single_metric = MetricDatum("Name", 20, Unit.Count, []) metric_publisher.publish("SomeNamespace", [single_metric]) From aa24ad82d21ac6c46736c1c06ffdc4711e4fd25d Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Wed, 23 Jun 2021 13:22:22 -0700 Subject: [PATCH 7/9] Addressing PR comments. --- samtranslator/metrics/metrics.py | 16 +++++++++------- tests/metrics/test_metrics.py | 18 ++++++------------ 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/samtranslator/metrics/metrics.py b/samtranslator/metrics/metrics.py index 9ef0920b29..2b979eaf79 100644 --- a/samtranslator/metrics/metrics.py +++ b/samtranslator/metrics/metrics.py @@ -1,3 +1,6 @@ +""" +Helper classes to publish metrics +""" import logging LOG = logging.getLogger(__name__) @@ -49,8 +52,7 @@ def _flush_metrics(self, namespace, metrics): try: self.cloudwatch_client.put_metric_data(Namespace=namespace, MetricData=metric_data) except Exception as e: - LOG.error("Failed to report {} metrics".format(len(metric_data))) - LOG.exception(e) + LOG.exception("Failed to report {} metrics".format(len(metric_data)), exc_info=e) class DummyMetricsPublisher(MetricsPublisher): @@ -59,7 +61,7 @@ def __init__(self): def publish(self, namespace, metrics): """Method to actually publish them metric""" - LOG.info("Dummy publisher ignoring {} metrices".format(len(metrics))) + LOG.debug("Dummy publisher ignoring {} metrices".format(len(metrics))) class Unit: @@ -128,7 +130,7 @@ def _record_metric(self, name, value, unit, dimensions=[]): """ self.metrics_cache.append(MetricDatum(name, value, unit, dimensions)) - def record_count(self, name, value, unit=Unit.Count, dimensions=[]): + def record_count(self, name, value, dimensions=[]): """ Create metric with unit Count. @@ -137,9 +139,9 @@ def record_count(self, name, value, unit=Unit.Count, dimensions=[]): :param unit: unit of metric (try using values from Unit class) :param dimensions: array of dimensions applied to the metric """ - self._record_metric(name, value, unit, dimensions) + self._record_metric(name, value, Unit.Count, dimensions) - def record_latency(self, name, value, unit=Unit.Milliseconds, dimensions=[]): + def record_latency(self, name, value, dimensions=[]): """ Create metric with unit Milliseconds. @@ -148,7 +150,7 @@ def record_latency(self, name, value, unit=Unit.Milliseconds, dimensions=[]): :param unit: unit of metric (try using values from Unit class) :param dimensions: array of dimensions applied to the metric """ - self._record_metric(name, value, unit, dimensions) + self._record_metric(name, value, Unit.Milliseconds, dimensions) def publish(self): """Method to publish all metrics, do not forget to call this method.""" diff --git a/tests/metrics/test_metrics.py b/tests/metrics/test_metrics.py index 608a63a9c6..52165f4cfc 100644 --- a/tests/metrics/test_metrics.py +++ b/tests/metrics/test_metrics.py @@ -29,22 +29,20 @@ class TestMetrics(TestCase): "DummyNamespace", "CountMetric", 12, - Unit.Count, [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], ), param( "DummyNamespace", "IAMError", 59, - Unit.Count, [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], ), ] ) - def test_publishing_count_metric(self, namespace, name, value, unit, dimensions): + def test_publishing_count_metric(self, namespace, name, value, dimensions): mock_metrics_publisher = MetricPublisherTestHelper() metrics = Metrics(namespace, mock_metrics_publisher) - metrics.record_count(name, value, unit, dimensions) + metrics.record_count(name, value, dimensions) metrics.publish() self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() @@ -58,22 +56,20 @@ def test_publishing_count_metric(self, namespace, name, value, unit, dimensions) "DummyNamespace", "SARLatency", 1200, - Unit.Milliseconds, [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], ), param( "DummyNamespace", "IAMLatency", 400, - Unit.Milliseconds, [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], ), ] ) - def test_publishing_latency_metric(self, namespace, name, value, unit, dimensions): + def test_publishing_latency_metric(self, namespace, name, value, dimensions): mock_metrics_publisher = MetricPublisherTestHelper() metrics = Metrics(namespace, mock_metrics_publisher) - metrics.record_latency(name, value, unit, dimensions) + metrics.record_latency(name, value, dimensions) metrics.publish() self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() @@ -87,22 +83,20 @@ def test_publishing_latency_metric(self, namespace, name, value, unit, dimension "DummyNamespace", "CountMetric", 12, - Unit.Count, [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], ), param( "DummyNamespace", "LatencyMetric", 1200, - Unit.Milliseconds, [{"Name": "SAM", "Value": "Dim1"}, {"Name": "SAM", "Value": "Dim2"}], ), ] ) - def test_publishing_metric_without_calling_publish(self, namespace, name, value, unit, dimensions): + def test_publishing_metric_without_calling_publish(self, namespace, name, value, dimensions): mock_metrics_publisher = MetricPublisherTestHelper() metrics = Metrics(namespace, mock_metrics_publisher) - metrics.record_count(name, value, unit, dimensions) + metrics.record_count(name, value, dimensions) del metrics self.assertEqual(len(mock_metrics_publisher.metrics_cache), 1) published_metric = mock_metrics_publisher.metrics_cache[0].get_metric_data() From 26403fea3a0bece31c569df7cf691e86f5cdd0fc Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Wed, 23 Jun 2021 14:37:15 -0700 Subject: [PATCH 8/9] Clearing __init__py from test module. --- tests/metrics/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 tests/metrics/__init__.py diff --git a/tests/metrics/__init__.py b/tests/metrics/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 From 953cce8de18b06fe18cbeda786a4bef7f51dbbcc Mon Sep 17 00:00:00 2001 From: Tarun Mall Date: Wed, 23 Jun 2021 14:42:37 -0700 Subject: [PATCH 9/9] Updating documentation for publish method. --- samtranslator/metrics/metrics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samtranslator/metrics/metrics.py b/samtranslator/metrics/metrics.py index 2b979eaf79..42e83808cd 100644 --- a/samtranslator/metrics/metrics.py +++ b/samtranslator/metrics/metrics.py @@ -60,7 +60,7 @@ def __init__(self): MetricsPublisher.__init__(self) def publish(self, namespace, metrics): - """Method to actually publish them metric""" + """Do not publish any metric, this is a dummy publisher used for offline use.""" LOG.debug("Dummy publisher ignoring {} metrices".format(len(metrics))) @@ -153,6 +153,6 @@ def record_latency(self, name, value, dimensions=[]): self._record_metric(name, value, Unit.Milliseconds, dimensions) def publish(self): - """Method to publish all metrics, do not forget to call this method.""" + """Calls publish method from the configured metrics publisher to publish metrics""" self.metrics_publisher.publish(self.namespace, self.metrics_cache) self.metrics_cache = []