DistributionAggregation fixes#375
Conversation
c24t
left a comment
c24t
left a comment
There was a problem hiding this comment.
It's possible there are still bugs in this and the other aggregation classes, these are just the ones I found on the way to converting DistributionAggregation into a metric.
| self._distribution = distribution or {} | ||
| self.aggregation_data = aggregation_data.DistributionAggregationData( | ||
| 0, 0, 0, 0, 0, None, boundaries) | ||
| 0, 0, float('inf'), float('-inf'), 0, None, boundaries) |
There was a problem hiding this comment.
Changed to match the java client's behavior, this previously failed on e.g. negative value points.
| self.assertEqual(aggregation_module.Type.DISTRIBUTION, | ||
| distribution_aggregation.aggregation_type) | ||
|
|
||
| def test_min_max(self): |
There was a problem hiding this comment.
Again, all lint and formatting except this one.
| self.assertEqual(1.4, dist_agg_data.mean_data) | ||
| self.assertEqual(5.2, dist_agg_data.sum_of_sqd_deviations) | ||
| self.assertIsNot(0, dist_agg_data.count_data) | ||
| self.assertEqual(3, dist_agg_data.exemplars[5].value) |
There was a problem hiding this comment.
This was actually asserting the buggy behavior.
| else: | ||
| last_bucket_index = len(self._bounds) | ||
| self._counts_per_bucket[last_bucket_index] += 1 | ||
| return last_bucket_index |
There was a problem hiding this comment.
One fix here and the other up in __init__, the other changes in this file are lint and formatting.
|
Please fix the lint. opencensus/stats/aggregation_data.py:221:5: E303 too many blank lines (2)
nox > Command flake8 --exclude=opencensus/trace/exporters/gen/opencensus/ opencensus/ failed with exit code 1
nox > Session lint failed. :( |
| columns=view_columns, | ||
| measure=view_measure, | ||
| aggregation=view_aggregation) | ||
| View(name=view_name, |
There was a problem hiding this comment.
Why is this created but never used?
There was a problem hiding this comment.
I didn't start the fire here, I just removed the unused view var to fix the F841 lint error, and same for the changes below.
Many of the tests in this module seem half-baked, but I think this is a problem for another PR.
| view_measure = measure | ||
| view_aggregation = mock.Mock() | ||
| view = View( | ||
| View( |
| view_measure = measure | ||
| view_aggregation = mock.Mock() | ||
| view = View( | ||
| View( |
| counts_per_bucket.append(0) | ||
| counts_per_bucket = [0 for ii in range(len(bounds) + 1)] | ||
| elif len(counts_per_bucket) != len(bounds) + 1: | ||
| raise ValueError("counts_per_bucket length does not match bounds " |
There was a problem hiding this comment.
I think we can change to "counts_per_bucket has X fields, which is more than the Y buckets allowed." where, X:len(counts_per_bucket) and Y: len(bounds) + 1. What do you think?
There was a problem hiding this comment.
One benefit of fixed error messages is that you can grep for the exact text, but I'm happy with either here.
4 participants
This diff addresses a few bugs in
DistributionAggregationandDistributionAggregationDatafrom #373 that needed to be fixed before converting these aggregations to metrics for #335.This is a noisy diff since I formatted and fixed old lint errors in the files I touched. I'll comment on the important changes.
Fixes #373.