From c96c019733f0ca60959cb36934530f9eb95b7f0e Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Tue, 29 Nov 2022 22:16:13 +0000 Subject: [PATCH] fix(monitoring-exporter): convert to GCM format based on metric data type, not instrument type --- .../instrument-snapshot.test.ts.js | 92 +++++++++++++++++++ .../src/monitoring.ts | 70 ++++++-------- .../src/transform.ts | 62 ++++++++----- .../test/instrument-snapshot.test.ts | 31 ++++++- .../test/monitoring.test.ts | 50 +++++++++- .../test/util.ts | 6 +- 6 files changed, 241 insertions(+), 70 deletions(-) diff --git a/packages/opentelemetry-cloud-monitoring-exporter/__snapshots__/instrument-snapshot.test.ts.js b/packages/opentelemetry-cloud-monitoring-exporter/__snapshots__/instrument-snapshot.test.ts.js index 474376df..be6cf5f1 100644 --- a/packages/opentelemetry-cloud-monitoring-exporter/__snapshots__/instrument-snapshot.test.ts.js +++ b/packages/opentelemetry-cloud-monitoring-exporter/__snapshots__/instrument-snapshot.test.ts.js @@ -811,3 +811,95 @@ exports['MetricExporter snapshot tests UpDownCounter - INT 1'] = [ ] } ] + +exports['MetricExporter snapshot tests reconfigure with views counter with histogram view 1'] = [ + { + "uri": "/v3/projects/otel-starter-project/metricDescriptors", + "body": { + "type": "workload.googleapis.com/myrenamedhistogram", + "description": "instrument description", + "displayName": "myrenamedhistogram", + "metricKind": "CUMULATIVE", + "valueType": "DOUBLE", + "unit": "{myunit}", + "labels": [ + { + "key": "opentelemetry_task", + "description": "OpenTelemetry task identifier" + } + ] + }, + "userAgent": [ + "opentelemetry-js/1.8.0 google-cloud-metric-exporter/0.14.0 google-api-nodejs-client/5.1.0 (gzip)" + ] + }, + { + "uri": "/v3/projects/otel-starter-project/timeSeries", + "body": { + "timeSeries": [ + { + "metric": { + "type": "workload.googleapis.com/myrenamedhistogram", + "labels": { + "opentelemetry_task": "opentelemetry_task" + } + }, + "resource": { + "type": "global", + "labels": { + "project_id": "otel-starter-project" + } + }, + "metricKind": "CUMULATIVE", + "valueType": "DOUBLE", + "points": [ + { + "value": { + "distributionValue": { + "count": "1", + "mean": 12.3, + "bucketOptions": { + "explicitBuckets": { + "bounds": [ + 0, + 5, + 10, + 25, + 50, + 75, + 100, + 250, + 500, + 1000 + ] + } + }, + "bucketCounts": [ + "0", + "0", + "0", + "1", + "0", + "0", + "0", + "0", + "0", + "0", + "0" + ] + } + }, + "interval": { + "startTime": "startTime", + "endTime": "endTime" + } + } + ] + } + ] + }, + "userAgent": [ + "opentelemetry-js/1.8.0 google-cloud-metric-exporter/0.14.0 google-api-nodejs-client/5.1.0 (gzip)" + ] + } +] diff --git a/packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts b/packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts index 14fdbe07..3874b18b 100644 --- a/packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts +++ b/packages/opentelemetry-cloud-monitoring-exporter/src/monitoring.ts @@ -15,7 +15,7 @@ import { PushMetricExporter, ResourceMetrics, - InstrumentDescriptor, + MetricData, } from '@opentelemetry/sdk-metrics'; import { ExportResult, @@ -62,12 +62,14 @@ export class MetricExporter implements PushMetricExporter { private _projectId: string | void | Promise; private readonly _metricPrefix: string; private readonly _auth: GoogleAuth; - private readonly _startTime = new Date().toISOString(); static readonly DEFAULT_METRIC_PREFIX: string = 'workload.googleapis.com'; - private registeredInstrumentDescriptors: Map = - new Map(); + /** + * Set of OTel metric names that have already had their metric descriptors successfully + * created + */ + private createdMetricDescriptors: Set = new Set(); private _monitoring: monitoring_v3.Monitoring; @@ -144,9 +146,7 @@ export class MetricExporter implements PushMetricExporter { const timeSeries: TimeSeries[] = []; for (const scopeMetric of resourceMetrics.scopeMetrics) { for (const metric of scopeMetric.metrics) { - const isRegistered = await this._registerMetricDescriptor( - metric.descriptor - ); + const isRegistered = await this._registerMetricDescriptor(metric); if (isRegistered) { timeSeries.push( ...createTimeSeries(metric, resource, this._metricPrefix) @@ -180,54 +180,38 @@ export class MetricExporter implements PushMetricExporter { /** * Returns true if the given metricDescriptor is successfully registered to * Google Cloud Monitoring, or the exact same metric has already been - * registered. Returns false otherwise. - * @param instrumentDescriptor The OpenTelemetry MetricDescriptor. + * registered. Returns false otherwise and should be skipped. + * + * @param metric The OpenTelemetry MetricData. */ private async _registerMetricDescriptor( - instrumentDescriptor: InstrumentDescriptor - ) { - const existingInstrumentDescriptor = - this.registeredInstrumentDescriptors.get(instrumentDescriptor.name); + metric: MetricData + ): Promise { + const isDescriptorCreated = this.createdMetricDescriptors.has( + metric.descriptor.name + ); - if (existingInstrumentDescriptor) { - if (existingInstrumentDescriptor === instrumentDescriptor) { - // Ignore descriptors that are already registered. - return true; - } else { - diag.warn( - 'A different metric with the same name is already registered: %s', - existingInstrumentDescriptor - ); - return false; - } + if (isDescriptorCreated) { + return true; } - try { - await this._createMetricDescriptor(instrumentDescriptor); - this.registeredInstrumentDescriptors.set( - instrumentDescriptor.name, - instrumentDescriptor - ); + const res = await this._createMetricDescriptor(metric); + if (res) { + this.createdMetricDescriptors.add(metric.descriptor.name); return true; - } catch (e) { - const err = asError(e); - diag.error('Error creating metric descriptor: %s', err.message); - return false; } + return false; } /** * Calls CreateMetricDescriptor in the GCM API for the given InstrumentDescriptor - * @param instrumentDescriptor The OpenTelemetry InstrumentDescriptor. + * @param metric The OpenTelemetry MetricData. + * @returns whether or not the descriptor was successfully created */ - private async _createMetricDescriptor( - instrumentDescriptor: InstrumentDescriptor - ) { + private async _createMetricDescriptor(metric: MetricData): Promise { const authClient = await this._authorize(); - const descriptor = transformMetricDescriptor( - instrumentDescriptor, - this._metricPrefix - ); + const descriptor = transformMetricDescriptor(metric, this._metricPrefix); + try { await this._monitoring.projects.metricDescriptors.create({ name: `projects/${this._projectId}`, @@ -235,9 +219,11 @@ export class MetricExporter implements PushMetricExporter { auth: authClient, }); diag.debug('sent metric descriptor', descriptor); + return true; } catch (e) { const err = asError(e); diag.error('Failed to create metric descriptor: %s', err.message); + return false; } } diff --git a/packages/opentelemetry-cloud-monitoring-exporter/src/transform.ts b/packages/opentelemetry-cloud-monitoring-exporter/src/transform.ts index c7bb4f66..473fad81 100644 --- a/packages/opentelemetry-cloud-monitoring-exporter/src/transform.ts +++ b/packages/opentelemetry-cloud-monitoring-exporter/src/transform.ts @@ -14,7 +14,6 @@ import { InstrumentDescriptor, - InstrumentType, Histogram, MetricData, DataPoint, @@ -39,17 +38,28 @@ const OPENTELEMETRY_TASK = 'opentelemetry_task'; const OPENTELEMETRY_TASK_DESCRIPTION = 'OpenTelemetry task identifier'; export const OPENTELEMETRY_TASK_VALUE_DEFAULT = generateDefaultTaskValue(); +/** + * + * @param metric the MetricData to create a descriptor for + * @param metricPrefix prefix to add to metric names + * @param displayNamePrefix prefix to add to display name in the descriptor + * @returns the GCM MetricDescriptor or null if the MetricData was empty + */ export function transformMetricDescriptor( - instrumentDescriptor: InstrumentDescriptor, + metric: MetricData, metricPrefix: string ): MetricDescriptor { + const { + descriptor: {name, description, unit}, + } = metric; + return { - type: transformMetricType(metricPrefix, instrumentDescriptor.name), - description: instrumentDescriptor.description, - displayName: instrumentDescriptor.name, - metricKind: transformMetricKind(instrumentDescriptor.type), - valueType: transformValueType(instrumentDescriptor.valueType), - unit: instrumentDescriptor.unit, + type: transformMetricType(metricPrefix, name), + description, + displayName: name, + metricKind: transformMetricKind(metric), + valueType: transformValueType(metric), + unit, labels: [ { key: OPENTELEMETRY_TASK, @@ -65,30 +75,34 @@ function transformMetricType(metricPrefix: string, name: string): string { } /** Transforms a OpenTelemetry instrument type to a GCM MetricKind. */ -function transformMetricKind(instrumentType: InstrumentType): MetricKind { - switch (instrumentType) { - case InstrumentType.COUNTER: - case InstrumentType.OBSERVABLE_COUNTER: - case InstrumentType.HISTOGRAM: - return MetricKind.CUMULATIVE; - case InstrumentType.UP_DOWN_COUNTER: - case InstrumentType.OBSERVABLE_GAUGE: - case InstrumentType.OBSERVABLE_UP_DOWN_COUNTER: +function transformMetricKind(metric: MetricData): MetricKind { + switch (metric.dataPointType) { + case DataPointType.SUM: + return metric.isMonotonic ? MetricKind.CUMULATIVE : MetricKind.GAUGE; + case DataPointType.GAUGE: return MetricKind.GAUGE; + case DataPointType.HISTOGRAM: + return MetricKind.CUMULATIVE; default: - exhaust(instrumentType); - diag.info('Encountered unexpected instrumentType=%s', instrumentType); + exhaust(metric); + diag.info( + 'Encountered unexpected data point type %s', + (metric as MetricData).dataPointType + ); return MetricKind.UNSPECIFIED; } } /** Transforms a OpenTelemetry ValueType to a GCM ValueType. */ -function transformValueType(valueType: OTValueType): ValueType { +function transformValueType(metric: MetricData): ValueType { + const {valueType} = metric.descriptor; if (valueType === OTValueType.DOUBLE) { return ValueType.DOUBLE; } else if (valueType === OTValueType.INT) { return ValueType.INT64; } else { + exhaust(valueType); + diag.info('Encountered unexpected value type %s', valueType); return ValueType.VALUE_TYPE_UNSPECIFIED; } } @@ -97,13 +111,13 @@ function transformValueType(valueType: OTValueType): ValueType { * Converts metric's timeseries to a TimeSeries, so that metric can be * uploaded to GCM. */ -export function createTimeSeries( - metric: TMetricData, +export function createTimeSeries( + metric: MetricData, resource: MonitoredResource, metricPrefix: string ): TimeSeries[] { - const metricKind = transformMetricKind(metric.descriptor.type); - const valueType = transformValueType(metric.descriptor.valueType); + const metricKind = transformMetricKind(metric); + const valueType = transformValueType(metric); return transformPoints(metric, metricPrefix).map(({point, metric}) => ({ metric, diff --git a/packages/opentelemetry-cloud-monitoring-exporter/test/instrument-snapshot.test.ts b/packages/opentelemetry-cloud-monitoring-exporter/test/instrument-snapshot.test.ts index b890e83d..59b50393 100644 --- a/packages/opentelemetry-cloud-monitoring-exporter/test/instrument-snapshot.test.ts +++ b/packages/opentelemetry-cloud-monitoring-exporter/test/instrument-snapshot.test.ts @@ -22,7 +22,7 @@ import {Attributes, ValueType} from '@opentelemetry/api'; import * as snapshot from 'snap-shot-it'; import {ExportResult, ExportResultCode} from '@opentelemetry/core'; -import {ResourceMetrics} from '@opentelemetry/sdk-metrics'; +import {Aggregation, ResourceMetrics, View} from '@opentelemetry/sdk-metrics'; import * as assert from 'assert'; import * as nock from 'nock'; import * as sinon from 'sinon'; @@ -230,6 +230,35 @@ describe('MetricExporter snapshot tests', () => { gcmNock.snapshotCalls(); }); }); + + describe('reconfigure with views', () => { + it('counter with histogram view', async () => { + const resourceMetrics = await generateMetricsData( + (_, meter) => { + meter + .createCounter('mycounter', { + description: 'instrument description', + unit: '{myunit}', + valueType: ValueType.DOUBLE, + }) + .add(12.3); + }, + { + views: [ + new View({ + instrumentName: 'mycounter', + aggregation: Aggregation.Histogram(), + name: 'myrenamedhistogram', + }), + ], + } + ); + + const result = await callExporter(resourceMetrics); + assert.deepStrictEqual(result, {code: ExportResultCode.SUCCESS}); + gcmNock.snapshotCalls(); + }); + }); }); function callExporter(resourceMetrics: ResourceMetrics): Promise { diff --git a/packages/opentelemetry-cloud-monitoring-exporter/test/monitoring.test.ts b/packages/opentelemetry-cloud-monitoring-exporter/test/monitoring.test.ts index cb17ed76..3e7f1624 100644 --- a/packages/opentelemetry-cloud-monitoring-exporter/test/monitoring.test.ts +++ b/packages/opentelemetry-cloud-monitoring-exporter/test/monitoring.test.ts @@ -73,15 +73,21 @@ describe('MetricExporter', () => { >; let getClientShouldFail: boolean; let createTimeSeriesShouldFail: boolean; + let createMetricDesriptorShouldFail: boolean; beforeEach(() => { getClientShouldFail = false; createTimeSeriesShouldFail = false; + createMetricDesriptorShouldFail = false; exporter = new MetricExporter({}); metricDescriptors = sinon.spy( // eslint-disable-next-line @typescript-eslint/no-unused-vars - async (request: any, params: any): Promise => {} + async (request: any, params: any): Promise => { + if (createMetricDesriptorShouldFail) { + throw new Error('fail'); + } + } ); sinon.replace( @@ -181,6 +187,48 @@ describe('MetricExporter', () => { assert.strictEqual(timeSeries.callCount, 1); }); + it('should skip metrics when MetricDescriptor creation fails', async () => { + const resourceMetrics = await generateMetricsData(); + + createMetricDesriptorShouldFail = true; + const result = await new Promise(resolve => { + exporter.export(resourceMetrics, result => { + resolve(result); + }); + }); + + assert.strictEqual(metricDescriptors.callCount, 1); + assert.strictEqual(timeSeries.callCount, 0); + assert.deepStrictEqual(result.code, ExportResultCode.SUCCESS); + }); + + it('should skip MetricDescriptor creation when a metric has already been seen', async () => { + const resourceMetrics = await generateMetricsData(); + + let result = await new Promise(resolve => { + exporter.export(resourceMetrics, result => { + resolve(result); + }); + }); + + assert.strictEqual(metricDescriptors.callCount, 1); + assert.strictEqual(timeSeries.callCount, 1); + assert.deepStrictEqual(result.code, ExportResultCode.SUCCESS); + + // Second time around, MetricDescriptors.create() should be skipped + metricDescriptors.resetHistory(); + timeSeries.resetHistory(); + result = await new Promise(resolve => { + exporter.export(resourceMetrics, result => { + resolve(result); + }); + }); + + assert.strictEqual(metricDescriptors.callCount, 0); + assert.strictEqual(timeSeries.callCount, 1); + assert.deepStrictEqual(result.code, ExportResultCode.SUCCESS); + }); + it('should return FAILED if there is an error sending TimeSeries', async () => { const resourceMetrics = await generateMetricsData(); diff --git a/packages/opentelemetry-cloud-monitoring-exporter/test/util.ts b/packages/opentelemetry-cloud-monitoring-exporter/test/util.ts index f2cc5a8c..fb7add49 100644 --- a/packages/opentelemetry-cloud-monitoring-exporter/test/util.ts +++ b/packages/opentelemetry-cloud-monitoring-exporter/test/util.ts @@ -14,6 +14,7 @@ import { MeterProvider, + MeterProviderOptions, MetricReader, ResourceMetrics, } from '@opentelemetry/sdk-metrics'; @@ -27,9 +28,10 @@ class InMemoryMetricReader extends MetricReader { } export async function generateMetricsData( - customize?: (meterProvider: MeterProvider, meter: Meter) => void + customize?: (meterProvider: MeterProvider, meter: Meter) => void, + meterProviderOptions?: MeterProviderOptions ): Promise { - const meterProvider = new MeterProvider(); + const meterProvider = new MeterProvider(meterProviderOptions); const reader = new InMemoryMetricReader(); meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test-meter');