Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for OT ValueRecorder #130

Closed
wants to merge 8 commits into from
Closed

feat: add support for OT ValueRecorder #130

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6487e5d
Adding support and updating tests
TigerHe7 Jul 28, 2020
896dcf2
fix lint
TigerHe7 Jul 28, 2020
e75daab
gix lint
TigerHe7 Jul 28, 2020
c12d715
Merge github.com:GoogleCloudPlatform/opentelemetry-operations-js into…
TigerHe7 Aug 7, 2020
cd251b9
add test for second conditional branch
TigerHe7 Aug 11, 2020
63f8488
fix lint
TigerHe7 Aug 11, 2020
b3a6b44
add tests to fix code cov
TigerHe7 Aug 11, 2020
b1e52cb
fix lint
TigerHe7 Aug 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,7 @@ export class MetricExporter implements IMetricExporter {
this._logger.debug('Google Cloud Monitoring export');
const timeSeries: TimeSeries[] = [];
for (const metric of metrics) {
const isRegistered = await this._registerMetricDescriptor(
metric.descriptor
);
const isRegistered = await this._registerMetricDescriptor(metric);
if (isRegistered) {
timeSeries.push(
createTimeSeries(
Expand Down Expand Up @@ -149,9 +147,8 @@ export class MetricExporter implements IMetricExporter {
* registered. Returns false otherwise.
* @param metricDescriptor The OpenTelemetry MetricDescriptor.
*/
private async _registerMetricDescriptor(
metricDescriptor: OTMetricDescriptor
) {
private async _registerMetricDescriptor(metric: MetricRecord) {
const metricDescriptor = metric.descriptor;
const existingMetricDescriptor = this.registeredMetricDescriptors.get(
metricDescriptor.name
);
Expand All @@ -167,7 +164,7 @@ export class MetricExporter implements IMetricExporter {
return false;
}
}
const isRegistered = await this._createMetricDescriptor(metricDescriptor)
const isRegistered = await this._createMetricDescriptor(metric)
.then(() => {
this.registeredMetricDescriptors.set(
metricDescriptor.name,
Expand All @@ -186,14 +183,15 @@ export class MetricExporter implements IMetricExporter {
* Creates a new metric descriptor.
* @param metricDescriptor The OpenTelemetry MetricDescriptor.
*/
private async _createMetricDescriptor(metricDescriptor: OTMetricDescriptor) {
private async _createMetricDescriptor(metric: MetricRecord) {
const authClient = await this._authorize();
const request = {
name: `projects/${this._projectId}`,
resource: transformMetricDescriptor(
metricDescriptor,
metric.descriptor,
this._metricPrefix,
this._displayNamePrefix
this._displayNamePrefix,
metric.aggregator.toPoint()
),
auth: authClient,
};
Expand Down
21 changes: 11 additions & 10 deletions packages/opentelemetry-cloud-monitoring-exporter/src/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ export const OPENTELEMETRY_TASK_VALUE_DEFAULT = generateDefaultTaskValue();
export function transformMetricDescriptor(
metricDescriptor: OTMetricDescriptor,
metricPrefix: string,
displayNamePrefix: string
displayNamePrefix: string,
point: OTPoint
): MetricDescriptor {
return {
type: transformMetricType(metricPrefix, metricDescriptor.name),
description: metricDescriptor.description,
displayName: transformDisplayName(displayNamePrefix, metricDescriptor.name),
metricKind: transformMetricKind(metricDescriptor.metricKind),
valueType: transformValueType(metricDescriptor.valueType),
valueType: transformValueType(metricDescriptor.valueType, point),
unit: metricDescriptor.unit,
labels: [
{
Expand Down Expand Up @@ -86,17 +87,18 @@ function transformMetricKind(kind: OTMetricKind): MetricKind {
case OTMetricKind.UP_DOWN_COUNTER:
case OTMetricKind.VALUE_OBSERVER:
case OTMetricKind.UP_DOWN_SUM_OBSERVER:
case OTMetricKind.VALUE_RECORDER:
return MetricKind.GAUGE;
default:
// TODO: Add support for OTMetricKind.ValueRecorder
// OTMetricKind.Measure was renamed to ValueRecorder in #1117
return MetricKind.UNSPECIFIED;
}
}

/** Transforms a OpenTelemetry ValueType to a StackDriver ValueType. */
function transformValueType(valueType: OTValueType): ValueType {
if (valueType === OTValueType.DOUBLE) {
function transformValueType(valueType: OTValueType, point: OTPoint): ValueType {
if (isDistributionValue(point.value) || isHistogramValue(point.value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with the JS SDK, but looking through this code, isDistribution() will be true for ValueRecorder. Is that right?

I'm confused because transformValue() will then throw on L242.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's correct. I've opened a separate PR to address the support for Distribution and Histogram type metrics to be exported here: #125

When testing with those changes, I've gotten the ValueRecorder to correctly export.

return ValueType.DISTRIBUTION;
} else if (valueType === OTValueType.DOUBLE) {
return ValueType.DOUBLE;
} else if (valueType === OTValueType.INT) {
return ValueType.INT64;
Expand All @@ -115,14 +117,13 @@ export function createTimeSeries(
startTime: string,
projectId: string
): TimeSeries {
const point = metric.aggregator.toPoint();
return {
metric: transformMetric(metric, metricPrefix),
resource: transformResource(metric.resource, projectId),
metricKind: transformMetricKind(metric.descriptor.metricKind),
valueType: transformValueType(metric.descriptor.valueType),
points: [
transformPoint(metric.aggregator.toPoint(), metric.descriptor, startTime),
],
valueType: transformValueType(metric.descriptor.valueType, point),
points: [transformPoint(point, metric.descriptor, startTime)],
};
}

Expand Down
31 changes: 25 additions & 6 deletions packages/opentelemetry-cloud-monitoring-exporter/test/transform.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@ describe('transform', () => {
metricKind: OTMetricKind.OBSERVER,
valueType: OTValueType.DOUBLE,
};
const point: OTPoint = {
value: 50,
timestamp: process.hrtime(),
};
const distributionPoint: OTPoint = {
value: {
min: 0,
max: 100,
count: 5,
sum: 200,
},
timestamp: process.hrtime(),
};

it('should return a Google Cloud Monitoring MetricKind', () => {
assert.strictEqual(
Expand Down Expand Up @@ -77,21 +90,25 @@ describe('transform', () => {
);
assert.strictEqual(
TEST_ONLY.transformMetricKind(OTMetricKind.VALUE_RECORDER),
MetricKind.UNSPECIFIED
MetricKind.GAUGE
);
});

it('should return a Google Cloud Monitoring ValueType', () => {
assert.strictEqual(
TEST_ONLY.transformValueType(OTValueType.INT),
TEST_ONLY.transformValueType(OTValueType.INT, point),
ValueType.INT64
);
assert.strictEqual(
TEST_ONLY.transformValueType(OTValueType.DOUBLE),
TEST_ONLY.transformValueType(OTValueType.DOUBLE, point),
ValueType.DOUBLE
);
assert.strictEqual(
TEST_ONLY.transformValueType(2),
TEST_ONLY.transformValueType(OTValueType.DOUBLE, distributionPoint),
ValueType.DISTRIBUTION
);
assert.strictEqual(
TEST_ONLY.transformValueType(2, point),
ValueType.VALUE_TYPE_UNSPECIFIED
);
});
Expand All @@ -117,7 +134,8 @@ describe('transform', () => {
const descriptor: MetricDescriptor = transformMetricDescriptor(
metricDescriptor,
'custom.googleapis.com/myorg/',
'myorg/'
'myorg/',
point
);

assert.strictEqual(descriptor.description, METRIC_DESCRIPTION);
Expand All @@ -135,7 +153,8 @@ describe('transform', () => {
const descriptor: MetricDescriptor = transformMetricDescriptor(
metricDescriptor1,
'custom.googleapis.com/myorg/',
'myorg/'
'myorg/',
point
);

assert.strictEqual(descriptor.description, METRIC_DESCRIPTION);
Expand Down