storage: merge GranularityDoesNotExist and AggregationDoesNotExist

We often don't have enough information to distinguish the two and it does not matter in the end. Simplify the code base a bit by merging the two without losing information.
gnocchixyz · Dec 14, 2017 · 965f908 · 965f908
1 parent 572f015
commit 965f908
Show file tree

Hide file tree

Showing 13 changed files with 55 additions and 35 deletions.
diff --git a/gnocchi/rest/aggregates/processor.py b/gnocchi/rest/aggregates/processor.py
@@ -75,8 +75,12 @@ def get_measures(storage, references, operations,
     for ref in references:
         if (ref.aggregation not in
                 ref.metric.archive_policy.aggregation_methods):
-            raise gnocchi_storage.AggregationDoesNotExist(ref.metric,
-                                                          ref.aggregation)
+            raise gnocchi_storage.AggregationDoesNotExist(
+                ref.metric, ref.aggregation,
+                # Use the first granularity, that should be good enough since
+                # they are all missing anyway
+                ref.metric.archive_policy.definition[0].granularity)
+
         if granularity is not None:
             for d in ref.metric.archive_policy.definition:
                 if d.granularity == granularity:

diff --git a/gnocchi/rest/api.py b/gnocchi/rest/api.py
@@ -513,7 +513,6 @@ def get_measures(self, start=None, stop=None, aggregation='mean',
                 self.metric, start, stop, aggregation,
                 granularity, resample)
         except (storage.MetricDoesNotExist,
-                storage.GranularityDoesNotExist,
                 storage.AggregationDoesNotExist) as e:
             abort(404, six.text_type(e))
 
@@ -1497,8 +1496,8 @@ def post(self, metric_id, start=None, stop=None, aggregation='mean',
             }
         except storage.InvalidQuery as e:
             abort(400, six.text_type(e))
-        except storage.GranularityDoesNotExist as e:
-            abort(400, six.text_type(e))
+        except storage.AggregationDoesNotExist as e:
+            abort(400, e)
 
 
 class ResourcesMetricsMeasuresBatchController(rest.RestController):
@@ -1837,7 +1836,6 @@ def get_cross_metric_measures_from_objs(metrics, start=None, stop=None,
         except exceptions.UnAggregableTimeseries as e:
             abort(400, e)
         except (storage.MetricDoesNotExist,
-                storage.GranularityDoesNotExist,
                 storage.AggregationDoesNotExist) as e:
             abort(404, six.text_type(e))
 

diff --git a/gnocchi/storage/__init__.py b/gnocchi/storage/__init__.py
@@ -59,23 +59,25 @@ def __init__(self, metric):
 class AggregationDoesNotExist(StorageError):
     """Error raised when the aggregation method doesn't exists for a metric."""
 
-    def __init__(self, metric, method):
+    def __init__(self, metric, method, granularity):
         self.metric = metric
         self.method = method
-        super(AggregationDoesNotExist, self).__init__(
-            "Aggregation method '%s' for metric %s does not exist" %
-            (method, metric))
-
-
-class GranularityDoesNotExist(StorageError):
-    """Error raised when the granularity doesn't exist for a metric."""
-
-    def __init__(self, metric, granularity):
-        self.metric = metric
         self.granularity = granularity
-        super(GranularityDoesNotExist, self).__init__(
-            "Granularity '%s' for metric %s does not exist" %
-            (utils.timespan_total_seconds(granularity), metric))
+        super(AggregationDoesNotExist, self).__init__(
+            "Aggregation method '%s' at granularity '%s' "
+            "for metric %s does not exist" %
+            (method, utils.timespan_total_seconds(granularity), metric))
+
+    def jsonify(self):
+        return {
+            "cause": "Aggregation does not exist",
+            "detail": {
+                # FIXME(jd) Pecan does not use our JSON renderer for errors
+                # So we need to convert this
+                "granularity": utils.timespan_total_seconds(self.granularity),
+                "aggregation_method": self.method,
+            },
+        }
 
 
 class MetricAlreadyExists(StorageError):
@@ -198,7 +200,9 @@ def get_measures(self, metric, from_timestamp=None, to_timestamp=None,
         :param resample: The granularity to resample to.
         """
         if aggregation not in metric.archive_policy.aggregation_methods:
-            raise AggregationDoesNotExist(metric, aggregation)
+            if granularity is None:
+                granularity = metric.archive_policy.definition[0].granularity
+            raise AggregationDoesNotExist(metric, aggregation, granularity)
 
         if granularity is None:
             agg_timeseries = utils.parallel_map(
@@ -239,7 +243,7 @@ def _get_measures_timeserie(self, metric,
                 points = d.points
                 break
         else:
-            raise GranularityDoesNotExist(metric, granularity)
+            raise AggregationDoesNotExist(metric, aggregation, granularity)
 
         try:
             all_keys = self._list_split_keys_for_metric(

diff --git a/gnocchi/storage/ceph.py b/gnocchi/storage/ceph.py
@@ -144,7 +144,8 @@ def _get_measures(self, metric, key, aggregation, version=3):
         except rados.ObjectNotFound:
             if self._object_exists(
                     self._build_unaggregated_timeserie_path(metric, 3)):
-                raise storage.AggregationDoesNotExist(metric, aggregation)
+                raise storage.AggregationDoesNotExist(
+                    metric, aggregation, key.sampling)
             else:
                 raise storage.MetricDoesNotExist(metric)
 

diff --git a/gnocchi/storage/file.py b/gnocchi/storage/file.py
@@ -150,6 +150,7 @@ def _get_measures(self, metric, key, aggregation, version=3):
         except IOError as e:
             if e.errno == errno.ENOENT:
                 if os.path.exists(self._build_metric_dir(metric)):
-                    raise storage.AggregationDoesNotExist(metric, aggregation)
+                    raise storage.AggregationDoesNotExist(
+                        metric, aggregation, key.sampling)
                 raise storage.MetricDoesNotExist(metric)
             raise
diff --git a/gnocchi/storage/redis.py b/gnocchi/storage/redis.py
@@ -99,5 +99,6 @@ def _get_measures(self, metric, key, aggregation, version=3):
         if data is None:
             if not self._client.exists(redis_key):
                 raise storage.MetricDoesNotExist(metric)
-            raise storage.AggregationDoesNotExist(metric, aggregation)
+            raise storage.AggregationDoesNotExist(
+                metric, aggregation, key.sampling)
         return data
diff --git a/gnocchi/storage/s3.py b/gnocchi/storage/s3.py
@@ -173,7 +173,8 @@ def _get_measures(self, metric, key, aggregation, version=3):
                     if e.response['Error'].get('Code') == 'NoSuchKey':
                         raise storage.MetricDoesNotExist(metric)
                     raise
-                raise storage.AggregationDoesNotExist(metric, aggregation)
+                raise storage.AggregationDoesNotExist(
+                    metric, aggregation, key.sampling)
             raise
         return response['Body'].read()
 

diff --git a/gnocchi/storage/swift.py b/gnocchi/storage/swift.py
@@ -155,7 +155,8 @@ def _get_measures(self, metric, key, aggregation, version=3):
                     if e.http_status == 404:
                         raise storage.MetricDoesNotExist(metric)
                     raise
-                raise storage.AggregationDoesNotExist(metric, aggregation)
+                raise storage.AggregationDoesNotExist(
+                    metric, aggregation, key.sampling)
             raise
         return contents
 

diff --git a/gnocchi/tests/functional/gabbits/aggregation.yaml b/gnocchi/tests/functional/gabbits/aggregation.yaml
@@ -176,7 +176,7 @@ tests:
       GET: /v1/aggregation/metric?metric=$HISTORY['get metric list'].$RESPONSE['$[0].id']&granularity=42
       status: 404
       response_strings:
-        - Granularity '42.0' for metric
+        - Aggregation method 'mean' at granularity '42.0' for metric
 
 # Aggregation by resource and metric_name
 

diff --git a/gnocchi/tests/functional/gabbits/metric-granularity.yaml b/gnocchi/tests/functional/gabbits/metric-granularity.yaml
@@ -43,7 +43,7 @@ tests:
       GET: /v1/metric/$RESPONSE['$[0].id']/measures?granularity=42
       status: 404
       response_strings:
-        - Granularity '42.0' for metric $RESPONSE['$[0].id'] does not exist
+        - Aggregation method 'mean' at granularity '42.0' for metric $RESPONSE['$[0].id'] does not exist
 
     - name: get measurements granularity
       GET: /v1/metric/$HISTORY['get metric list'].$RESPONSE['$[0].id']/measures?granularity=1

diff --git a/gnocchi/tests/functional/gabbits/metric.yaml b/gnocchi/tests/functional/gabbits/metric.yaml
@@ -367,7 +367,7 @@ tests:
       GET: /v1/aggregation/metric?metric=$HISTORY['get metric list for aggregates'].$RESPONSE['$[0].id']&aggregation=last
       status: 404
       response_strings:
-          - Aggregation method 'last' for metric $RESPONSE['$[0].id'] does not exist
+        - Aggregation method 'last' at granularity '1.0' for metric
 
     - name: aggregate measure unknown metric
       GET: /v1/aggregation/metric?metric=cee6ef1f-52cc-4a16-bbb5-648aedfd1c37

diff --git a/gnocchi/tests/functional/gabbits/search-metric.yaml b/gnocchi/tests/functional/gabbits/search-metric.yaml
@@ -90,16 +90,25 @@ tests:
       data:
           "=": 12
       status: 400
-      response_strings:
-          - Granularity '300.0' for metric $HISTORY['get metric id'].$RESPONSE['$[0].id'] does not exist
+      request_headers:
+          accept: application/json
+      response_json_paths:
+          $.description.cause: Aggregation does not exist
+          $.description.detail.granularity: 300
+          $.description.detail.aggregation_method: mean
+
 
     - name: search with incorrect granularity
       POST: /v1/search/metric?metric_id=$HISTORY['get metric id'].$RESPONSE['$[0].id']&granularity=300
       data:
           "=": 12
       status: 400
-      response_strings:
-          - Granularity '300.0' for metric $HISTORY['get metric id'].$RESPONSE['$[0].id'] does not exist
+      request_headers:
+          accept: application/json
+      response_json_paths:
+          $.description.cause: Aggregation does not exist
+          $.description.detail.granularity: 300
+          $.description.detail.aggregation_method: mean
 
     - name: search measure with wrong start
       POST: /v1/search/metric?metric_id=$HISTORY['get metric id'].$RESPONSE['$[0].id']&start=foobar

diff --git a/gnocchi/tests/test_storage.py b/gnocchi/tests/test_storage.py
@@ -821,7 +821,7 @@ def test_add_and_get_measures(self):
             to_timestamp=datetime64(2014, 1, 1, 12, 0, 2),
             granularity=numpy.timedelta64(5, 'm')))
 
-        self.assertRaises(storage.GranularityDoesNotExist,
+        self.assertRaises(storage.AggregationDoesNotExist,
                           self.storage.get_measures,
                           self.metric,
                           granularity=numpy.timedelta64(42, 's'))