Repository: ApptuitAI/apptuit-py

Issue:
Global tags should not over-ride tags from the datapoint
If the same tag key-value pair is on the datapoint and in global tags of the reporter, current implementation prefers the global tag:
https://github.com/ApptuitAI/apptuit-py/blob/9872766e4838fced1a580f4da3b44acf5fd3ea81/apptuit/pyformance/apptuit_reporter.py#L62


We should prefer the tag key-value of the data point over the global values

Discussion:

Patch:
diff --git a/apptuit/pyformance/apptuit_reporter.py b/apptuit/pyformance/apptuit_reporter.py
index a8e2a65..1f6896e 100644
--- a/apptuit/pyformance/apptuit_reporter.py
+++ b/apptuit/pyformance/apptuit_reporter.py
@@ -10,13 +10,13 @@ class ApptuitReporter(Reporter):
     def __init__(self, registry=None, reporting_interval=10, token=None,
                  api_endpoint="https://api.apptuit.ai", prefix="", tags=None):
         super(ApptuitReporter, self).__init__(registry=registry,
-             reporting_interval=reporting_interval)
+                                              reporting_interval=reporting_interval)
         self.endpoint = api_endpoint
-        self.metric_tags = {}
         self.token = token
         self.tags = tags
-        self.prefix = prefix
+        self.prefix = prefix if prefix is not None else ""
         self.client = Apptuit(token, api_endpoint)
+        self.__decoded_metrics_cache = {}
 
     def report_now(self, registry=None, timestamp=None):
         """
@@ -26,7 +26,7 @@ class ApptuitReporter(Reporter):
             timestamp: timestamp of the data point
         """
         dps = self._collect_data_points(registry or self.registry, timestamp)
-        if len(dps) > 0:
+        if dps:
             self.client.send(dps)
 
     def _get_tags(self, key):
@@ -37,11 +37,12 @@ class ApptuitReporter(Reporter):
         Returns:
             metric name, dictionary of tags
         """
-        if key not in self.metric_tags.keys():
-            metric_name, metric_tags = timeseries.decode_metric(key)
-            self.metric_tags[key] = (metric_name, metric_tags)
-        else:
-            metric_name, metric_tags = self.metric_tags[key]
+        val = self.__decoded_metrics_cache.get(key)
+        if val:
+            return val[0], val[1]
+
+        metric_name, metric_tags = timeseries.decode_metric(key)
+        self.__decoded_metrics_cache[key] = (metric_name, metric_tags)
         return metric_name, metric_tags
 
     def _collect_data_points(self, registry, timestamp=None):
@@ -56,13 +57,19 @@ class ApptuitReporter(Reporter):
         timestamp = timestamp or int(round(self.clock.time()))
         metrics = registry.dump_metrics()
         dps = []
+        global_tags = self.tags if self.tags else {}
         for key in metrics.keys():
             metric_name, metric_tags = self._get_tags(key)
-            if self.tags is not None and len(self.tags) != 0:
-                metric_tags.update(self.tags)
+            if metric_tags and global_tags:
+                tags = global_tags.copy()
+                tags.update(metric_tags)
+            elif metric_tags:
+                tags = metric_tags
+            else:
+                tags = global_tags
             for value_key in metrics[key].keys():
                 dps.append(DataPoint(metric="{0}{1}.{2}".format(self.prefix, metric_name, value_key),
-                                     tags=metric_tags,
+                                     tags=tags,
                                      timestamp=timestamp,
                                      value=metrics[key][value_key]))
         return dps


Test patch:
diff --git a/tests/test_pyformance_reporter.py b/tests/test_pyformance_reporter.py
index aaa6d1c..65d5537 100644
--- a/tests/test_pyformance_reporter.py
+++ b/tests/test_pyformance_reporter.py
@@ -3,7 +3,7 @@
 """
 import random
 import time
-from nose.tools import assert_raises, assert_equals, assert_greater_equal
+from nose.tools import assert_raises, assert_equals, assert_greater_equal, assert_true
 from requests.exceptions import HTTPError
 from apptuit import ApptuitException
 from apptuit.pyformance.apptuit_reporter import ApptuitReporter
@@ -214,5 +214,86 @@ def test_collect_data_points():
     assert_equals(len(dps), 1)
     assert_equals(dps[0].value, 2)
     assert_equals(dps[0].metric, "apr.counter.count")
-    assert_equals(dps[0].tags, {'host': 'localhost', 'region': 'us-east-1', 'service': 'web-server', 'tk1': 'tv1',
+    assert_equals(dps[0].tags, {'host': 'localhost', 'region': 'us-east-1',
+                                'service': 'web-server', 'tk1': 'tv1',
                                 'tk2': 'tv2'})
+
+def test_globaltags_override():
+    """
+        Test that if the global tags and metric tags contain same tag key,
+        the metric tags override global tags
+    """
+    token = "asdashdsauh_8aeraerf"
+    tags = {"region": "us-east-1"}
+    registry = MetricsRegistry()
+    reporter = ApptuitReporter(registry=registry,
+                               reporting_interval=1,
+                               token=token,
+                               tags=tags)
+    counter1 = registry.counter('counter1 {"region":"us-west-2","id": 1}')
+    counter2 = registry.counter('counter2 {"region":"us-west-3","id": 2, "new_tag": "foo"}')
+    counter3 = registry.counter('counter3')
+    counter1.inc(2)
+    counter2.inc()
+    counter3.inc()
+    dps = reporter._collect_data_points(reporter.registry)
+    dps = sorted(dps, key=lambda x: x.metric)
+    assert_equals(dps[0].tags, {"region": "us-west-2", "id": 1})
+    assert_equals(dps[1].tags, {"region": "us-west-3", "id": 2, "new_tag": "foo"})
+    assert_equals(dps[2].tags, {"region": "us-east-1"})
+    assert_equals(reporter.tags, {"region": "us-east-1"})
+
+def test_globaltags_none():
+    """
+        Test that metric tags work when global tags are not present
+    """
+    token = "asdashdsauh_8aeraerf"
+    tags = {"region": "us-east-1"}
+    registry = MetricsRegistry()
+    reporter = ApptuitReporter(registry=registry,
+                               reporting_interval=1,
+                               token=token,
+                               tags=None)
+    counter1 = registry.counter('counter1 {"region":"us-west-2","id": 1}')
+    counter2 = registry.counter('counter2 {"region":"us-west-3","id": 2, "new_tag": "foo"}')
+    counter1.inc(2)
+    counter2.inc()
+    dps = reporter._collect_data_points(reporter.registry)
+    dps = sorted(dps, key=lambda x: x.metric)
+    assert_equals(dps[0].tags, {"region": "us-west-2", "id": 1})
+    assert_equals(dps[1].tags, {"region": "us-west-3", "id": 2, "new_tag": "foo"})
+    assert_true(reporter.tags is None)
+
+def test_valid_prefix():
+    """
+        Test that prefix works
+    """
+    token = "asdashdsauh_8aeraerf"
+    tags = {"region": "us-east-1"}
+    registry = MetricsRegistry()
+    reporter = ApptuitReporter(registry=registry,
+                               reporting_interval=1,
+                               prefix="pre-",
+                               token=token,
+                               tags=tags)
+    counter1 = registry.counter('counter1')
+    counter1.inc()
+    dps = reporter._collect_data_points(reporter.registry)
+    assert_equals(dps[0].metric, "pre-counter1.count")
+
+def test_none_prefix():
+    """
+        Test for None prefix
+    """
+    token = "asdashdsauh_8aeraerf"
+    tags = {"region": "us-east-1"}
+    registry = MetricsRegistry()
+    reporter = ApptuitReporter(registry=registry,
+                               reporting_interval=1,
+                               prefix=None,
+                               token=token,
+                               tags=tags)
+    counter1 = registry.counter('counter1')
+    counter1.inc()
+    dps = reporter._collect_data_points(reporter.registry)
+    assert_equals(dps[0].metric, "counter1.count")