square / metrics

Metrics Query Engine
Apache License 2.0
170 stars 21 forks source link

Converter API Change #251

Closed Nathan-Fenner closed 8 years ago

Nathan-Fenner commented 8 years ago

This PR is now ready for review.

Previously, things were organized like this:

Now, things are organized like this:

This PR makes the conversion of metrics from tagged to untagged form orthogonal to other parts of the system, namely the Timeseries storage API.

In practical terms, what this means is that the implementation for Blueflood no longer bothers doing any conversion. It is given "graphite" metrics, so those are all it needs to know about. Hypothetically, you could transparently switch from storing metrics as graphite names to compressed gzipped strings, and all you'd do is switch out the converter, without modifying Blueflood or its configuration.

Changes to Testing

Since MetricConverters are now needed to perform query tests, the mock converter popped up in a lot of places. And since it contains the same information as the mock MetricMetadataAPI, I wrote a function in the mock package that gives you one of each, by asking for a list of TaggedMetrics.

Many tests were switched to this new form, since it was the easiest way to populate the mock MetricConverter.

Raw Timeseries Values

Raw timeseries values are no longer stored in timeseries. They are instead reported in the Evaluation Notes for an execution.

Changes to MetricMetadataAPI

MetricMetadataAPI is renamed to MetricMetadataInterface

MetricMetadataAPI is now an interface which only has the subset of methods used for querying (GetAllMetricForTag, GetAllMetrics)

MetricMetadataModifier is now an interface which has only the subset of methods used for adding/removing metrics.

The evaluation context uses a MetricMetadataAPI, so it can't be used to modify the database.

@drcapulet

drcapulet commented 8 years ago

I definitely find it odd to make the TimeseriesStorageAPI no longer use tagged metrics and instead relies on strings - seems like we're losing a dimension of data - @syamp thoughts?

Nathan-Fenner commented 8 years ago

There might still be a better way of doing things.

Largely I wanted to minimize the duplication involved in conversion between Blueflood (which actually isn't tag-based at all) and the MQE ingestion pipeline (that consumes untagged graphite metrics, converts them to tagged form, then saves them).

It's not too bad right now, but I think that if additional backends are added in the future, maintaining changes in conversion for all of them will be a lot more difficult. Especially making a transition from non-tagged to tagged metrics natively, Blueflood not having to change at all would be convenient.

Nathan-Fenner commented 8 years ago

Closed- alternative implementation may be considered in the future