Fix responses with bounds#268
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical bug in the bounds handling logic for metric samples. The issue occurred when bound values were exactly zero - they were incorrectly skipped due to Python's truthiness evaluation. The fix uses protobuf's HasField() method to properly distinguish between unset optional fields and fields with zero values, and ensures that both upper and lower bounds are always returned (using infinity values when not set).
Changes:
- Fixed bug where zero-valued bounds were incorrectly ignored due to falsy evaluation
- Ensured both upper and lower bounds are always yielded as pairs, using infinity defaults for unset values
- Changed from conditional yielding based on truthiness to proper protobuf field presence checking
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lower = bound.lower if bound.HasField("lower") else -math.inf | ||
| yield MetricSample( | ||
| ts, mid, cid, f"{met}_bound_{i}_lower", lower | ||
| ) | ||
| upper = bound.upper if bound.HasField("upper") else math.inf | ||
| yield MetricSample( | ||
| ts, mid, cid, f"{met}_bound_{i}_upper", upper | ||
| ) |
There was a problem hiding this comment.
The bounds handling logic has been updated to fix the bug where zero values were ignored, but there are no tests covering this functionality. Consider adding tests that verify:
- Bounds with zero values are properly returned
- Missing bounds return infinity values as expected
- Both upper and lower bounds are always yielded as pairs
This is important to prevent regression of the bug that was just fixed.
Fixes a bug where bounds of zero were ignored. Also ensures that we always return a pair of upper and lower bounds, where +/-inf is returned in case the bound is not set. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com>
3 participants
Fixes a bug where bounds of zero were ignored. Also ensures that we always return a pair of upper and lower bounds, where +/-inf is returned in case the bound is not set.