Drop negative measurements on record#389
Drop negative measurements on record#389c24t merged 13 commits intocensus-instrumentation:masterfrom
Conversation
and a few incidental lint errors.
and update tests.
at record time.
| for ii, bound in enumerate(agg_data.bounds): | ||
| cum_count += agg_data.counts_per_bucket[ii] | ||
| points[str(bound)] = cum_count | ||
| labels = desc['labels'] if points is None else None |
There was a problem hiding this comment.
points is never null here, but I didn't touch it because I don't know what prometheus expects. @mayurkale22 let me know if labels should be null when there are no points in the distribution and I'll change it.
|
On a side note, I think we should add CHANGELOG.md now, to document all the notable changes about the versions. Same as: https://github.com/census-instrumentation/opencensus-java/blob/master/CHANGELOG.md. This PR could be a breaking change if users are recording negative value for any measure. What do you think? |
and out of DistributionAggregationData.
I think adding a changelog is a great idea, and we should push a version update once these changes are merged. We have some work to do to check that the code added since the last release is ready to ship though. |
| read from current execution context. | ||
| """ | ||
| if tag_map_tags is None: | ||
| tag_map_tags = execution_context.get_current_tag_map() |
There was a problem hiding this comment.
Unrelated to this PR, I'm assuming we want to get this at call time and not module load time.
| "non-negative") | ||
| logger.info("Measure '{}' has negative value ({}), refusing " | ||
| "to record measurements from {}" | ||
| .format(measure.name, value, self)) |
There was a problem hiding this comment.
Not that we're doing it other places in the library, but I think it's a good idea to include more context at a more verbose logging level so users can turn up specific loggers to see what's going wrong in detail. Reviewers: let me know what you think.
| """associates the measure of type Int with the given value""" | ||
| if value < 0: | ||
| # Should be an error in a later release. | ||
| logger.warning("Cannot record negative values") |
There was a problem hiding this comment.
Note that we're breaking with the java impl here in that we don't mark the map invalid until record time.
| return | ||
| for measure, value in self.measurement_map.items(): | ||
| if value < 0: | ||
| self._invalid = True |
There was a problem hiding this comment.
I would set this in put method, no need to iterate on map here.
There was a problem hiding this comment.
I don't have a strong preference, but this is intentional since I don't think it makes sense to raise until the user tries to record.
There was a problem hiding this comment.
I don't think it makes sense to raise until the user tries to record.
Agreed. But my point was if you set self._invalid = True in the put method, during record time you just have to check if (self._invalid == True) instead of iterating on map.
There was a problem hiding this comment.
Got it, but this would mean marking the map invalid if the user puts the same measure twice before recording, first with a negative value and second positive. Obviously they shouldn't do this, but there's no reason in principle to invalidate the map if they do. It also lets us log the particular negative value when record is called (vs. doing it earlier in put).
| elif isinstance(agg_data, | ||
| aggregation_data_module.DistributionAggregationData): | ||
|
|
||
| assert(agg_data.bounds == sorted(agg_data.bounds)) |
There was a problem hiding this comment.
We should do same in the stats/stackdriver exporter?
There was a problem hiding this comment.
Here? I don't think we need this assertion everywhere we use distribution aggregations, I've only got it in the prometheus exporter since guaranteeing the bounds were sorted let me simplify the code a bit.
|
Something to consider for later when we're making other breaking changes: we might want to remove the |
| if not all(boundaries[ii] < boundaries[ii + 1] | ||
| for ii in range(len(boundaries) - 1)): | ||
| raise ValueError("bounds must be sorted in increasing order") | ||
| for ii, bb in enumerate(boundaries): |
There was a problem hiding this comment.
We could do the above check during this loop too, and then we only have to iterate through the list of boundaries once?
There was a problem hiding this comment.
This loop should normally break on the first item, I think the slight readability improvement is worth it here.
| "length") | ||
| else: | ||
| assert all(cc >= 0 for cc in counts_per_bucket) | ||
| assert len(counts_per_bucket) == len(bounds) + 1 |
There was a problem hiding this comment.
Asserts aren't guaranteed to run in production, since some people might run Python without assertions, but we always raise ValueErrors or AssertionErrors for these, I think.
There was a problem hiding this comment.
Sorry, I should have elaborated. Caveat #1 from this blog post mentions it. The assert docs also mention it, but not necessarily emphasising how they shouldn't be used.
There was a problem hiding this comment.
I tried to use asserts in a way consistent with the blog. An assertion failure here should mean a programming error since we only ever expect to call DistributionAggregationData.__init__ from DistributionAggregation.__init__, where we actually do raise a ValueError if the user gives us unsorted bounds.
I.e. precondition checks for API methods should raise Type/ValueErrors (and etc.), but precondition checks in internal-only code can assert instead since bad user input should never cause them to fail. The logic doesn't change if the assertions are removed, and they should only fail if there's a bug in the library.
That said, there's no bright line separating the API from the internals here, and we don't lose anything by changing these to ValueErrors (other than signaling the difference between expected and unexpected errors). If you've got a strong preference I don't mind changing it.
mayurkale22
left a comment
mayurkale22
left a comment
There was a problem hiding this comment.
LGTM. Next PR should be -> Exporter/Stats/Stackdriver: Add 0 count for underflow/first bucket
4 participants
This diff prevents sets of measurements from being recorded to view data objects if any measurement in the set is negative, prevents
MeasurementMapsfrom being recorded if they've ever contained negative value measurements, and removes 0- and negative-valued bucket boundaries fromDistributionAggregations.See the java implementation for the justification for these changes.
Fixes #376