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

Add support for Derived Cumulative API #503

Merged

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented May 7, 2019

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

LGTM with a variety of mostly nit-picky changes.


type ValueExtractor = () => number;

interface CumulativeEntry {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add JSDoc comments to this interface and its members?

/**
* Constructs a new DerivedCumulative instance.
*
* @param {string} name The name of the metric.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think about removing all the JSDoc types since those are included in the TypeScript types anyway (and so feel redundant here)?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to from an object or function?

[...cumulativeEntry.labelValues, ...this.constantLabelValues],
points: [{value, timestamp}],
startTimestamp: this.startTime
} as TimeSeries;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use unknown here instead of any? (Same for other functions below)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 mayurkale22 force-pushed the derived_cumulative branch from f56c2ae to 2b5441f Compare May 8, 2019 16:59
@codecov-io
Copy link

codecov-io commented May 8, 2019

Codecov Report

Merging #503 into master will increase coverage by 0.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/detect-resource.ts 92.85% <0%> (-7.15%) ⬇️
src/stackdriver-monitoring.ts 78.65% <0%> (-3.38%) ⬇️
src/common-utils.ts 93.75% <0%> (-2.92%) ⬇️
src/metrics/gauges/derived-gauge.ts 95.91% <0%> (-0.81%) ⬇️
test/test-detect-resource.ts 99.51% <0%> (-0.49%) ⬇️
src/index.ts 100% <0%> (ø) ⬆️
src/binary-format.ts 100% <0%> (ø) ⬆️
src/grpc-stats/server-stats.ts 100% <0%> (ø) ⬆️
src/resource-labels.ts 100% <0%> (ø) ⬆️
src/grpc-stats/client-stats.ts 100% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 111223e...0fbb5fd. Read the comment docs.

@mayurkale22 mayurkale22 force-pushed the derived_cumulative branch from 229a9b7 to 306ee15 Compare May 9, 2019 18:25

// Checks if the specified collection is a LengthAttributeInterface.
// tslint:disable-next-line:no-any
export function isLengthAttributeInterface(obj: any):
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Thanks @draffensperger for the reviews.

@mayurkale22 mayurkale22 merged commit 89f20bb into census-instrumentation:master May 9, 2019
@mayurkale22 mayurkale22 deleted the derived_cumulative branch May 9, 2019 20:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants