Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

Metrics: Add Cumulative APIs. #1848

Merged
merged 6 commits into from
Apr 19, 2019

Conversation

songy23
Copy link
Contributor

@songy23 songy23 commented Apr 18, 2019

Updates #1815.

Although the Spec PR (census-instrumentation/opencensus-specs#254) hasn't been submitted, Cumulative APIs have already been added to Go and Node. This PR adds the Cumulative APIs in Java similar to Go (census-instrumentation/opencensus-go#1090).

Most are a copy-and-paste from Gauge APIs since they are very similar.

Copy link
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

few nits related to since annotations.

LGTM otherwise.


/**
* Derived Double Cumulative metric, to report cumulative measurement of a double value. Cumulative
* values can go up or stay the same, but can never go down. The cumulative values cannot be
Copy link
Contributor

Choose a reason for hiding this comment

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

"The cumulative values" -> "Cumulative values" - similarly below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -159,6 +159,64 @@ public DerivedDoubleGauge addDerivedDoubleGauge(
@ExperimentalApi
public abstract DerivedDoubleGauge addDerivedDoubleGauge(String name, MetricOptions options);

/**
* Builds a new long cumulative to be added to the registry. This is more convenient form when you
* want to manually increase values as per your service requirements.
Copy link
Contributor

Choose a reason for hiding this comment

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

"This is more convenient form" -> "This form is more convenient" or "This is a more convenient form" - here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG, updated.

* @since 0.21
*/
@ExperimentalApi
public abstract LongCumulative addLongCumulative(String name, MetricOptions options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the name add.. - seems like it's only building something not adding it to anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's both building a new *Cumulative object and adding it to the MetricRegistry so that all metric readers can read it from the MetricRegistry.

@songy23
Copy link
Contributor Author

songy23 commented Apr 19, 2019

Thanks! Implementation will be in a follow-up PR.

@songy23 songy23 merged commit 8a86284 into census-instrumentation:master Apr 19, 2019
@songy23 songy23 deleted the cumulative-api branch April 19, 2019 05:24
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.

4 participants