Add helper util to convert OC Metrics data models to SD monitoring data models #266
Conversation
| @@ -103,7 +103,7 @@ export interface Distribution { | |||
| } | |||
|
|
|||
| export interface Point { | |||
| interval: {endTime: string, startTime: string}; | |||
| interval: {endTime: string, startTime?: string}; | |||
There was a problem hiding this comment.
startTime should be optional (Must be present for cumulative metrics only). More info: https://cloud-dot-devsite.googleplex.com/monitoring/api/ref_v3/rest/v3/TimeInterval
| @@ -115,8 +115,8 @@ export interface Point { | |||
|
|
|||
| export interface TimeSeries { | |||
| metric: {type: string; labels: {[key: string]: string};}; | |||
| resource: {type: 'global', labels: {[key: string]: string}}; | |||
| resource: {type: string, labels: {[key: string]: string}}; | |||
There was a problem hiding this comment.
In the future, we have planned to add more type like k8s_container, gce_instance and aws_ec2_instance.
| timeSeriesList.push({ | ||
| metric: this.createMetric( | ||
| metricDescriptor, timeSeries.labelValues, metricPrefix), | ||
| resource: monitoredResource, |
There was a problem hiding this comment.
Just to double check: it's fine to use the same monitoredResource here (i.e no need to convert)?
There was a problem hiding this comment.
Currently no, but once we have monitored resource util in place (#173) we need to do the conversion same as Java's implementation.
| * monitoring data models. | ||
| */ | ||
| export class StackdriverStatsExporterUtils { | ||
| static readonly OPENCENSUS_TASK: string = 'opencensus_task'; |
There was a problem hiding this comment.
Why are these constants and methods static within a class? While that is needed in Java, IMO it's just extra wrapping for TypeScript since the module itself already acts as a namespace. What would you think about just exporting the functions and constants directly?
| valueType: this.createValueType(metricDescriptor.type), | ||
| unit: metricDescriptor.unit, | ||
| labels: this.createLabelDescriptor(metricDescriptor.labelKeys) | ||
| } as MetricDescriptor; |
There was a problem hiding this comment.
Could you remove the as MetricDescriptor? The TS compiler should know the object must match the type based on the method return type.
| const metricKind = this.createMetricKind(metricDescriptor.type); | ||
| const valueType = this.createValueType(metricDescriptor.type); | ||
|
|
||
| for (const timeSeries of metric.timeseries) { |
There was a problem hiding this comment.
Optional: could we use map here instead of a for loop + push?
| */ | ||
| static createLabelDescriptor(labelKeys: LabelKey[]): LabelDescriptor[] { | ||
| const labelDescriptorList = labelKeys.map(labelKey => { | ||
| return { |
There was a problem hiding this comment.
Optional, you can avoid the return if you put the object in paratheses, so labelKeys.map(labelKey => ({ key: ...}))
| key: labelKey.key, | ||
| valueType: 'STRING', // Now we only support String type. | ||
| description: labelKey.description | ||
| } as LabelDescriptor; |
There was a problem hiding this comment.
Could you use a type annotation on labelDescriptorList, i.e. const labelDesceriptorList: LabelDescriptor[] = rather than a type assertion as LabelDescriptor here? The type annotation will cause the code to fail to compile if the interface changes so would help detect refactoring bugs.
| */ | ||
| static createMetric( | ||
| metricDescriptor: OCMetricDescriptor, labelValues: LabelValue[], | ||
| metricPrefix: string): {type: string; labels: {[key: string]: string};} { |
There was a problem hiding this comment.
The {type: string; labels: {[key: string]: string};} type looks fairly complex, what would you think about making it a first class exported type alias?
| static createPoint( | ||
| point: TimeSeriesPoint, startTimeStamp: Timestamp, | ||
| valueType: ValueType): Point { | ||
| let value; |
There was a problem hiding this comment.
Optional: you can avoid use of let here by creating a separate function like createValue or similar that would take in the value type and value and return the value, then just have const value = createValue(valueType, point)
| this.createDistribution(point.value as DistributionValue) | ||
| }; | ||
| } else { | ||
| // console.log(`${valueType} is not supported.`); |
There was a problem hiding this comment.
Could you remove this commented code and else black?
| return str.replace('000Z', `${nsStr}Z`); | ||
| } | ||
|
|
||
| private static padNS(ns: number) { |
There was a problem hiding this comment.
Could you make this function name a bit more descriptive? It seems like it's left padding a number, but I wouldn't know that without reading the code.
mayurkale22
force-pushed
the
stackdriver-stats-utils
branch
3 times, most recently
from
de638fb to
eeaf2a2
Compare
|
@draffensperger thanks for the reviews. I have addressed all the comments, PTAL. |
|
|
||
| /** Returns a task label value in the format of 'nodejs-<pid>@<hostname>'. */ | ||
| function generateDefaultTaskValue(): string { | ||
| console.log('inside generateDefaultTaskValue'); |
mayurkale22
force-pushed
the
stackdriver-stats-utils
branch
from
09a70f3 to
4c4a190
Compare
Codecov Report
@@ Coverage Diff @@
## master #266 +/- ##
========================================
+ Coverage 91.5% 94.8% +3.3%
========================================
Files 108 110 +2
Lines 7520 7779 +259
Branches 692 713 +21
========================================
+ Hits 6881 7375 +494
+ Misses 639 404 -235
Continue to review full report at Codecov.
|
This PR is part of #257, Basically,
StackdriverStatsExporterUtilsmethods will be used byStackdriverStatsExporterto translate data from OpenCensus data models to StackDriver monitoring data models. I will send separate PR to include this util inStackdriverStatsExporter.