-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cumulative gauges #626
Cumulative gauges #626
Conversation
class CumulativePointLong(gauge.GaugePointLong): | ||
"""A `GaugePointLong` that cannot decrease.""" | ||
|
||
def set(self, val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a set
method for Cumulative. See census-instrumentation/opencensus-java#1853.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
21ad4b2 leaves the class hierarchy in place, but means DerivedGaugePoint
now calls the hidden _set
method. Another option is to flatten these classes out -- have separate classes for derived gauges and cumulatives and duplicate some code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
21ad4b2 looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except minor nits
:type val: int | ||
:param val: Value to set. | ||
""" | ||
def _set(self, val): | ||
if not isinstance(val, six.integer_types): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or consider just raising an error saying set
is not supported with cumulative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still have to split this up since the derived point wrapper is calling set
on both, but I may be missing you point here. In any case I think it's fine as-is, and we can refactor this if gauges and cumulatives drift further apart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
This PR adds support for cumulative gauges, i.e. gauges that can't decrease.
See PRs for the same feature in the go and java clients.
See previous gauge PRs for some context on the mixin black magic we're using in this module: #494, #498, #503.
Fixes #586.