Support int typed aggregations by colincadams · Pull Request #696 · census-instrumentation/opencensus-python

colincadams · 2019-06-27T02:58:22Z

This makes Aggregation a factory of AggregationData, using the type of the underlying measure to determine which value type to use.

fixes #565

Currently just a first pass, looking for feedback on the approach that we started discussing in #565.

reyang · 2019-06-27T04:50:58Z

@lzchen please help to review.

songy23

Looks good overall, please fix lint.

opencensus/stats/aggregation.py

lzchen · 2019-06-28T05:24:56Z

opencensus/stats/aggregation.py

+    def __init__(self, buckets=None):
        self._buckets = buckets or []

    @property


You still have an aggregation_type property. As well remove the param comment.

I ended up removing BaseAggregation altogether, per @c24t's comments.

lzchen · 2019-06-28T05:28:00Z

opencensus/stats/aggregation.py

@@ -78,16 +61,24 @@ class SumAggregation(BaseAggregation):
    :param aggregation_type: represents the type of this aggregation


IMO, I would include a get_metric_type(measure) method in BaseAggregation with no implementation to let developers know that aggregations should implement this (there might be new kinds of aggregations)

Same as above.

lzchen · 2019-06-28T05:31:52Z

opencensus/stats/aggregation.py

@@ -101,16 +92,16 @@ class CountAggregation(BaseAggregation):
    :param aggregation_type: represents the type of this aggregation


Fix param comments

lzchen · 2019-06-28T05:33:53Z

opencensus/stats/aggregation_data.py

+        :class:`opencensus.metrics.export.value.ValueDouble` or
+        :class:`opencensus.metrics.export.value.ValueLong`
+    :param value_type: the type of value to be used when creating a point
+    :type sum_data: meh


loll 🤸‍♂

lzchen · 2019-06-28T05:34:50Z

opencensus/stats/aggregation.py

@@ -78,16 +61,24 @@ class SumAggregation(BaseAggregation):
    :param aggregation_type: represents the type of this aggregation


Nit: Fix "Sum Aggregation DESCRIBES" in comments

lzchen · 2019-06-28T05:49:54Z

This is great! The factory design is very slick. Just a couple of comments and should be good to go!

c24t

This is a huge improvement, great that you were able to remove aggregation.aggregation_data and aggregation.aggregation_type and get_metric_type.

Besides @lzchen's comments I'd ask that you also remove BaseAggregation, or at least move buckets down to the distribution class.

c24t · 2019-07-02T00:40:37Z

opencensus/stats/aggregation.py

    """
-    def __init__(self, buckets=None, aggregation_type=Type.NONE):
-        self._aggregation_type = aggregation_type
+    def __init__(self, buckets=None):


While you're in here: I don't see any reason for buckets to be on BaseAggregation, it looks like an odd choice from the first implementation (#149) that never got cleaned up. Since aggregation_type is gone now I recommend scrapping this whole class.

Done, just removed the class.

c24t · 2019-07-02T01:06:52Z

opencensus/stats/aggregation_data.py



-class SumAggregationDataFloat(BaseAggregationData):
+class SumAggregationData(BaseAggregationData):


Why the decision to make value_type an instance attr instead of keeping separate aggregation classes for each value type here?

Initially, I had this private class with value_type and then bound classes like:

SumAggregationDataFloat = functools.partial(SumAggregationData, value_type=value.ValueDouble) SumAggregationDataInt = functools.partial(SumAggregationData, value_type=value.ValueInt)

But then in SumAggregation.new_aggregation_data, we needed an additional set of conditionals to check the type of the measure or metric and decide which to use. Instead doing it this way, we just need the one set of conditionals in .get_metric_type which we need anyway, and can reuse MetricDescriptor.to_type_class. It seemed a little cleaner, but really it isn't that different.

We could also duplicate the classes entirely, instead of the functools.partial, but that seemed unnecessary.

I guess the other option is to force the user to decide and provide Int or Float, but since we can determine it that seems easier and less error prone.

lzchen · 2019-07-02T20:54:19Z

Also fixes [#679]

colincadams · 2019-07-12T01:03:09Z

opencensus/stats/aggregation_data.py

@@ -52,16 +52,21 @@ def to_point(self, timestamp):
        raise NotImplementedError  # pragma: NO COVER




The BaseAggregationData class seems a little odd to me as well, each implementation supplies aggregation_data upon construction but then doesn't update it, and stores there data in a separate instance variable that they update and return.

Is there a reason for this class to exist?

Not that I can tell, besides to show that aggregations should implement to_point, but in that case there should be an unimplemented add_sample in the class too.

In any case it looks like you can lose the aggregation_data attr and property.

I just removed BaseAggregationData entirely

reyang · 2019-07-12T01:12:34Z

opencensus/stats/aggregation.py

+        """Get the MetricDescriptorType for the metric produced by this
+        aggregation and measure.
+        """
+        if isinstance(measure, measure_module.MeasureInt):


I don't have strong opinion here, it seems a more "Pythonic" style is:

if isinstance(measure, measure_module.MeasureInt): return MetricDescriptorType.CUMULATIVE_INT64 if isinstance(measure, measure_module.MeasureFloat): return MetricDescriptorType.CUMULATIVE_DOUBLE raise ValueError

Up to you.

Good catch, surprised the linter didn't yell about this (or maybe it was a warning I didn't see 😁)

c24t

LGTM after removing the attr from BaseAggregationData (or the whole class). Please also add a note to the changelog.

reyang

This is a big improvement, thank you @colincadams!

colincadams · 2019-07-17T23:40:27Z

Thanks all! And thanks for updating the changelog @c24t, totally forgot!

colincadams requested review from a team, c24t, reyang and songy23 as code owners June 27, 2019 02:58

googlebot added the cla: yes label Jun 27, 2019

colincadams added 4 commits June 26, 2019 22:58

add sum and lastvalue aggregations for ints

3a6263d

remove type, etc.

44ce81d

start fixing tests

5289909

more test fixes

6659817

colincadams force-pushed the aggregate-ints branch 2 times, most recently from 5c78be8 to 7e2474f Compare June 27, 2019 03:08

finish fixing tests

1681a35

colincadams force-pushed the aggregate-ints branch from 7e2474f to 1681a35 Compare June 27, 2019 03:15

songy23 reviewed Jun 27, 2019

View reviewed changes

opencensus/stats/aggregation.py Outdated Show resolved Hide resolved

colincadams added 2 commits June 27, 2019 19:30

remove unnecessary todo

f7668cc

fix lint

f89bea5

lzchen reviewed Jun 28, 2019

View reviewed changes

c24t reviewed Jul 2, 2019

View reviewed changes

colincadams added 5 commits July 11, 2019 20:10

Address comments and fix docstrings

eb199aa

Merge branch 'master' into aggregate-ints

207f980

fix

f98ddc1

fix aggregation classes based off object

e890189

fix tests

f62746f

colincadams commented Jul 12, 2019

View reviewed changes

colincadams added 2 commits July 11, 2019 21:05

lint

b27a8ee

revert noxfile

22e8bc3

reyang reviewed Jul 12, 2019

View reviewed changes

fix small issue

25fb25c

c24t approved these changes Jul 12, 2019

View reviewed changes

colincadams added 2 commits July 14, 2019 16:47

remove BaseAggregationData

8f9b313

Merge branch 'master' into aggregate-ints

71994eb

reyang approved these changes Jul 17, 2019

View reviewed changes

c24t added 2 commits July 17, 2019 13:28

Merge branch 'master' into aggregate-ints

7e0706a

Update changelog

7549e50

c24t merged commit 0d00458 into census-instrumentation:master Jul 17, 2019

c24t mentioned this pull request Jul 17, 2019

Invalid point value type error. MeasureInts being silently converted to ValueDouble #679

Closed

colincadams deleted the aggregate-ints branch July 17, 2019 23:39

micolous mentioned this pull request Oct 26, 2019

Exporter/Prometheus: broken for SumAggregation and LastValueAggregation on 0.2.1 with OpenCensus 0.7.0 and later #807

Open

		@@ -78,16 +61,24 @@ class SumAggregation(BaseAggregation):
		:param aggregation_type: represents the type of this aggregation

		@@ -101,16 +92,16 @@ class CountAggregation(BaseAggregation):
		:param aggregation_type: represents the type of this aggregation



		class SumAggregationDataFloat(BaseAggregationData):
		class SumAggregationData(BaseAggregationData):

		@@ -52,16 +52,21 @@ def to_point(self, timestamp):
		raise NotImplementedError # pragma: NO COVER

Conversation

colincadams commented Jun 27, 2019

Uh oh!

reyang commented Jun 27, 2019

Uh oh!

songy23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lzchen Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen commented Jun 28, 2019

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen commented Jul 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

colincadams commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lzchen Jun 28, 2019 •

edited

Loading

lzchen Jun 28, 2019 •

edited

Loading

lzchen Jun 28, 2019 •

edited

Loading