Second round of comments (Metrics)

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,19 +108,19 @@ 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,
         project_number=project_number,
         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.

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,14 +135,13 @@ 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)
@@ -196,14 +192,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__':