From 989b8d0cc2daf4e3b2c5227a35b689d625e68ebe Mon Sep 17 00:00:00 2001 From: Andy Fleming Date: Tue, 17 May 2022 20:58:09 -0700 Subject: [PATCH 1/6] fix(metrics): updates unit option behavior to be spec-compliant --- .../src/InstrumentDescriptor.ts | 2 +- .../test/InstrumentDescriptor.test.ts | 53 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts index 81b19b7ec1d..077d867f0b1 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts @@ -42,7 +42,7 @@ export function createInstrumentDescriptor(name: string, type: InstrumentType, o name, type, description: options?.description ?? '', - unit: options?.unit ?? '1', + unit: options?.unit === '' ? '1' : options?.unit ?? '', valueType: options?.valueType ?? ValueType.DOUBLE, }; } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts new file mode 100644 index 00000000000..a55444a34b9 --- /dev/null +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts @@ -0,0 +1,53 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as assert from 'assert'; +import {createInstrumentDescriptor, InstrumentType} from '../src/InstrumentDescriptor'; + +describe('InstrumentDescriptor', () => { + + describe('createInstrumentDescriptor', () => { + + // Spec compliance: https://github.com/open-telemetry/opentelemetry-js/issues/2910 + it('should interpret a null unit value as a blank string', () => { + const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { + unit: null as any, + }); + + assert.strictEqual(result.unit, ''); + }); + + // Spec compliance: https://github.com/open-telemetry/opentelemetry-js/issues/2910 + it('should interpret an undefined unit value as a blank string', () => { + const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { + unit: undefined, + }); + + assert.strictEqual(result.unit, ''); + }); + + // Spec compliance: https://github.com/open-telemetry/opentelemetry-js/issues/2910 + it('should interpret a null unit value as a blank string', () => { + const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { + unit: '', + }); + + assert.strictEqual(result.unit, '1'); + }); + + }); + +}); From de30301087fce774b91ec5694373c8d20207d654 Mon Sep 17 00:00:00 2001 From: Andy Fleming Date: Wed, 25 May 2022 08:39:14 -0700 Subject: [PATCH 2/6] adjusts expectation for defaults for unit --- .../opentelemetry-sdk-metrics-base/test/Instruments.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts index 7c82dfc5819..d83239b9978 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts @@ -64,7 +64,7 @@ describe('Instruments', () => { descriptor: { name: 'test', description: '', - unit: '1', + unit: '', type: InstrumentType.COUNTER, valueType: ValueType.INT, }, @@ -180,7 +180,7 @@ describe('Instruments', () => { descriptor: { name: 'test', description: '', - unit: '1', + unit: '', type: InstrumentType.UP_DOWN_COUNTER, valueType: ValueType.INT, }, @@ -265,7 +265,7 @@ describe('Instruments', () => { descriptor: { name: 'test', description: '', - unit: '1', + unit: '', type: InstrumentType.HISTOGRAM, valueType: ValueType.INT, }, From e90d93bc9be456664ebae79ffdaba65323c6f938 Mon Sep 17 00:00:00 2001 From: Andy Fleming Date: Thu, 26 May 2022 16:19:46 -0700 Subject: [PATCH 3/6] updates unit value for tests --- .../test/metricsHelper.ts | 6 +++--- .../test/metricsHelper.ts | 12 ++++++------ .../test/metricsHelper.ts | 6 +++--- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts index 997cd0ed86d..36a45d13158 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts @@ -131,7 +131,7 @@ export function ensureExportedCounterIsCorrect( assert.deepStrictEqual(metric, { name: 'int-counter', description: 'sample counter description', - unit: '1', + unit: '', data: 'sum', sum: { dataPoints: [ @@ -159,7 +159,7 @@ export function ensureExportedObservableGaugeIsCorrect( assert.deepStrictEqual(metric, { name: 'double-observable-gauge', description: 'sample observable gauge description', - unit: '1', + unit: '', data: 'gauge', gauge: { dataPoints: [ @@ -187,7 +187,7 @@ export function ensureExportedHistogramIsCorrect( assert.deepStrictEqual(metric, { name: 'int-histogram', description: 'sample histogram description', - unit: '1', + unit: '', data: 'histogram', histogram: { dataPoints: [ diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts index 31e2bcb96d3..a3c9571d3f2 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts @@ -237,7 +237,7 @@ export function ensureCounterIsCorrect( assert.deepStrictEqual(metric, { name: 'int-counter', description: 'sample counter description', - unit: '1', + unit: '', sum: { dataPoints: [ { @@ -261,7 +261,7 @@ export function ensureDoubleCounterIsCorrect( assert.deepStrictEqual(metric, { name: 'double-counter', description: 'sample counter description', - unit: '1', + unit: '', doubleSum: { dataPoints: [ { @@ -287,7 +287,7 @@ export function ensureObservableGaugeIsCorrect( assert.deepStrictEqual(metric, { name, description: 'sample observable gauge description', - unit: '1', + unit: '', gauge: { dataPoints: [ { @@ -311,7 +311,7 @@ export function ensureObservableCounterIsCorrect( assert.deepStrictEqual(metric, { name, description: 'sample observable counter description', - unit: '1', + unit: '', doubleSum: { isMonotonic: true, dataPoints: [ @@ -337,7 +337,7 @@ export function ensureObservableUpDownCounterIsCorrect( assert.deepStrictEqual(metric, { name, description: 'sample observable up down counter description', - unit: '1', + unit: '', doubleSum: { isMonotonic: false, dataPoints: [ @@ -363,7 +363,7 @@ export function ensureHistogramIsCorrect( assert.deepStrictEqual(metric, { name: 'int-histogram', description: 'sample histogram description', - unit: '1', + unit: '', histogram: { dataPoints: [ { diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts index 0bacab995cb..daa98a0d98f 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts @@ -135,7 +135,7 @@ export function ensureExportedCounterIsCorrect( assert.deepStrictEqual(metric, { name: 'int-counter', description: 'sample counter description', - unit: '1', + unit: '', sum: { dataPoints: [ { @@ -158,7 +158,7 @@ export function ensureExportedObservableGaugeIsCorrect( assert.deepStrictEqual(metric, { name: 'double-observable-gauge', description: 'sample observable gauge description', - unit: '1', + unit: '', gauge: { dataPoints: [ { @@ -181,7 +181,7 @@ export function ensureExportedHistogramIsCorrect( assert.deepStrictEqual(metric, { name: 'int-histogram', description: 'sample histogram description', - unit: '1', + unit: '', histogram: { dataPoints: [ { From 42a753a2aa974798e82c731262cb6e197b969d00 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Fri, 27 May 2022 10:23:01 -0400 Subject: [PATCH 4/6] Add changelog entry --- experimental/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 60443a66935..6ff4ecb81d2 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -27,6 +27,7 @@ All notable changes to experimental packages in this project will be documented * fix(otlp-exporter-base): include esm and esnext in package files #2952 @dyladan * fix(otlp-http-exporter): update endpoint to match spec #2895 @svetlanabrennan * fix(otlp-transformer): include esm and esnext in package files and update README #2992 @pichlermarc +* fix(metrics): specification compliant default metric unit #2983 @andyfleming ### :books: (Refine Doc) From b8c50e82d362b8099dad13b406eb967b743a9aa7 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Fri, 27 May 2022 10:33:50 -0400 Subject: [PATCH 5/6] Remove coersion of blank string to '1' --- .../src/InstrumentDescriptor.ts | 2 +- .../test/InstrumentDescriptor.test.ts | 39 +++++-------------- 2 files changed, 10 insertions(+), 31 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts index 077d867f0b1..fa2985ff9d1 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts @@ -42,7 +42,7 @@ export function createInstrumentDescriptor(name: string, type: InstrumentType, o name, type, description: options?.description ?? '', - unit: options?.unit === '' ? '1' : options?.unit ?? '', + unit: options?.unit ?? '', valueType: options?.valueType ?? ValueType.DOUBLE, }; } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts index a55444a34b9..c6bcbc6731a 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts @@ -18,36 +18,15 @@ import * as assert from 'assert'; import {createInstrumentDescriptor, InstrumentType} from '../src/InstrumentDescriptor'; describe('InstrumentDescriptor', () => { - describe('createInstrumentDescriptor', () => { - - // Spec compliance: https://github.com/open-telemetry/opentelemetry-js/issues/2910 - it('should interpret a null unit value as a blank string', () => { - const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { - unit: null as any, - }); - - assert.strictEqual(result.unit, ''); - }); - - // Spec compliance: https://github.com/open-telemetry/opentelemetry-js/issues/2910 - it('should interpret an undefined unit value as a blank string', () => { - const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { - unit: undefined, - }); - - assert.strictEqual(result.unit, ''); - }); - - // Spec compliance: https://github.com/open-telemetry/opentelemetry-js/issues/2910 - it('should interpret a null unit value as a blank string', () => { - const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { - unit: '', - }); - - assert.strictEqual(result.unit, '1'); - }); - + for (const val of [null, undefined]) { + it(`should interpret an empty unit value as a blank string (${val})`, () => { + const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { + unit: val as any, + }); + + assert.strictEqual(result.unit, ''); + }) + } }); - }); From 4a3fd25f01ab030ca7e9941248d5862ce2d86438 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Fri, 27 May 2022 10:38:31 -0400 Subject: [PATCH 6/6] Lint --- .../test/InstrumentDescriptor.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts index c6bcbc6731a..22397faea4b 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/InstrumentDescriptor.test.ts @@ -24,9 +24,8 @@ describe('InstrumentDescriptor', () => { const result = createInstrumentDescriptor('example', InstrumentType.COUNTER, { unit: val as any, }); - assert.strictEqual(result.unit, ''); - }) + }); } }); });