feat(sdk-metrics)!: drop deprecated type field on MetricDescriptor

Move the field to the internal `InstrumentDescriptor` type and keep it for internal use. Fixes open-telemetry#3439
tildeio · Dec 20, 2024 · bf47aa2 · bf47aa2
1 parent 90afa28
commit bf47aa2
Show file tree

Hide file tree

Showing 21 changed files with 58 additions and 81 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
 
 ### :boom: Breaking Change
 
+* feat(sdk-metrics)!: drop deprecated `type` field on `MetricDescriptor` [#5291](https://github.com/open-telemetry/opentelemetry-js/pull/5291)
 * feat(sdk-metrics)!: drop deprecated `InstrumentDescriptor` type; use `MetricDescriptor` instead [#5277](https://github.com/open-telemetry/opentelemetry-js/pull/5266)
 * feat(sdk-metrics)!: bump minimum version of `@opentelemetry/api` peer dependency to 1.9.0 [#5254](https://github.com/open-telemetry/opentelemetry-js/pull/5254) @chancancode
 * chore(shim-opentracing): replace deprecated SpanAttributes [#4430](https://github.com/open-telemetry/opentelemetry-js/pull/4430) @JamieDanielson

diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -7,6 +7,8 @@ All notable changes to experimental packages in this project will be documented
 
 ### :boom: Breaking Change
 
+* feat(exporter-prometheus)!, feat(shim-opencensus)!: drop support for the deprecated `type` field on `MetricDescriptor` [#5291](https://github.com/open-telemetry/opentelemetry-js/pull/5291)
+
 ### :rocket: (Enhancement)
 
 ### :bug: (Bug Fix)

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts b/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts
@@ -17,7 +17,6 @@
 import { diag, Attributes, AttributeValue } from '@opentelemetry/api';
 import {
   ResourceMetrics,
-  InstrumentType,
   DataPointType,
   ScopeMetrics,
   MetricData,
@@ -90,10 +89,14 @@ function sanitizePrometheusMetricName(name: string): string {
  */
 function enforcePrometheusNamingConvention(
   name: string,
-  type: InstrumentType
+  data: MetricData
 ): string {
   // Prometheus requires that metrics of the Counter kind have "_total" suffix
-  if (!name.endsWith('_total') && type === InstrumentType.COUNTER) {
+  if (
+    !name.endsWith('_total') &&
+    data.dataPointType === DataPointType.SUM &&
+    data.isMonotonic
+  ) {
     name = name + '_total';
   }
 
@@ -210,7 +213,7 @@ export class PrometheusSerializer {
     }
     const dataPointType = metricData.dataPointType;
 
-    name = enforcePrometheusNamingConvention(name, metricData.descriptor.type);
+    name = enforcePrometheusNamingConvention(name, metricData);
 
     const help = `# HELP ${name} ${escapeString(
       metricData.descriptor.description || 'description missing'
@@ -225,25 +228,13 @@ export class PrometheusSerializer {
       case DataPointType.SUM:
       case DataPointType.GAUGE: {
         results = metricData.dataPoints
-          .map(it =>
-            this._serializeSingularDataPoint(
-              name,
-              metricData.descriptor.type,
-              it
-            )
-          )
+          .map(it => this._serializeSingularDataPoint(name, metricData, it))
           .join('');
         break;
       }
       case DataPointType.HISTOGRAM: {
         results = metricData.dataPoints
-          .map(it =>
-            this._serializeHistogramDataPoint(
-              name,
-              metricData.descriptor.type,
-              it
-            )
-          )
+          .map(it => this._serializeHistogramDataPoint(name, metricData, it))
           .join('');
         break;
       }
@@ -259,12 +250,12 @@ export class PrometheusSerializer {
 
   private _serializeSingularDataPoint(
     name: string,
-    type: InstrumentType,
+    data: MetricData,
     dataPoint: DataPoint<number>
   ): string {
     let results = '';
 
-    name = enforcePrometheusNamingConvention(name, type);
+    name = enforcePrometheusNamingConvention(name, data);
     const { value, attributes } = dataPoint;
     const timestamp = hrTimeToMilliseconds(dataPoint.endTime);
     results += stringify(
@@ -279,12 +270,12 @@ export class PrometheusSerializer {
 
   private _serializeHistogramDataPoint(
     name: string,
-    type: InstrumentType,
+    data: MetricData,
     dataPoint: DataPoint<Histogram>
   ): string {
     let results = '';
 
-    name = enforcePrometheusNamingConvention(name, type);
+    name = enforcePrometheusNamingConvention(name, data);
     const attributes = dataPoint.attributes;
     const histogram = dataPoint.value;
     const timestamp = hrTimeToMilliseconds(dataPoint.endTime);

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
@@ -389,9 +389,9 @@ describe('PrometheusExporter', () => {
 
       assert.deepStrictEqual(lines, [
         ...serializedDefaultResourceLines,
-        '# HELP metric_observable_counter a test description',
-        '# TYPE metric_observable_counter counter',
-        'metric_observable_counter{key1="attributeValue1"} 20',
+        '# HELP metric_observable_counter_total a test description',
+        '# TYPE metric_observable_counter_total counter',
+        'metric_observable_counter_total{key1="attributeValue1"} 20',
         '',
       ]);
     });

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts
@@ -107,7 +107,7 @@ describe('PrometheusSerializer', () => {
 
         const result = serializer['_serializeSingularDataPoint'](
           metric.descriptor.name,
-          metric.descriptor.type,
+          metric,
           pointData[0]
         );
         return result;
@@ -162,7 +162,7 @@ describe('PrometheusSerializer', () => {
 
         const result = serializer['_serializeHistogramDataPoint'](
           metric.descriptor.name,
-          metric.descriptor.type,
+          metric,
           pointData[0]
         );
         return result;
@@ -511,7 +511,7 @@ describe('PrometheusSerializer', () => {
       } else {
         const result = serializer['_serializeSingularDataPoint'](
           metric.descriptor.name,
-          metric.descriptor.type,
+          metric,
           pointData[0]
         );
         return result;
@@ -598,7 +598,7 @@ describe('PrometheusSerializer', () => {
 
       const result = serializer['_serializeSingularDataPoint'](
         metric.descriptor.name,
-        metric.descriptor.type,
+        metric,
         pointData[0]
       );
       return result;

diff --git a/experimental/packages/otlp-transformer/test/metrics.test.ts b/experimental/packages/otlp-transformer/test/metrics.test.ts
@@ -18,7 +18,6 @@ import { Resource } from '@opentelemetry/resources';
 import {
   AggregationTemporality,
   DataPointType,
-  InstrumentType,
   MetricData,
   ResourceMetrics,
 } from '@opentelemetry/sdk-metrics';
@@ -110,7 +109,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.COUNTER,
         name: 'counter',
         unit: '1',
         valueType: ValueType.INT,
@@ -136,7 +134,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.UP_DOWN_COUNTER,
         name: 'up-down-counter',
         unit: '1',
         valueType: ValueType.INT,
@@ -162,7 +159,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.OBSERVABLE_COUNTER,
         name: 'observable-counter',
         unit: '1',
         valueType: ValueType.INT,
@@ -188,7 +184,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.OBSERVABLE_UP_DOWN_COUNTER,
         name: 'observable-up-down-counter',
         unit: '1',
         valueType: ValueType.INT,
@@ -211,7 +206,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.OBSERVABLE_GAUGE,
         name: 'gauge',
         unit: '1',
         valueType: ValueType.DOUBLE,
@@ -241,7 +235,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.HISTOGRAM,
         name: 'hist',
         unit: '1',
         valueType: ValueType.INT,
@@ -282,7 +275,6 @@ describe('Metrics', () => {
     return {
       descriptor: {
         description: 'this is a description',
-        type: InstrumentType.HISTOGRAM,
         name: 'xhist',
         unit: '1',
         valueType: ValueType.INT,

diff --git a/experimental/packages/shim-opencensus/src/metric-transform.ts b/experimental/packages/shim-opencensus/src/metric-transform.ts
@@ -22,14 +22,12 @@ import {
   DataPointType,
   GaugeMetricData,
   HistogramMetricData,
-  InstrumentType,
   MetricData,
   SumMetricData,
 } from '@opentelemetry/sdk-metrics';
 
 type BaseMetric = Omit<MetricData, 'dataPoints' | 'dataPointType'>;
 interface MappedType {
-  type: InstrumentType;
   valueType: ValueType;
   dataPointType:
     | DataPointType.GAUGE
@@ -51,7 +49,6 @@ export function mapOcMetric(metric: oc.Metric): MetricData | null {
       description,
       name,
       unit,
-      type: mappedType.type,
       valueType: mappedType.valueType,
     },
   };
@@ -72,33 +69,28 @@ function mapOcMetricDescriptorType(
   switch (type) {
     case oc.MetricDescriptorType.GAUGE_INT64:
       return {
-        type: InstrumentType.OBSERVABLE_GAUGE,
         valueType: ValueType.INT,
         dataPointType: DataPointType.GAUGE,
       };
     case oc.MetricDescriptorType.GAUGE_DOUBLE:
       return {
-        type: InstrumentType.OBSERVABLE_GAUGE,
         valueType: ValueType.DOUBLE,
         dataPointType: DataPointType.GAUGE,
       };
 
     case oc.MetricDescriptorType.CUMULATIVE_INT64:
       return {
-        type: InstrumentType.COUNTER,
         valueType: ValueType.INT,
         dataPointType: DataPointType.SUM,
       };
     case oc.MetricDescriptorType.CUMULATIVE_DOUBLE:
       return {
-        type: InstrumentType.COUNTER,
         valueType: ValueType.DOUBLE,
         dataPointType: DataPointType.SUM,
       };
 
     case oc.MetricDescriptorType.CUMULATIVE_DISTRIBUTION:
       return {
-        type: InstrumentType.HISTOGRAM,
         valueType: ValueType.DOUBLE,
         dataPointType: DataPointType.HISTOGRAM,
       };

diff --git a/experimental/packages/shim-opencensus/test/OpenCensusMetricProducer.test.ts b/experimental/packages/shim-opencensus/test/OpenCensusMetricProducer.test.ts
@@ -100,7 +100,6 @@ describe('OpenCensusMetricProducer', () => {
     assert.deepStrictEqual(ocMetric.descriptor, {
       description: 'Test OC description',
       name: 'measure',
-      type: 'COUNTER',
       unit: 'ms',
       valueType: ValueType.DOUBLE,
     });

diff --git a/experimental/packages/shim-opencensus/test/metric-transform.test.ts b/experimental/packages/shim-opencensus/test/metric-transform.test.ts
@@ -23,7 +23,6 @@ import {
   DataPointType,
   GaugeMetricData,
   HistogramMetricData,
-  InstrumentType,
   SumMetricData,
 } from '@opentelemetry/sdk-metrics';
 import * as assert from 'assert';
@@ -64,7 +63,6 @@ describe('metric-transform', () => {
       descriptor: {
         description: 'ocDescription',
         name: 'ocMetricName',
-        type: InstrumentType.COUNTER,
         unit: 'ocUnit',
         valueType: ValueType.INT,
       },
@@ -107,7 +105,6 @@ describe('metric-transform', () => {
       descriptor: {
         description: 'ocDescription',
         name: 'ocMetricName',
-        type: InstrumentType.COUNTER,
         unit: 'ocUnit',
         valueType: ValueType.DOUBLE,
       },
@@ -177,7 +174,6 @@ describe('metric-transform', () => {
       descriptor: {
         description: 'ocDescription',
         name: 'ocMetricName',
-        type: InstrumentType.HISTOGRAM,
         unit: 'ocUnit',
         valueType: ValueType.DOUBLE,
       },
@@ -219,7 +215,6 @@ describe('metric-transform', () => {
       descriptor: {
         description: 'ocDescription',
         name: 'ocMetricName',
-        type: InstrumentType.OBSERVABLE_GAUGE,
         unit: 'ocUnit',
         valueType: ValueType.INT,
       },
@@ -261,7 +256,6 @@ describe('metric-transform', () => {
       descriptor: {
         description: 'ocDescription',
         name: 'ocMetricName',
-        type: InstrumentType.OBSERVABLE_GAUGE,
         unit: 'ocUnit',
         valueType: ValueType.DOUBLE,
       },

diff --git a/packages/sdk-metrics/src/InstrumentDescriptor.ts b/packages/sdk-metrics/src/InstrumentDescriptor.ts
@@ -31,6 +31,12 @@ import { InstrumentType, MetricDescriptor } from './export/MetricData';
  * which may not contains internal fields like metric advice.
  */
 export interface InstrumentDescriptor extends MetricDescriptor {
+  /**
+   * For internal use; exporter should avoid depending on the type of the
+   * instrument as their resulting aggregator can be re-mapped with views.
+   */
+  readonly type: InstrumentType;
+
   /**
    * See {@link MetricAdvice}
    *

diff --git a/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts b/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts
@@ -25,11 +25,11 @@ import {
   DataPointType,
   ExponentialHistogramMetricData,
   InstrumentType,
-  MetricDescriptor,
 } from '../export/MetricData';
 import { diag, HrTime } from '@opentelemetry/api';
 import { Maybe } from '../utils';
 import { AggregationTemporality } from '../export/AggregationTemporality';
+import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Buckets } from './exponential-histogram/Buckets';
 import { getMapping } from './exponential-histogram/mapping/getMapping';
 import { Mapping } from './exponential-histogram/mapping/types';
@@ -566,7 +566,7 @@ export class ExponentialHistogramAggregator
   }
 
   toMetricData(
-    descriptor: MetricDescriptor,
+    descriptor: InstrumentDescriptor,
     aggregationTemporality: AggregationTemporality,
     accumulationByAttributes: AccumulationRecord<ExponentialHistogramAccumulation>[],
     endTime: HrTime

diff --git a/packages/sdk-metrics/src/aggregator/Histogram.ts b/packages/sdk-metrics/src/aggregator/Histogram.ts
@@ -24,11 +24,11 @@ import {
   DataPointType,
   HistogramMetricData,
   InstrumentType,
-  MetricDescriptor,
 } from '../export/MetricData';
 import { HrTime } from '@opentelemetry/api';
 import { binarySearchUB, Maybe } from '../utils';
 import { AggregationTemporality } from '../export/AggregationTemporality';
+import { InstrumentDescriptor } from '../InstrumentDescriptor';
 
 /**
  * Internal value type for HistogramAggregation.
@@ -217,7 +217,7 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
   }
 
   toMetricData(
-    descriptor: MetricDescriptor,
+    descriptor: InstrumentDescriptor,
     aggregationTemporality: AggregationTemporality,
     accumulationByAttributes: AccumulationRecord<HistogramAccumulation>[],
     endTime: HrTime