Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pull request for no-ap-in-storage #583

Merged
3 commits merged into from
Dec 14, 2017
Merged

Pull request for no-ap-in-storage #583

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cea2f2e
storage: remove double check of granularity
jd Dec 13, 2017
2ec6f26
storage: merge GranularityDoesNotExist and AggregationDoesNotExist
jd Dec 13, 2017
4f14e6a
Merge branch 'master' into no-ap-in-storage
Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions gnocchi/rest/aggregates/processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 2 additions & 4 deletions gnocchi/rest/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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))

Expand Down
48 changes: 24 additions & 24 deletions gnocchi/storage/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. but it literally validates this right here and knows exactly if it's the granularity or the aggregate that is wrong... imo, if a storage driver does not find the actual corresponding file/object because it's yet to receive data, it should not return an error but return an empty timeseries becaues that's exactly what it is.

that said, i imagine this is because you are trying to address #548? is this the only way to handle it?

Copy link
Member Author

@jd jd Dec 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm actually targeting #175. For that I need to remove archive policy usage from the storage driver and move the actual detection of invalid requests to the API layer.

Which mean even this code you quote is going away ASAP :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. so how do you envision 175 working? there is no validation and the user can submit any combination of granularity+aggregate? because if there's validation at some point prior to this, i'm thinking it's still better to just return empty ts rather than error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the user can submit any combination of granularity+aggregate?

Exactly.

i'm thinking it's still better to just return empty ts rather than error.

Actually, it's better to raise an error in storage and leave the layer above (e.g. REST API) decides what behaviour it wants to implement

That being said, this PR do not change the current behaviour of the API or even of storage – it merges 2 different exception into one. I feel like we're talking about something that is not there (yet).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fair. i guess as long as we eventually move this validation stuff from storage then.


if granularity is None:
agg_timeseries = utils.parallel_map(
Expand Down Expand Up @@ -239,20 +243,16 @@ def _get_measures_timeserie(self, metric,
points = d.points
break
else:
raise GranularityDoesNotExist(metric, granularity)
raise AggregationDoesNotExist(metric, aggregation, granularity)

all_keys = None
try:
all_keys = self._list_split_keys_for_metric(
metric, aggregation, granularity)
except MetricDoesNotExist:
for d in metric.archive_policy.definition:
if d.granularity == granularity:
return carbonara.AggregatedTimeSerie(
sampling=granularity,
aggregation_method=aggregation,
max_size=d.points)
raise GranularityDoesNotExist(metric, granularity)
return carbonara.AggregatedTimeSerie(
sampling=granularity,
aggregation_method=aggregation,
max_size=points)

if from_timestamp:
from_timestamp = carbonara.SplitKey.from_timestamp_and_sampling(
Expand Down
3 changes: 2 additions & 1 deletion gnocchi/storage/ceph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
3 changes: 2 additions & 1 deletion gnocchi/storage/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,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
3 changes: 2 additions & 1 deletion gnocchi/storage/redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 2 additions & 1 deletion gnocchi/storage/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
3 changes: 2 additions & 1 deletion gnocchi/storage/swift.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion gnocchi/tests/functional/gabbits/aggregation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion gnocchi/tests/functional/gabbits/metric-granularity.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion gnocchi/tests/functional/gabbits/metric.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 13 additions & 4 deletions gnocchi/tests/functional/gabbits/search-metric.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion gnocchi/tests/test_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,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'))
Expand Down