Metrics point conversion#467
Conversation
instead of underlying primitives.
|
|
||
| # If there is no histogram, do not record an exemplar | ||
| self._exemplars = \ | ||
| {bucket: exemplars} if len(self._bounds) > 0 else None |
There was a problem hiding this comment.
I'm assuming this is a typo and this line should have been indented. Score one for semantic whitespace detractors.
| self.assertEqual(sum_of_sqd_deviations, | ||
| dist_agg_data.sum_of_sqd_deviations) | ||
| self.assertEqual([1, 1, 1], dist_agg_data.counts_per_bucket) | ||
| self.assertEqual([exemplar_1, exemplar_2], dist_agg_data.exemplars[2]) |
There was a problem hiding this comment.
This was asserting the wrong behavior: attaching all exemplars to the last bucket.
| cls.CUMULATIVE_DOUBLE: float, | ||
| cls.CUMULATIVE_DISTRIBUTION: ValueDistribution, | ||
| cls.SUMMARY: ValueSummary | ||
| cls.GAUGE_INT64: value.ValueLong, |
There was a problem hiding this comment.
I have a general question, would this hurt the performance as we need to build the dictionary each time the function gets called (and optimization doesn't work since cls is parameterized)?
There was a problem hiding this comment.
That's a good point. I assumed it was cheap enough to do that I didn't bother trying to optimize it, but this will get called on every stat conversion and there's no reason not to move this up into the class. Changed in 7a43a50.
There was a problem hiding this comment.
It'd be interesting to profile the library on an example app to see what other easy improvements we could make.
There was a problem hiding this comment.
Good point. We should have some level of performance test in the CI.
opencensus/stats/aggregation_data.py
Outdated
| :param timestamp: The time to report the point as having been recorded. | ||
|
|
||
| :rtype: :class: `opencensus.metrics.export.point.Point` | ||
| :return: a :class: `opencensus.metrics.export.value.ValueLong`-valued |
There was a problem hiding this comment.
Minor nit -> s/ValueLong/ValueDouble
| self.assertEqual(converted_point.value.sum, 4950) | ||
| self.assertEqual(converted_point.value.sum_of_squared_deviation, | ||
| 80850.0) | ||
| self.assertEqual([bb.count for bb in converted_point.value.buckets], |
There was a problem hiding this comment.
Can you please add check for bucket_options?
4 participants
This PR adds
to_pointfunctions to each stats aggregation data class and fixes an unrelated bug inDistributionAggregationDataexemplars. This lays the groundwork forMetricProducers, which will let us convert stats data into metrics data models for export.Addresses #335.