Add support for Derived Cumulative API #503
Conversation
|
|
||
| type ValueExtractor = () => number; | ||
|
|
||
| interface CumulativeEntry { |
There was a problem hiding this comment.
Could you add JSDoc comments to this interface and its members?
| /** | ||
| * Constructs a new DerivedCumulative instance. | ||
| * | ||
| * @param {string} name The name of the metric. |
There was a problem hiding this comment.
What would you think about removing all the JSDoc types since those are included in the TypeScript types anyway (and so feel redundant here)?
There was a problem hiding this comment.
Done, have to fix same in other places. I will open new PR to make things consistent everywhere.
| * observed from a obj. The ValueExtractor is invoked whenever | ||
| * metrics are collected, meaning the reported value is up-to-date. | ||
| * | ||
| * @param {LabelValue[]} labelValues The list of the label values. |
There was a problem hiding this comment.
Similar comment here on not including the type in the JsDoc
|
|
||
| /** | ||
| * Creates a TimeSeries. The value of a single point in the TimeSeries is | ||
| * observed from a obj. The ValueExtractor is invoked whenever |
There was a problem hiding this comment.
Change to from an object or function?
| [...cumulativeEntry.labelValues, ...this.constantLabelValues], | ||
| points: [{value, timestamp}], | ||
| startTimestamp: this.startTime | ||
| } as TimeSeries; |
There was a problem hiding this comment.
What would you think about moving the type from here to the arrow function signature?
So it would become ([_, cumulativeEntry]): TimeSeries => {. I believe the as TimeSeries means that the compiler wouldn't report an error if the fields in the structure don't match the type correctly, whereas specifying it as a return type would.
There was a problem hiding this comment.
I believe the as TimeSeries means that the compiler wouldn't report an error if the fields in the structure don't match the type correctly, whereas specifying it as a return type would
Hmm, you are correct. Good catch! Thanks.
|
|
||
| // Checks if the specified collection is a LengthAttributeInterface. | ||
| // tslint:disable-next-line:no-any | ||
| export function isLengthAttributeInterface(obj: any): |
There was a problem hiding this comment.
Can you use unknown here instead of any? (Same for other functions below)
There was a problem hiding this comment.
Good Point, unknown type is available since TypeScript 3.0, we are still using 2.9.0 version for core package. For now, I have added TODO comment: 229a9b7. PTAL once you have time.
There was a problem hiding this comment.
The TODO comment looks great, but I'd suggest just putting it right above this function and then removing the commented out code below it. LGTM to me overall though so approving again!
mayurkale22
force-pushed
the
derived_cumulative
branch
from
f56c2ae
to
2b5441f
Compare
Codecov Report
@@ Coverage Diff @@
## master #503 +/- ##
==========================================
+ Coverage 94.76% 95.14% +0.37%
==========================================
Files 145 147 +2
Lines 10397 10559 +162
Branches 888 897 +9
==========================================
+ Hits 9853 10046 +193
+ Misses 544 513 -31
Continue to review full report at Codecov.
|
mayurkale22
force-pushed
the
derived_cumulative
branch
from
565b325
to
d42e803
Compare
mayurkale22
force-pushed
the
derived_cumulative
branch
from
229a9b7
to
306ee15
Compare
|
|
||
| // Checks if the specified collection is a LengthAttributeInterface. | ||
| // tslint:disable-next-line:no-any | ||
| export function isLengthAttributeInterface(obj: any): |
There was a problem hiding this comment.
The TODO comment looks great, but I'd suggest just putting it right above this function and then removing the commented out code below it. LGTM to me overall though so approving again!
|
Thanks @draffensperger for the reviews. |
Updates #450
Specs: https://github.com/census-instrumentation/opencensus-specs/pull/254/files?short_path=0aaf645#diff-0aaf6455b2b0b7a5f2ed45f6b36792ee
This is mostly same as derived-gauge.
Last piece, registering with Metric-Registry will be in a follow-up PR.