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(sdk-metrics)!: drop deprecated type field on MetricDescriptor #5291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure what to say here, there is probably more to say, since, as the test shown this change can have noticeable impact


### :rocket: (Enhancement)

### :bug: (Bug Fix)
Expand Down
35 changes: 13 additions & 22 deletions experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import { diag, Attributes, AttributeValue } from '@opentelemetry/api';
import {
ResourceMetrics,
InstrumentType,
DataPointType,
ScopeMetrics,
MetricData,
Expand Down Expand Up @@ -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
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5266 (comment)

name = name + '_total';
}

Expand Down Expand Up @@ -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'
Expand All @@ -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;
}
Expand All @@ -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(
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
'',
]);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pichlermarc I think this is the natural conclusion of your proposed check right? (or– did I misunderstood your comment and flipped the check?)

Expand Down
8 changes: 4 additions & 4 deletions experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ describe('PrometheusSerializer', () => {

const result = serializer['_serializeSingularDataPoint'](
metric.descriptor.name,
metric.descriptor.type,
metric,
pointData[0]
);
return result;
Expand Down Expand Up @@ -162,7 +162,7 @@ describe('PrometheusSerializer', () => {

const result = serializer['_serializeHistogramDataPoint'](
metric.descriptor.name,
metric.descriptor.type,
metric,
pointData[0]
);
return result;
Expand Down Expand Up @@ -511,7 +511,7 @@ describe('PrometheusSerializer', () => {
} else {
const result = serializer['_serializeSingularDataPoint'](
metric.descriptor.name,
metric.descriptor.type,
metric,
pointData[0]
);
return result;
Expand Down Expand Up @@ -598,7 +598,7 @@ describe('PrometheusSerializer', () => {

const result = serializer['_serializeSingularDataPoint'](
metric.descriptor.name,
metric.descriptor.type,
metric,
pointData[0]
);
return result;
Expand Down
8 changes: 0 additions & 8 deletions experimental/packages/otlp-transformer/test/metrics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { Resource } from '@opentelemetry/resources';
import {
AggregationTemporality,
DataPointType,
InstrumentType,
MetricData,
ResourceMetrics,
} from '@opentelemetry/sdk-metrics';
Expand Down Expand Up @@ -110,7 +109,6 @@ describe('Metrics', () => {
return {
descriptor: {
description: 'this is a description',
type: InstrumentType.COUNTER,
name: 'counter',
unit: '1',
valueType: ValueType.INT,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -211,7 +206,6 @@ describe('Metrics', () => {
return {
descriptor: {
description: 'this is a description',
type: InstrumentType.OBSERVABLE_GAUGE,
name: 'gauge',
unit: '1',
valueType: ValueType.DOUBLE,
Expand Down Expand Up @@ -241,7 +235,6 @@ describe('Metrics', () => {
return {
descriptor: {
description: 'this is a description',
type: InstrumentType.HISTOGRAM,
name: 'hist',
unit: '1',
valueType: ValueType.INT,
Expand Down Expand Up @@ -282,7 +275,6 @@ describe('Metrics', () => {
return {
descriptor: {
description: 'this is a description',
type: InstrumentType.HISTOGRAM,
name: 'xhist',
unit: '1',
valueType: ValueType.INT,
Expand Down
8 changes: 0 additions & 8 deletions experimental/packages/shim-opencensus/src/metric-transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -51,7 +49,6 @@ export function mapOcMetric(metric: oc.Metric): MetricData | null {
description,
name,
unit,
type: mappedType.type,
valueType: mappedType.valueType,
},
};
Expand All @@ -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,
};
Expand Down
1 change: 0 additions & 1 deletion experimental/packages/shim-opencensus/test/OpenCensusMetricProducer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ describe('OpenCensusMetricProducer', () => {
assert.deepStrictEqual(ocMetric.descriptor, {
description: 'Test OC description',
name: 'measure',
type: 'COUNTER',
unit: 'ms',
valueType: ValueType.DOUBLE,
});
Expand Down
6 changes: 0 additions & 6 deletions experimental/packages/shim-opencensus/test/metric-transform.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
DataPointType,
GaugeMetricData,
HistogramMetricData,
InstrumentType,
SumMetricData,
} from '@opentelemetry/sdk-metrics';
import * as assert from 'assert';
Expand Down Expand Up @@ -64,7 +63,6 @@ describe('metric-transform', () => {
descriptor: {
description: 'ocDescription',
name: 'ocMetricName',
type: InstrumentType.COUNTER,
unit: 'ocUnit',
valueType: ValueType.INT,
},
Expand Down Expand Up @@ -107,7 +105,6 @@ describe('metric-transform', () => {
descriptor: {
description: 'ocDescription',
name: 'ocMetricName',
type: InstrumentType.COUNTER,
unit: 'ocUnit',
valueType: ValueType.DOUBLE,
},
Expand Down Expand Up @@ -177,7 +174,6 @@ describe('metric-transform', () => {
descriptor: {
description: 'ocDescription',
name: 'ocMetricName',
type: InstrumentType.HISTOGRAM,
unit: 'ocUnit',
valueType: ValueType.DOUBLE,
},
Expand Down Expand Up @@ -219,7 +215,6 @@ describe('metric-transform', () => {
descriptor: {
description: 'ocDescription',
name: 'ocMetricName',
type: InstrumentType.OBSERVABLE_GAUGE,
unit: 'ocUnit',
valueType: ValueType.INT,
},
Expand Down Expand Up @@ -261,7 +256,6 @@ describe('metric-transform', () => {
descriptor: {
description: 'ocDescription',
name: 'ocMetricName',
type: InstrumentType.OBSERVABLE_GAUGE,
unit: 'ocUnit',
valueType: ValueType.DOUBLE,
},
Expand Down
6 changes: 6 additions & 0 deletions packages/sdk-metrics/src/InstrumentDescriptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
*
Expand Down
4 changes: 2 additions & 2 deletions packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -566,7 +566,7 @@ export class ExponentialHistogramAggregator
}

toMetricData(
descriptor: MetricDescriptor,
descriptor: InstrumentDescriptor,
aggregationTemporality: AggregationTemporality,
accumulationByAttributes: AccumulationRecord<ExponentialHistogramAccumulation>[],
endTime: HrTime
Expand Down
4 changes: 2 additions & 2 deletions packages/sdk-metrics/src/aggregator/Histogram.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -217,7 +217,7 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
}

toMetricData(
descriptor: MetricDescriptor,
descriptor: InstrumentDescriptor,
aggregationTemporality: AggregationTemporality,
accumulationByAttributes: AccumulationRecord<HistogramAccumulation>[],
endTime: HrTime
Expand Down
Loading
Loading