From 6de90274e4f50746f339a6bc8917e3d37531a62a Mon Sep 17 00:00:00 2001 From: Saman Vaisipour Date: Mon, 8 Jul 2019 11:35:17 -0400 Subject: [PATCH] Second round of comments (Metrics) --- metrics.py | 26 +++++------ metrics_test.py | 114 +++++++++++++++++++++++++++--------------------- 2 files changed, 77 insertions(+), 63 deletions(-) diff --git a/metrics.py b/metrics.py index 3f14e45..64ca0d4 100644 --- a/metrics.py +++ b/metrics.py @@ -40,7 +40,7 @@ import uuid import requests -from typing import Dict, Optional +from typing import Dict, Optional, Text _CLEARCUT_ENDPOINT = 'https://play.googleapis.com/log' _CLOUD_HCLS = 'CLOUD_HCLS' @@ -69,12 +69,12 @@ class _ConcordEvent(object): """Encapsulates information representing a Concord event.""" def __init__(self, - event_name: str, - event_type: str, + event_name: Text, + event_type: Text, project_number: int, - console_type: str, - page_hostname: str, - event_metadata: Optional[Dict[str, str]] = None) -> None: + console_type: Text, + page_hostname: Text, + event_metadata: Optional[Dict[Text, Text]] = None) -> None: self._event_name = event_name self._event_type = event_type self._project_number = project_number @@ -108,11 +108,11 @@ class _MetricsCollector(object): Instances of this class share the same internal state, and thus behave the same all the time. """ - _shared_events = [] - _shared_session_identifier = uuid.uuid4().hex + _events = [] + _session_identifier = uuid.uuid4().hex def add_metrics(self, project_number: int, - metrics_name: str, **metrics_kw: str) -> None: + metrics_name: Text, **metrics_kw: Text) -> None: concord_event = _ConcordEvent( event_name=metrics_name, event_type=_DEEP_VARIANT_RUN, @@ -120,7 +120,7 @@ def add_metrics(self, project_number: int, console_type=_CLOUD_HCLS, page_hostname=_VIRTUAL_CHC_DEEPVARIANT, event_metadata={k: v for k, v in metrics_kw.items()}) - self._shared_events.append(concord_event) + self._events.append(concord_event) def submit_metrics(self): """Submits all the collected metrics to Concord endpoint. @@ -145,12 +145,12 @@ def _clearcut_request(self): 'log_source_name': _CONCORD, 'zwieback_cookie': - self._shared_session_identifier, + self._session_identifier, 'request_time_ms': _now_ms(), 'log_event': [{ 'source_extension_json': e.to_json(sort_keys=True) - } for e in self._shared_events] + } for e in self._events] } @@ -159,7 +159,7 @@ def _now_ms(): return int(round(time.time() * 1000)) -def add(project_number: int, metrics_name: str, **metrics_kw: str) -> None: +def add(project_number: int, metrics_name: Text, **metrics_kw: Text) -> None: """Adds the given metric to the metrics to be submitted to Concord. Note: All metrics are submitted at exit. diff --git a/metrics_test.py b/metrics_test.py index 17d1d52..3c1fe8c 100644 --- a/metrics_test.py +++ b/metrics_test.py @@ -42,41 +42,38 @@ import json import unittest -from metrics import _CLEARCUT_ENDPOINT as CLEARCUT_ENDPOINT -from metrics import _MetricsCollector as MetricsCollector + +import metrics import mock # This is to test if all metrics collector instances share same session -# identifier. Mocks '_shared_session_identifier' on import. -@mock.patch('metrics._MetricsCollector._shared_session_identifier', 'abcd') +# identifier. Mocks '_session_identifier' on import. +@mock.patch('metrics._MetricsCollector._session_identifier', 'abcd') class MetricsCollectorTest(unittest.TestCase): """Tests for MetricsCollector class.""" - def _clear_metrics_collector(self): - # 'metrics_collector' is a singleton. Remove any shared state before - # starting next test. - MetricsCollector()._shared_events[:] = [] + def setUp(self): + super(MetricsCollectorTest, self).setUp() + # 'metrics_collector' has class attributes, clear them before each test. + metrics._MetricsCollector()._events[:] = [] @mock.patch('requests.post') @mock.patch('time.time', return_value=1234) def test_submit_metrics(self, unused_mock_time, mock_requests_post): - self._clear_metrics_collector() - metrics_collector = MetricsCollector() - - metrics_collector.add_metrics( + metrics.add( 123, 'test-metrics-1', attribute_1=1, attribute_2='string-1', attribute_3=True) - metrics_collector.add_metrics( + metrics.add( 123, 'test-metrics-2', attribute_1=2, attribute_2='string-2', attribute_3=True) - metrics_collector.submit_metrics() + metrics._MetricsCollector().submit_metrics() mock_requests_post.assert_called_with( data=json.dumps( @@ -138,52 +135,69 @@ def test_submit_metrics(self, unused_mock_time, mock_requests_post): sort_keys=True), headers=None, timeout=10, - url=CLEARCUT_ENDPOINT) + url=metrics._CLEARCUT_ENDPOINT) @mock.patch('requests.post') @mock.patch('time.time', side_effect=(1234, 1235)) def test_two_metrics_collector(self, unused_mock_time, mock_requests_post): - self._clear_metrics_collector() - first_metric_collector = MetricsCollector() - second_metric_collector = MetricsCollector() + first_metric_collector = metrics._MetricsCollector() + second_metric_collector = metrics._MetricsCollector() first_metric_collector.add_metrics(123, 'test-metrics-1', attribute_1=1) second_metric_collector.add_metrics(123, 'test-metrics-2', attribute_2=2) + metrics.add(123, 'test-metrics-3', attribute_3=3) def expected_post_data(request_time_ms): template = { 'zwieback_cookie': 'abcd', 'log_source_name': 'CONCORD', - 'log_event': [{ - 'source_extension_json': - json.dumps({ - 'console_type': 'CLOUD_HCLS', - 'event_metadata': [{ - 'key': 'attribute_1', - 'value': '1' - }], - 'event_name': 'test-metrics-1', - 'event_type': 'DeepVariantRun', - 'page_hostname': 'virtual.chc.deepvariant', - 'project_number': 123 - }, - sort_keys=True) - }, - { - 'source_extension_json': - json.dumps({ - 'console_type': 'CLOUD_HCLS', - 'event_metadata': [{ - 'key': 'attribute_2', - 'value': '2' - }], - 'event_name': 'test-metrics-2', - 'event_type': 'DeepVariantRun', - 'page_hostname': 'virtual.chc.deepvariant', - 'project_number': 123 - }, - sort_keys=True) - }], + 'log_event': [ + { + 'source_extension_json': + json.dumps({ + 'console_type': 'CLOUD_HCLS', + 'event_metadata': [{ + 'key': 'attribute_1', + 'value': '1' + }], + 'event_name': 'test-metrics-1', + 'event_type': 'DeepVariantRun', + 'page_hostname': 'virtual.chc.deepvariant', + 'project_number': 123 + }, + sort_keys=True) + }, + { + 'source_extension_json': + json.dumps({ + 'console_type': 'CLOUD_HCLS', + 'event_metadata': [{ + 'key': 'attribute_2', + 'value': '2' + }], + 'event_name': 'test-metrics-2', + 'event_type': 'DeepVariantRun', + 'page_hostname': 'virtual.chc.deepvariant', + 'project_number': 123 + }, + sort_keys=True) + }, + { + 'source_extension_json': + json.dumps({ + 'console_type': 'CLOUD_HCLS', + 'event_metadata': [{ + 'key': 'attribute_3', + 'value': '3' + }], + 'event_name': 'test-metrics-3', + 'event_type': 'DeepVariantRun', + 'page_hostname': 'virtual.chc.deepvariant', + 'project_number': 123 + }, + sort_keys=True) + } + ], 'client_info': { 'client_type': 'PYTHON' } @@ -196,14 +210,14 @@ def expected_post_data(request_time_ms): data=expected_post_data(1234000), headers=None, timeout=10, - url=CLEARCUT_ENDPOINT) + url=metrics._CLEARCUT_ENDPOINT) second_metric_collector.submit_metrics() mock_requests_post.assert_called_with( data=expected_post_data(1235000), headers=None, timeout=10, - url=CLEARCUT_ENDPOINT) + url=metrics._CLEARCUT_ENDPOINT) if __name__ == '__main__':