Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Cumulative gauges#626

Merged
c24t merged 13 commits intocensus-instrumentation:masterfrom
c24t:cumulatives
Apr 23, 2019
Merged

Cumulative gauges#626
c24t merged 13 commits intocensus-instrumentation:masterfrom
c24t:cumulatives

Conversation

@c24t
Copy link
Copy Markdown
Member

@c24t c24t commented Apr 22, 2019

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.

class CumulativePointLong(gauge.GaugePointLong):
"""A `GaugePointLong` that cannot decrease."""

def set(self, val):
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

@songy23 songy23 left a 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

:param val: Value to set.
"""
def _set(self, val):
if not isinstance(val, six.integer_types):
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@c24t c24t merged commit 9a07314 into census-instrumentation:master Apr 23, 2019
@c24t c24t deleted the cumulatives branch April 23, 2019 19:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metrics: Add Cumulative APIs

4 participants