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

Adds interfaces for [Metrics api]#188

Merged
isaikevych merged 4 commits intocensus-instrumentation:masterfrom
isaikevych:metrics
Nov 14, 2018
Merged

Adds interfaces for [Metrics api]#188
isaikevych merged 4 commits intocensus-instrumentation:masterfrom
isaikevych:metrics

Conversation

@isaikevych
Copy link
Copy Markdown
Contributor

@isaikevych isaikevych commented Nov 13, 2018

Fixes #133

Comment thread packages/opencensus-core/src/metrics/export/types.ts Outdated
Comment thread packages/opencensus-core/src/metrics/export/types.ts Outdated
Comment thread packages/opencensus-core/src/metrics/export/types.ts
Comment thread packages/opencensus-core/src/metrics/export/types.ts Outdated
Comment thread packages/opencensus-core/src/metrics/export/types.ts Outdated
/** Properties of a TimeSeries. */
export interface ITimeSeries {
/** TimeSeries startTimestamp */
readonly startTimestamp: (Timestamp|null);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be (Timestamp|null) -> (google.protobuf.Timestamp|null), @songy23 Any thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mayurkale22 as you mentioned here issue 133 we cant use proto files, as an option I can create namespace google -> protobuf -> ...

readonly value: (number|null);
}

export interface Timestamp {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like we don't need this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The current interfaces uses this

@mayurkale22
Copy link
Copy Markdown
Member

Please fix the build

* In case of a streaming RPC this can be sent for metrics that already
* sent the MetricDescriptor once.
*/
readonly name: string;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Metric contains only MetricDescriptor and TimeSeries. Please remove readonly name: string;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It has name: metrics.proto #L46

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

That's the name for MetricDescriptor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

}

/** A timestamped measurement. */
export interface Point {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The Point should have only timestamp and Value.

Refer https://github.com/census-instrumentation/opencensus-go/pull/969/files#diff-d6cca479b1a793ab4e0a1a3f22549f84R24 or Java/Python implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

* (string) In case of a streaming RPC this can be sent for metrics
* that already sent the MetricDescriptor once.
*/
readonly descriptor: MetricDescriptor|string;
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.

string is only for saving encoded bytes, in the API we should always use MetricDescriptor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

* In case of a streaming RPC this can be sent for metrics that already
* sent the MetricDescriptor once.
*/
readonly name: string;
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.

That's the name for MetricDescriptor.

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. Please fix the CI.

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.

5 participants