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.

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

refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope #2959

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file.
* feat: warn when hooked module is already loaded [#2926](https://github.com/open-telemetry/opentelemetry-js/pull/2926) @nozik
* feat: implement OSDetector [#2927](https://github.com/open-telemetry/opentelemetry-js/pull/2927) @rauno56
* feat: implement HostDetector [#2921](https://github.com/open-telemetry/opentelemetry-js/pull/2921) @rauno56
* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc
Copy link
Member

@legendecas legendecas May 10, 2022

Choose a reason for hiding this comment

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

There are two CHANGELOG files: one in the root directory and one in the exprimental/. I find that people tend to overlook the one in the experimental/ directory. Maybe we should merge those two.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also picked the wrong one more than once. 😅

However, I added this entry in both CHANGELOG.md files intentionally as it adds InstrumentationScope to @opentelemetry/core (enhancement for stable), but also changes field names in the Metrics SDK (breaking for experimental).

Copy link
Member

Choose a reason for hiding this comment

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

in this case you should modify both changelog files like you did.

There are two CHANGELOG files: one in the root directory and one in the exprimental/. I find that people tend to overlook the one in the experimental/ directory. Maybe we should merge those two.

I thought about this, but I think it makes for a confusing changelog file.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc
* `@opentelemetry/core`: add InstrumentationScope [#2959](https://github.com/open-telemetry/opentelemetry-js/pull/2959) @pichlermarc

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the changelog entry for stable in b5296d9 🙂


### :bug: (Bug Fix)

Expand Down
2 changes: 2 additions & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ All notable changes to experimental packages in this project will be documented

### :boom: Breaking Change

* refactor(metrics-sdk): rename InstrumentationLibrary -> InstrumentationScope #2959 @pichlermarc

### :rocket: (Enhancement)

* feat(exporters): update proto version and use otlp-transformer #2929 @pichlermarc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ describe('OTLPMetricExporter - web', () => {

ensureCounterIsCorrect(
metric1,
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[0].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[0].dataPoints[0].startTime)
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[0].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[0].dataPoints[0].startTime)
);


Expand All @@ -126,8 +126,8 @@ describe('OTLPMetricExporter - web', () => {
);
ensureObservableGaugeIsCorrect(
metric2,
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].startTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].startTime),
6,
'double-observable-gauge2'
);
Expand All @@ -138,8 +138,8 @@ describe('OTLPMetricExporter - web', () => {
);
ensureHistogramIsCorrect(
metric3,
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[2].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[2].dataPoints[0].startTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[2].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[2].dataPoints[0].startTime),
[0, 100],
[0, 2, 0]
);
Expand Down Expand Up @@ -221,8 +221,8 @@ describe('OTLPMetricExporter - web', () => {
assert.ok(typeof metric1 !== 'undefined', "metric doesn't exist");
ensureCounterIsCorrect(
metric1,
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[0].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[0].dataPoints[0].startTime)
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[0].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[0].dataPoints[0].startTime)
);

assert.ok(
Expand All @@ -231,8 +231,8 @@ describe('OTLPMetricExporter - web', () => {
);
ensureObservableGaugeIsCorrect(
metric2,
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].startTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].startTime),
6,
'double-observable-gauge2'
);
Expand All @@ -243,8 +243,8 @@ describe('OTLPMetricExporter - web', () => {
);
ensureHistogramIsCorrect(
metric3,
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[2].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[2].dataPoints[0].startTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[2].dataPoints[0].endTime),
hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[2].dataPoints[0].startTime),
[0, 100],
[0, 2, 0]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
} from '@opentelemetry/api-metrics';
import { Resource } from '@opentelemetry/resources';
import * as assert from 'assert';
import { InstrumentationLibrary, VERSION } from '@opentelemetry/core';
import { InstrumentationScope, VERSION } from '@opentelemetry/core';
import {
AggregationTemporality,
ExplicitBucketHistogramAggregation,
Expand Down Expand Up @@ -171,7 +171,7 @@ export const mockedResources: Resource[] = [
new Resource({ name: 'resource 2' }),
];

export const mockedInstrumentationLibraries: InstrumentationLibrary[] = [
export const mockedInstrumentationLibraries: InstrumentationScope[] = [
{
name: 'lib1',
version: '0.0.1',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,22 +238,22 @@ describe('OTLPMetricExporter - node with json over http', () => {
assert.ok(typeof metric1 !== 'undefined', "counter doesn't exist");
ensureCounterIsCorrect(
metric1,
core.hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[0].dataPoints[0].endTime),
core.hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[0].dataPoints[0].startTime)
core.hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[0].dataPoints[0].endTime),
core.hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[0].dataPoints[0].startTime)
);
assert.ok(typeof metric2 !== 'undefined', "observable gauge doesn't exist");
ensureObservableGaugeIsCorrect(
metric2,
core.hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].endTime),
core.hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].startTime),
core.hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].endTime),
core.hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].startTime),
6,
'double-observable-gauge2'
);
assert.ok(typeof metric3 !== 'undefined', "histogram doesn't exist");
ensureHistogramIsCorrect(
metric3,
core.hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].endTime),
core.hrTimeToNanoseconds(metrics.instrumentationLibraryMetrics[0].metrics[1].dataPoints[0].startTime),
core.hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].endTime),
core.hrTimeToNanoseconds(metrics.scopeMetrics[0].metrics[1].dataPoints[0].startTime),
[0, 100],
[0, 2, 0]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
ResourceMetrics,
InstrumentType,
DataPointType,
InstrumentationLibraryMetrics,
ScopeMetrics,
MetricData,
DataPoint,
Histogram,
Expand Down Expand Up @@ -178,15 +178,15 @@ export class PrometheusSerializer {

serialize(resourceMetrics: ResourceMetrics): string {
let str = '';
for (const instrumentationLibraryMetrics of resourceMetrics.instrumentationLibraryMetrics) {
str += this.serializeInstrumentationLibraryMetrics(instrumentationLibraryMetrics);
for (const scopeMetrics of resourceMetrics.scopeMetrics) {
str += this.serializeScopeMetrics(scopeMetrics);
}
return str;
}

serializeInstrumentationLibraryMetrics(instrumentationLibraryMetrics: InstrumentationLibraryMetrics) {
serializeScopeMetrics(scopeMetrics: ScopeMetrics) {
let str = '';
for (const metric of instrumentationLibraryMetrics.metrics) {
for (const metric of scopeMetrics.metrics) {
str += this.serializeMetricData(metric) + '\n';
}
return str;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ describe('PrometheusSerializer', () => {

const resourceMetrics = await reader.collect();
assert(resourceMetrics != null);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics.length, 1);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics[0].metrics.length, 1);
const metric = resourceMetrics.instrumentationLibraryMetrics[0].metrics[0];
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
const metric = resourceMetrics.scopeMetrics[0].metrics[0];
assert.strictEqual(metric.dataPointType, DataPointType.SINGULAR);
const pointData = metric.dataPoints as DataPoint<number>[];
assert.strictEqual(pointData.length, 1);
Expand Down Expand Up @@ -122,9 +122,9 @@ describe('PrometheusSerializer', () => {

const resourceMetrics = await reader.collect();
assert(resourceMetrics != null);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics.length, 1);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics[0].metrics.length, 1);
const metric = resourceMetrics.instrumentationLibraryMetrics[0].metrics[0];
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
const metric = resourceMetrics.scopeMetrics[0].metrics[0];
assert.strictEqual(metric.dataPointType, DataPointType.HISTOGRAM);
const pointData = metric.dataPoints as DataPoint<Histogram>[];
assert.strictEqual(pointData.length, 1);
Expand Down Expand Up @@ -180,11 +180,11 @@ describe('PrometheusSerializer', () => {

const resourceMetrics = await reader.collect();
assert(resourceMetrics != null);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics.length, 1);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics[0].metrics.length, 1);
const instrumentationLibraryMetrics = resourceMetrics.instrumentationLibraryMetrics[0];
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
const scopeMetrics = resourceMetrics.scopeMetrics[0];

const result = serializer.serializeInstrumentationLibraryMetrics(instrumentationLibraryMetrics);
const result = serializer.serializeScopeMetrics(scopeMetrics);
return result;
}

Expand Down Expand Up @@ -232,11 +232,11 @@ describe('PrometheusSerializer', () => {

const resourceMetrics = await reader.collect();
assert(resourceMetrics != null);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics.length, 1);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics[0].metrics.length, 1);
const instrumentationLibraryMetrics = resourceMetrics.instrumentationLibraryMetrics[0];
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
const scopeMetrics = resourceMetrics.scopeMetrics[0];

const result = serializer.serializeInstrumentationLibraryMetrics(instrumentationLibraryMetrics);
const result = serializer.serializeScopeMetrics(scopeMetrics);
return result;
}

Expand Down Expand Up @@ -277,9 +277,9 @@ describe('PrometheusSerializer', () => {

const resourceMetrics = await reader.collect();
assert(resourceMetrics != null);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics.length, 1);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics[0].metrics.length, 1);
const metric = resourceMetrics.instrumentationLibraryMetrics[0].metrics[0];
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
const metric = resourceMetrics.scopeMetrics[0].metrics[0];
assert.strictEqual(metric.dataPointType, DataPointType.SINGULAR);
const pointData = metric.dataPoints as DataPoint<number>[];
assert.strictEqual(pointData.length, 1);
Expand Down Expand Up @@ -316,9 +316,9 @@ describe('PrometheusSerializer', () => {

const resourceMetrics = await reader.collect();
assert(resourceMetrics != null);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics.length, 1);
assert.strictEqual(resourceMetrics.instrumentationLibraryMetrics[0].metrics.length, 1);
const metric = resourceMetrics.instrumentationLibraryMetrics[0].metrics[0];
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
const metric = resourceMetrics.scopeMetrics[0].metrics[0];
assert.strictEqual(metric.dataPointType, DataPointType.SINGULAR);
const pointData = metric.dataPoints as DataPoint<number>[];
assert.strictEqual(pointData.length, 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { HrTime } from '@opentelemetry/api';
import { MetricAttributes } from '@opentelemetry/api-metrics';
import { InstrumentationLibrary } from '@opentelemetry/core';
import { InstrumentationScope } from '@opentelemetry/core';
import { Resource } from '@opentelemetry/resources';
import { InstrumentDescriptor } from '../InstrumentDescriptor';
import { Histogram } from '../aggregator/types';
Expand Down Expand Up @@ -56,14 +56,14 @@ export interface HistogramMetricData extends BaseMetricData {
*/
export type MetricData = SingularMetricData | HistogramMetricData;

export interface InstrumentationLibraryMetrics {
instrumentationLibrary: InstrumentationLibrary;
export interface ScopeMetrics {
Copy link
Member

Choose a reason for hiding this comment

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

Is InstrumentationScopeMetrics a name to be considered? Although it is somewhat verbose, we are not introducing any new term in the case.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I found that this is the name in the OTLP proto.

Copy link
Member Author

@pichlermarc pichlermarc May 11, 2022

Choose a reason for hiding this comment

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

Oh, I found that this is the name in the OTLP proto.

I considered InstrumentationScopeMetrics at first, but I think having this to as close as possible to the OTLP proto would make sense. 🙂

I checked the other language SDKs but there does not seem to be a consensus on what to call it. Java uses this MetricData to send to the Exporter, .NET does not map the concept directly, and Python is currently in the process of changing it to ScopeMetrics. If that Python PR merges, we will have consistent naming across Python, and JS and proto. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I think its fine if we're matching the proto

scope: InstrumentationScope;
metrics: MetricData[];
}

export interface ResourceMetrics {
resource: Resource;
instrumentationLibraryMetrics: InstrumentationLibraryMetrics[];
scopeMetrics: ScopeMetrics[];
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
*/

import { HrTime } from '@opentelemetry/api';
import { hrTime, InstrumentationLibrary } from '@opentelemetry/core';
import { hrTime, InstrumentationScope } from '@opentelemetry/core';
import { Resource } from '@opentelemetry/resources';
import { instrumentationLibraryId } from '../utils';
import { instrumentationScopeId } from '../utils';
import { ViewRegistry } from '../view/ViewRegistry';
import { MeterSharedState } from './MeterSharedState';
import { MetricCollector } from './MetricCollector';
Expand All @@ -35,11 +35,11 @@ export class MeterProviderSharedState {

constructor(public resource: Resource) {}

getMeterSharedState(instrumentationLibrary: InstrumentationLibrary) {
const id = instrumentationLibraryId(instrumentationLibrary);
getMeterSharedState(instrumentationScope: InstrumentationScope) {
const id = instrumentationScopeId(instrumentationScope);
let meterSharedState = this.meterSharedStates.get(id);
if (meterSharedState == null) {
meterSharedState = new MeterSharedState(this, instrumentationLibrary);
meterSharedState = new MeterSharedState(this, instrumentationScope);
this.meterSharedStates.set(id, meterSharedState);
}
return meterSharedState;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

import { HrTime } from '@opentelemetry/api';
import * as metrics from '@opentelemetry/api-metrics';
import { InstrumentationLibrary } from '@opentelemetry/core';
import { InstrumentationLibraryMetrics } from '../export/MetricData';
import { InstrumentationScope } from '@opentelemetry/core';
import { ScopeMetrics } from '../export/MetricData';
import { createInstrumentDescriptorWithView, InstrumentDescriptor } from '../InstrumentDescriptor';
import { Meter } from '../Meter';
import { isNotNullish } from '../utils';
Expand All @@ -37,12 +37,12 @@ export class MeterSharedState {
private _observableRegistry = new ObservableRegistry();
meter: Meter;

constructor(private _meterProviderSharedState: MeterProviderSharedState, private _instrumentationLibrary: InstrumentationLibrary) {
constructor(private _meterProviderSharedState: MeterProviderSharedState, private _instrumentationScope: InstrumentationScope) {
this.meter = new Meter(this);
}

registerMetricStorage(descriptor: InstrumentDescriptor) {
const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationLibrary);
const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationScope);
const storages = views
.map(view => {
const viewDescriptor = createInstrumentDescriptorWithView(view, descriptor);
Expand All @@ -58,7 +58,7 @@ export class MeterSharedState {
}

registerAsyncMetricStorage(descriptor: InstrumentDescriptor, callback: metrics.ObservableCallback) {
const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationLibrary);
const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationScope);
views.forEach(view => {
const viewDescriptor = createInstrumentDescriptorWithView(view, descriptor);
const aggregator = view.aggregation.createAggregator(viewDescriptor);
Expand All @@ -76,7 +76,7 @@ export class MeterSharedState {
* @param collectionTime the HrTime at which the collection was initiated.
* @returns the list of {@link MetricData} collected.
*/
async collect(collector: MetricCollectorHandle, collectionTime: HrTime): Promise<InstrumentationLibraryMetrics> {
async collect(collector: MetricCollectorHandle, collectionTime: HrTime): Promise<ScopeMetrics> {
/**
* 1. Call all observable callbacks first.
* 2. Collect metric result for the collector.
Expand All @@ -93,7 +93,7 @@ export class MeterSharedState {
.filter(isNotNullish);

return {
instrumentationLibrary: this._instrumentationLibrary,
scope: this._instrumentationScope,
metrics: metricDataList.filter(isNotNullish),
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ export class MetricCollector implements MetricProducer {
const collectionTime = hrTime();
const meterCollectionPromises = Array.from(this._sharedState.meterSharedStates.values())
.map(meterSharedState => meterSharedState.collect(this, collectionTime));
const instrumentationLibraryMetrics = await Promise.all(meterCollectionPromises);
const scopeMetrics = await Promise.all(meterCollectionPromises);

return {
resource: this._sharedState.resource,
instrumentationLibraryMetrics,
scopeMetrics,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class TemporalMetricProcessor<T> {
* @param collector The information of the MetricCollector.
* @param collectors The registered collectors.
* @param resource The resource to attach these metrics against.
* @param instrumentationLibrary The instrumentation library that generated these metrics.
* @param instrumentationScope The instrumentation scope that generated these metrics.
* @param instrumentDescriptor The instrumentation descriptor that these metrics generated with.
* @param currentAccumulations The current accumulation of metric data from instruments.
* @param sdkStartTime The sdk start timestamp.
Expand Down
Loading