fix: cloud monitoring incompatibility with opentelemetry/metrics 0.9.0 #110
Conversation
Codecov Report
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
+ Coverage 92.69% 93.88% +1.19%
==========================================
Files 10 10
Lines 260 278 +18
Branches 43 52 +9
==========================================
+ Hits 241 261 +20
+ Misses 19 17 -2
Continue to review full report at Codecov.
|
| @@ -117,11 +117,21 @@ export class MetricExporter implements IMetricExporter { | |||
| } | |||
| } | |||
|
|
|||
| let sendFailed: Error | undefined; | |||
There was a problem hiding this comment.
Could this just be a boolean? I think that might be more readable.
E.g.
let sendFailed = false;
...
catch (err) {
sendFailed = true;
}
| err.message = `Send TimeSeries failed: ${err.message}`; | ||
| sendFailed = err; | ||
| this._logger.error(err.message); |
There was a problem hiding this comment.
I think it might be more readable and less confusing behavior to create a new variable with the modified message to be logged rather than mutating the original error. I think modifying the caught error wouldn't do much unless it is re-thrown. Not a big deal either way, though.
| assert(!result.interval.startTime); | ||
| }); | ||
|
|
||
| it('should throw an error when given a distriution value', () => { |
| if (isDistributionValue(value)) { | ||
| // TODO: Add support for Distribution | ||
| throw new Error('Distributions are not yet supported'); | ||
| } | ||
| if (isHistogramValue(value)) { | ||
| // TODO: Add support for Histogram | ||
| throw new Error('Histograms are not yet supported'); | ||
| } |
There was a problem hiding this comment.
This is inconsistent with the existing code, which uses the valueType rather than the value to distinguish the behavior. I think there are pros and cons to each; using the valueType isn't as type safe as using the value, but it is simpler. I would recommend using valueType for now and perhaps revisiting when distributions/histograms are implemented. Does that make sense to you? Having explicit cases for each with TODOs and specific errors still looks good to me.
There was a problem hiding this comment.
I agree that it would be a better solution to just switch on valueType if valueType could be either int, double, histogram, or distribution. From what I understand however, valueType can only ever be int or double (see link to code). I believe that switching solely on valueType would cause unexpected behaviour when value is a Distribution or Histogram. For example when valueType is INT and value is a Distribution, the output of the function would have the following structure:
{
int64Value: {
min: <int>,
max: <int>,
count: <int>,
sum: <int>,
}
}
which I suspect will cause a problem when the payload gets sent to the timeseries.create api.
Its also possible that I'm misunderstanding how valuetypes and values relate to one another.
There was a problem hiding this comment.
I see, I misunderstood how that works. I was having trouble locating the ValueType enum, so thanks for sending that.
So I think what you have here is good. What I would suggest is making a user-defined type guard for the functions checking the types. E.g.:
function isDistributionValue(value: number | OTDistribution | OTHistogram): value is OTDistribution {
return value.hasOwnProperty('min');
}
Then I think you could safely remove the as number type casts in 180 and 182 since it would be narrowed down to number.
That would also help later working with those distribution and histogram types.
There was a problem hiding this comment.
Great idea! Will do
…he code and fixing typos
This PR upgrades the cloud monitoring exporter's dependency on @opentelemetry/metrics to the latest release (0.9.0).
Changes
The biggest change to @opentelemetry/metrics was the removal of
labelKeysfromMetricOptions. In response, labels are now added to GCP metrics using the metric instead of the metric descriptor as per the GCP DocsFunctions were adjusted to handle the additional
MetricKinds that were addedTesting
Added some additional unit tests; all tests pass.
Tested metric export on a GCE VM running the latest version of node.
Fixes #102