From e618758addc5c70f7ad97e9a8fc1668c41ab7d25 Mon Sep 17 00:00:00 2001
From: "marc.pichler" <marc.pichler@dynatrace.com>
Date: Mon, 24 Jan 2022 16:52:23 +0100
Subject: [PATCH 1/6] feat(views): handle view conflicts.

---
 .../src/InstrumentDescriptor.ts               |  32 ++++
 .../src/Meter.ts                              |  32 ++--
 .../src/state/AsyncMetricStorage.ts           |   4 +
 .../src/state/MetricStorage.ts                |   3 +
 .../src/state/MetricStorageRegistry.ts        |  65 ++++++++
 .../src/state/SyncMetricStorage.ts            |   4 +
 .../test/state/MetricStorageRegistry.test.ts  | 145 ++++++++++++++++++
 7 files changed, 269 insertions(+), 16 deletions(-)
 create mode 100644 experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
 create mode 100644 experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.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 8aa57c9f68d..7a2edfd1d78 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
@@ -46,3 +46,35 @@ export function createInstrumentDescriptorWithView(view: View, instrument: Instr
     valueType: instrument.valueType,
   };
 }
+
+export function isDescriptorCompatibleWith(descriptor: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  return descriptor.name === otherDescriptor.name
+    && descriptor.description === otherDescriptor.description
+    && descriptor.unit === otherDescriptor.unit
+    && descriptor.type === otherDescriptor.type
+    && descriptor.valueType === otherDescriptor.valueType;
+}
+
+export function getDescriptorIncompatibilityDetails(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  let incompatibility = '';
+  if (existing.description !== otherDescriptor.description) {
+    incompatibility += `\n- Description '${existing.description}' does not match '${otherDescriptor.description}'`;
+  }
+  if (existing.unit !== otherDescriptor.unit) {
+    incompatibility += `\n- Unit '${existing.unit}' does not match '${otherDescriptor.description}'`;
+  }
+  if (existing.type !== otherDescriptor.type) {
+    incompatibility += `\n- Type '${existing.type}' does not match '${otherDescriptor.type}'`;
+  }
+  if (existing.valueType !== otherDescriptor.valueType) {
+    incompatibility += `\n- Value Type '${existing.valueType}' does not match '${otherDescriptor.valueType}'`;
+  }
+
+  return incompatibility;
+}
+
+export function isDescriptorAsync(descriptor: InstrumentDescriptor) {
+  return (descriptor.type === InstrumentType.OBSERVABLE_GAUGE ||
+    descriptor.type === InstrumentType.OBSERVABLE_UP_DOWN_COUNTER ||
+    descriptor.type === InstrumentType.OBSERVABLE_COUNTER);
+}
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
index 7808db28e26..ae6728c4277 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
@@ -16,22 +16,25 @@
 
 import * as metrics from '@opentelemetry/api-metrics-wip';
 import { InstrumentationLibrary } from '@opentelemetry/core';
-import { createInstrumentDescriptor, InstrumentDescriptor } from './InstrumentDescriptor';
+import {
+  createInstrumentDescriptor, InstrumentDescriptor
+} from './InstrumentDescriptor';
 import { Counter, Histogram, InstrumentType, UpDownCounter } from './Instruments';
 import { MeterProviderSharedState } from './state/MeterProviderSharedState';
 import { MultiMetricStorage } from './state/MultiWritableMetricStorage';
 import { SyncMetricStorage } from './state/SyncMetricStorage';
-import { MetricStorage } from './state/MetricStorage';
 import { MetricData } from './export/MetricData';
 import { isNotNullish } from './utils';
 import { MetricCollectorHandle } from './state/MetricCollector';
-import { HrTime } from '@opentelemetry/api';
 import { AsyncMetricStorage } from './state/AsyncMetricStorage';
+import { HrTime } from '@opentelemetry/api';
+import { WritableMetricStorage } from './state/WritableMetricStorage';
+import { MetricStorageRegistry } from './state/MetricStorageRegistry';
 
 // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#meter
 
 export class Meter implements metrics.Meter {
-  private _metricStorageRegistry = new Map<string, MetricStorage>();
+  private _metricStorageRegistry = new MetricStorageRegistry();
 
   // instrumentation library required by spec to be on meter
   // spec requires provider config changes to apply to previously created meters, achieved by holding a reference to the provider
@@ -89,31 +92,28 @@ export class Meter implements metrics.Meter {
     this._registerAsyncMetricStorage(descriptor, callback);
   }
 
-  private _registerMetricStorage(descriptor: InstrumentDescriptor) {
+  private _registerMetricStorage(descriptor: InstrumentDescriptor): WritableMetricStorage {
     const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationLibrary);
-    const storages = views.map(view => {
-      const storage = SyncMetricStorage.create(view, descriptor);
-      // TODO: handle conflicts
-      this._metricStorageRegistry.set(descriptor.name, storage);
-      return storage;
-    });
-    if (storages.length === 1)  {
+    const storages = views.map(view => this._metricStorageRegistry.register(SyncMetricStorage.create(view, descriptor)))
+      .filter(isNotNullish);
+
+    if (storages.length === 1) {
       return storages[0];
     }
+
+    // This will be a no-op WritableMetricStorage when length is null.
     return new MultiMetricStorage(storages);
   }
 
   private _registerAsyncMetricStorage(descriptor: InstrumentDescriptor, callback: metrics.ObservableCallback) {
     const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationLibrary);
     views.forEach(view => {
-      const storage = AsyncMetricStorage.create(view, descriptor, callback);
-      // TODO: handle conflicts
-      this._metricStorageRegistry.set(descriptor.name, storage);
+      this._metricStorageRegistry.register(AsyncMetricStorage.create(view, descriptor, callback));
     });
   }
 
   async collect(collector: MetricCollectorHandle, collectionTime: HrTime): Promise<MetricData[]> {
-    const result = await Promise.all(Array.from(this._metricStorageRegistry.values()).map(metricStorage => {
+    const result = await Promise.all(Array.from(this._metricStorageRegistry.getStorages()).map(metricStorage => {
       return metricStorage.collect(
         collector,
         this._meterProviderSharedState.metricCollectors,
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts
index d98ececf7fd..6885dca620f 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts
@@ -50,6 +50,10 @@ export class AsyncMetricStorage<T extends Maybe<Accumulation>> implements Metric
     this._temporalMetricStorage = new TemporalMetricProcessor(aggregator);
   }
 
+  getInstrumentDescriptor(): InstrumentDescriptor {
+    return this._instrumentDescriptor;
+  }
+
   private _record(measurements: AttributeHashMap<number>) {
     const processed = new AttributeHashMap<number>();
     Array.from(measurements.entries()).forEach(([attributes, value]) => {
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts
index 7369800bd04..13b25516b2b 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts
@@ -20,6 +20,7 @@ import { Resource } from '@opentelemetry/resources';
 import { MetricData } from '../export/MetricData';
 import { Maybe } from '../utils';
 import { MetricCollectorHandle } from './MetricCollector';
+import { InstrumentDescriptor } from '../InstrumentDescriptor';
 
 /**
  * Internal interface.
@@ -41,4 +42,6 @@ export interface MetricStorage {
     sdkStartTime: HrTime,
     collectionTime: HrTime,
   ): Promise<Maybe<MetricData>>;
+
+  getInstrumentDescriptor(): InstrumentDescriptor;
 }
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
new file mode 100644
index 00000000000..2d7c3f1e9dc
--- /dev/null
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
@@ -0,0 +1,65 @@
+/*
+ * 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 { MetricStorage } from './MetricStorage';
+import {
+  getDescriptorIncompatibilityDetails, isDescriptorAsync,
+  isDescriptorCompatibleWith
+} from '../InstrumentDescriptor';
+import * as api from '@opentelemetry/api';
+import { Maybe } from '../utils';
+
+/**
+ * Internal class for storing @{LinkMetricStorage}
+ */
+export class MetricStorageRegistry {
+  private readonly _metricStorageRegistry = new Map<string, MetricStorage>();
+
+  getStorages(): IterableIterator<MetricStorage> {
+    return this._metricStorageRegistry.values();
+  }
+
+  register<T extends MetricStorage>(storage: T): Maybe<T> {
+    const expectedDescriptor = storage.getInstrumentDescriptor();
+
+    // create and register a new one if it does not exist yet.
+    if (!this._metricStorageRegistry.has(expectedDescriptor.name)) {
+      this._metricStorageRegistry.set(expectedDescriptor.name, storage);
+      return storage;
+    }
+
+    // We already checked for existence with has()
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    const existingStorage = this._metricStorageRegistry.get(expectedDescriptor.name)!;
+    const existingDescriptor = existingStorage.getInstrumentDescriptor();
+
+    // Return storage if it is compatible.
+    if (isDescriptorCompatibleWith(existingDescriptor, expectedDescriptor)) {
+      // Is compatible, but views for async instruments cannot be registered twice.
+      if(isDescriptorAsync(existingDescriptor)) {
+        api.diag.warn(`A view for an async instrument with the name '${expectedDescriptor.name}' has already been registered.`);
+        return undefined;
+      }
+
+      return (existingStorage as T);
+    }
+
+    // Warn and return undefined if it is incompatible.
+    api.diag.warn(`A view or instrument with the name '${expectedDescriptor.name}' has already been registered and is incompatible with another registered view.\nDetails:\n`,
+      getDescriptorIncompatibilityDetails(existingDescriptor, expectedDescriptor));
+    return undefined;
+  }
+}
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts
index 609f0fc5858..9ed25a03aee 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts
@@ -48,6 +48,10 @@ export class SyncMetricStorage<T extends Maybe<Accumulation>> implements Writabl
     this._temporalMetricStorage = new TemporalMetricProcessor(aggregator);
   }
 
+  getInstrumentDescriptor(): InstrumentDescriptor {
+    return this._instrumentDescriptor;
+  }
+
   record(value: number, attributes: Attributes, context: Context) {
     attributes = this._attributesProcessor.process(attributes, context);
     this._deltaMetricStorage.record(value, attributes, context);
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
new file mode 100644
index 00000000000..5f46f368841
--- /dev/null
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
@@ -0,0 +1,145 @@
+/*
+ * 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 { MetricStorageRegistry } from '../../src/state/MetricStorageRegistry';
+import { InstrumentType } from '../../src/Instruments';
+import { ValueType } from '@opentelemetry/api-metrics-wip';
+import { MetricStorage } from '../../src/state/MetricStorage';
+import { HrTime } from '@opentelemetry/api';
+import { InstrumentationLibrary } from '@opentelemetry/core';
+import { Resource } from '@opentelemetry/resources';
+import { MetricData } from '../../src/export/MetricData';
+import { MetricCollectorHandle } from '../../src/state/MetricCollector';
+import { InstrumentDescriptor } from '../../src/InstrumentDescriptor';
+import { Maybe } from '../../src/utils';
+import * as assert from 'assert';
+
+
+class TestMetricStorage implements MetricStorage {
+  constructor(readonly _descriptor: InstrumentDescriptor) {
+  }
+
+  collect(collector: MetricCollectorHandle,
+          collectors: MetricCollectorHandle[],
+          resource: Resource,
+          instrumentationLibrary: InstrumentationLibrary,
+          sdkStartTime: HrTime,
+          collectionTime: HrTime,
+  ): Promise<Maybe<MetricData>> {
+    return Promise.resolve(undefined);
+  }
+
+  getInstrumentDescriptor(): InstrumentDescriptor {
+    return this._descriptor;
+  }
+}
+
+describe('MetricStorageRegistry', () => {
+  describe('register', () => {
+    it('should register MetricStorage if it does not exist', () => {
+      const registry = new MetricStorageRegistry();
+      const storage = new TestMetricStorage({
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      });
+
+      const registeredStorage = registry.register(storage);
+      const registeredStorages = Array.from(registry.getStorages());
+
+      // returned the same storage
+      assert.strictEqual(registeredStorage, storage);
+      // registered the actual storage
+      assert.deepStrictEqual([storage], registeredStorages);
+    });
+
+    it('should not register when incompatible instrument is already registered', () => {
+      const registry = new MetricStorageRegistry();
+      const storage = new TestMetricStorage({
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      });
+
+      const otherStorage = new TestMetricStorage({
+        name: 'instrument',
+        type: InstrumentType.UP_DOWN_COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      });
+
+      registry.register(storage);
+      const failedRegisteredStorage = registry.register(otherStorage);
+      const registeredStorages = Array.from(registry.getStorages());
+
+      // returned undefined
+      assert.strictEqual(failedRegisteredStorage, undefined);
+      // registered the actual storage, but not more than that.
+      assert.deepStrictEqual([storage], registeredStorages);
+    });
+
+    it('should not register when compatible async instrument is already registered', () => {
+      const registry = new MetricStorageRegistry();
+      const descriptor = {
+        name: 'instrument',
+        type: InstrumentType.OBSERVABLE_COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      };
+
+      const storage = new TestMetricStorage(descriptor);
+      const otherStorage = new TestMetricStorage(descriptor);
+
+      registry.register(storage);
+      const failedRegisteredStorage = registry.register(otherStorage);
+      const registeredStorages = Array.from(registry.getStorages());
+
+      // returned undefined
+      assert.strictEqual(failedRegisteredStorage, undefined);
+      // registered the actual storage, but not more than that.
+      assert.deepStrictEqual([storage], registeredStorages);
+    });
+
+    it('should return the existing instrument if a compatible sync instrument is already registered', () => {
+      const registry = new MetricStorageRegistry();
+      const descriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      };
+
+      const storage = new TestMetricStorage(descriptor);
+      const otherStorage = new TestMetricStorage(descriptor);
+
+      registry.register(storage);
+      const previouslyRegisteredStorage = registry.register(otherStorage);
+      const registeredStorages = Array.from(registry.getStorages());
+
+      // returned undefined
+      assert.strictEqual(previouslyRegisteredStorage, storage);
+      // registered the actual storage, but not more than that.
+      assert.deepStrictEqual([storage], registeredStorages);
+    });
+  });
+});

From 052bc02969bee9214e6d699d47861e2be7a9e519 Mon Sep 17 00:00:00 2001
From: "marc.pichler" <marc.pichler@dynatrace.com>
Date: Tue, 25 Jan 2022 16:39:08 +0100
Subject: [PATCH 2/6] fix(views): remove description as a cause of instrument
 incompatibility.

---
 .../src/InstrumentDescriptor.ts                       | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
index 7a2edfd1d78..4d844596105 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
@@ -48,8 +48,8 @@ export function createInstrumentDescriptorWithView(view: View, instrument: Instr
 }
 
 export function isDescriptorCompatibleWith(descriptor: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  // description is ignored as it is not semantic in nature.
   return descriptor.name === otherDescriptor.name
-    && descriptor.description === otherDescriptor.description
     && descriptor.unit === otherDescriptor.unit
     && descriptor.type === otherDescriptor.type
     && descriptor.valueType === otherDescriptor.valueType;
@@ -57,17 +57,14 @@ export function isDescriptorCompatibleWith(descriptor: InstrumentDescriptor, oth
 
 export function getDescriptorIncompatibilityDetails(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
   let incompatibility = '';
-  if (existing.description !== otherDescriptor.description) {
-    incompatibility += `\n- Description '${existing.description}' does not match '${otherDescriptor.description}'`;
-  }
   if (existing.unit !== otherDescriptor.unit) {
-    incompatibility += `\n- Unit '${existing.unit}' does not match '${otherDescriptor.description}'`;
+    incompatibility += `\t- Unit '${existing.unit}' does not match '${otherDescriptor.description}'\n`;
   }
   if (existing.type !== otherDescriptor.type) {
-    incompatibility += `\n- Type '${existing.type}' does not match '${otherDescriptor.type}'`;
+    incompatibility += `\t- Type '${existing.type}' does not match '${otherDescriptor.type}'\n`;
   }
   if (existing.valueType !== otherDescriptor.valueType) {
-    incompatibility += `\n- Value Type '${existing.valueType}' does not match '${otherDescriptor.valueType}'`;
+    incompatibility += `\t- Value Type '${existing.valueType}' does not match '${otherDescriptor.valueType}'\n`;
   }
 
   return incompatibility;

From 4c60202d25f17f98de6cb127f4d919f4dbdb8f68 Mon Sep 17 00:00:00 2001
From: "marc.pichler" <marc.pichler@dynatrace.com>
Date: Tue, 25 Jan 2022 16:39:32 +0100
Subject: [PATCH 3/6] fix(view): throw on incompatible insturments.

---
 .../src/state/MetricStorageRegistry.ts               | 12 +++++++-----
 .../test/state/MetricStorageRegistry.test.ts         |  6 ++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
index 2d7c3f1e9dc..e7fec5fe20c 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
@@ -49,7 +49,7 @@ export class MetricStorageRegistry {
     // Return storage if it is compatible.
     if (isDescriptorCompatibleWith(existingDescriptor, expectedDescriptor)) {
       // Is compatible, but views for async instruments cannot be registered twice.
-      if(isDescriptorAsync(existingDescriptor)) {
+      if (isDescriptorAsync(existingDescriptor)) {
         api.diag.warn(`A view for an async instrument with the name '${expectedDescriptor.name}' has already been registered.`);
         return undefined;
       }
@@ -57,9 +57,11 @@ export class MetricStorageRegistry {
       return (existingStorage as T);
     }
 
-    // Warn and return undefined if it is incompatible.
-    api.diag.warn(`A view or instrument with the name '${expectedDescriptor.name}' has already been registered and is incompatible with another registered view.\nDetails:\n`,
-      getDescriptorIncompatibilityDetails(existingDescriptor, expectedDescriptor));
-    return undefined;
+    // Throw an error if it is not compatible.
+    throw new Error('A view or instrument with the name'
+      + expectedDescriptor.name
+      + 'has already been registered and is incompatible with another registered view.\n'
+      + 'Details:\n'
+      + getDescriptorIncompatibilityDetails(existingDescriptor, expectedDescriptor));
   }
 }
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
index 5f46f368841..0bc6548f05f 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
@@ -68,7 +68,7 @@ describe('MetricStorageRegistry', () => {
       assert.deepStrictEqual([storage], registeredStorages);
     });
 
-    it('should not register when incompatible instrument is already registered', () => {
+    it('should throw when incompatible instrument is already registered', () => {
       const registry = new MetricStorageRegistry();
       const storage = new TestMetricStorage({
         name: 'instrument',
@@ -87,11 +87,9 @@ describe('MetricStorageRegistry', () => {
       });
 
       registry.register(storage);
-      const failedRegisteredStorage = registry.register(otherStorage);
+      assert.throws(() => registry.register(otherStorage));
       const registeredStorages = Array.from(registry.getStorages());
 
-      // returned undefined
-      assert.strictEqual(failedRegisteredStorage, undefined);
       // registered the actual storage, but not more than that.
       assert.deepStrictEqual([storage], registeredStorages);
     });

From 9300f523253f89ab37dde251a61b3c78cad1801b Mon Sep 17 00:00:00 2001
From: "marc.pichler" <marc.pichler@dynatrace.com>
Date: Wed, 16 Feb 2022 17:26:12 +0100
Subject: [PATCH 4/6] feat(views): update implementation current proposed spec.

---
 .../src/InstrumentDescriptor.ts               |  24 +--
 .../src/Meter.ts                              |   2 +-
 .../src/state/AsyncMetricStorage.ts           |  14 +-
 .../src/state/MetricStorage.ts                |  24 ++-
 .../src/state/MetricStorageRegistry.ts        |  82 ++++++---
 .../src/state/SyncMetricStorage.ts            |  14 +-
 .../src/view/RegistrationConflicts.ts         |  91 ++++++++++
 .../test/state/MetricStorageRegistry.test.ts  | 167 +++++++++++++++---
 8 files changed, 322 insertions(+), 96 deletions(-)
 create mode 100644 experimental/packages/opentelemetry-sdk-metrics-base/src/view/RegistrationConflicts.ts

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
index 1508716e65d..8b4347e3e40 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
@@ -20,7 +20,7 @@ import { View } from './view/View';
 /**
  * Supported types of metric instruments.
  */
- export enum InstrumentType {
+export enum InstrumentType {
   COUNTER = 'COUNTER',
   HISTOGRAM = 'HISTOGRAM',
   UP_DOWN_COUNTER = 'UP_DOWN_COUNTER',
@@ -58,30 +58,8 @@ export function createInstrumentDescriptorWithView(view: View, instrument: Instr
 }
 
 export function isDescriptorCompatibleWith(descriptor: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
-  // description is ignored as it is not semantic in nature.
   return descriptor.name === otherDescriptor.name
     && descriptor.unit === otherDescriptor.unit
     && descriptor.type === otherDescriptor.type
     && descriptor.valueType === otherDescriptor.valueType;
 }
-
-export function getDescriptorIncompatibilityDetails(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
-  let incompatibility = '';
-  if (existing.unit !== otherDescriptor.unit) {
-    incompatibility += `\t- Unit '${existing.unit}' does not match '${otherDescriptor.description}'\n`;
-  }
-  if (existing.type !== otherDescriptor.type) {
-    incompatibility += `\t- Type '${existing.type}' does not match '${otherDescriptor.type}'\n`;
-  }
-  if (existing.valueType !== otherDescriptor.valueType) {
-    incompatibility += `\t- Value Type '${existing.valueType}' does not match '${otherDescriptor.valueType}'\n`;
-  }
-
-  return incompatibility;
-}
-
-export function isDescriptorAsync(descriptor: InstrumentDescriptor) {
-  return (descriptor.type === InstrumentType.OBSERVABLE_GAUGE ||
-    descriptor.type === InstrumentType.OBSERVABLE_UP_DOWN_COUNTER ||
-    descriptor.type === InstrumentType.OBSERVABLE_COUNTER);
-}
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
index edf9466a87a..310ead8fff5 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
@@ -129,7 +129,7 @@ export class Meter implements metrics.Meter {
    * @returns the list of {@link MetricData} collected.
    */
   async collect(collector: MetricCollectorHandle, collectionTime: HrTime): Promise<MetricData[]> {
-    const result = await Promise.all(Array.from(this._metricStorageRegistry.getStorages()).map(metricStorage => {
+    const result = await Promise.all(this._metricStorageRegistry.getStorages().map(metricStorage => {
       return metricStorage.collect(
         collector,
         this._meterProviderSharedState.metricCollectors,
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts
index 6885dca620f..a620d75f6d5 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/AsyncMetricStorage.ts
@@ -18,7 +18,10 @@ import { HrTime } from '@opentelemetry/api';
 import { ObservableCallback } from '@opentelemetry/api-metrics-wip';
 import { Accumulation, Aggregator } from '../aggregator/types';
 import { View } from '../view/View';
-import { createInstrumentDescriptorWithView, InstrumentDescriptor } from '../InstrumentDescriptor';
+import {
+  createInstrumentDescriptorWithView,
+  InstrumentDescriptor
+} from '../InstrumentDescriptor';
 import { AttributesProcessor } from '../view/AttributesProcessor';
 import { MetricStorage } from './MetricStorage';
 import { InstrumentationLibrary } from '@opentelemetry/core';
@@ -36,24 +39,21 @@ import { AttributeHashMap } from './HashMap';
  *
  * Stores and aggregates {@link MetricData} for asynchronous instruments.
  */
-export class AsyncMetricStorage<T extends Maybe<Accumulation>> implements MetricStorage {
+export class AsyncMetricStorage<T extends Maybe<Accumulation>> extends MetricStorage {
   private _deltaMetricStorage: DeltaMetricProcessor<T>;
   private _temporalMetricStorage: TemporalMetricProcessor<T>;
 
   constructor(
-    private _instrumentDescriptor: InstrumentDescriptor,
+    _instrumentDescriptor: InstrumentDescriptor,
     aggregator: Aggregator<T>,
     private _attributesProcessor: AttributesProcessor,
     private _callback: ObservableCallback
   ) {
+    super(_instrumentDescriptor);
     this._deltaMetricStorage = new DeltaMetricProcessor(aggregator);
     this._temporalMetricStorage = new TemporalMetricProcessor(aggregator);
   }
 
-  getInstrumentDescriptor(): InstrumentDescriptor {
-    return this._instrumentDescriptor;
-  }
-
   private _record(measurements: AttributeHashMap<number>) {
     const processed = new AttributeHashMap<number>();
     Array.from(measurements.entries()).forEach(([attributes, value]) => {
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts
index 13b25516b2b..4623ffab079 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorage.ts
@@ -20,21 +20,24 @@ import { Resource } from '@opentelemetry/resources';
 import { MetricData } from '../export/MetricData';
 import { Maybe } from '../utils';
 import { MetricCollectorHandle } from './MetricCollector';
-import { InstrumentDescriptor } from '../InstrumentDescriptor';
+import { createInstrumentDescriptor, InstrumentDescriptor } from '../InstrumentDescriptor';
 
 /**
  * Internal interface.
  *
  * Represents a storage from which we can collect metrics.
  */
-export interface MetricStorage {
+export abstract class MetricStorage {
+  constructor(protected _instrumentDescriptor: InstrumentDescriptor) {
+  }
+
   /**
    * Collects the metrics from this storage.
    *
    * Note: This is a stateful operation and may reset any interval-related
    * state for the MetricCollector.
    */
-  collect(
+  abstract collect(
     collector: MetricCollectorHandle,
     collectors: MetricCollectorHandle[],
     resource: Resource,
@@ -43,5 +46,18 @@ export interface MetricStorage {
     collectionTime: HrTime,
   ): Promise<Maybe<MetricData>>;
 
-  getInstrumentDescriptor(): InstrumentDescriptor;
+  getInstrumentDescriptor(): InstrumentDescriptor{
+    return this._instrumentDescriptor;
+  }
+
+  updateDescription(description: string): void{
+    this._instrumentDescriptor = createInstrumentDescriptor(
+      this._instrumentDescriptor.name,
+      this._instrumentDescriptor.type,
+      {
+        description: description,
+        valueType: this._instrumentDescriptor.valueType,
+        unit: this._instrumentDescriptor.unit
+      });
+  }
 }
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
index e7fec5fe20c..a761e29b6a4 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
@@ -15,53 +15,79 @@
  */
 
 import { MetricStorage } from './MetricStorage';
-import {
-  getDescriptorIncompatibilityDetails, isDescriptorAsync,
-  isDescriptorCompatibleWith
-} from '../InstrumentDescriptor';
+import { isDescriptorCompatibleWith } from '../InstrumentDescriptor';
 import * as api from '@opentelemetry/api';
 import { Maybe } from '../utils';
+import { getConflictResolutionRecipe, getIncompatibilityDetails } from '../view/RegistrationConflicts';
 
 /**
- * Internal class for storing @{LinkMetricStorage}
+ * Internal class for storing {@link MetricStorage}
  */
 export class MetricStorageRegistry {
-  private readonly _metricStorageRegistry = new Map<string, MetricStorage>();
+  private readonly _metricStorageRegistry = new Map<string, MetricStorage[]>();
 
-  getStorages(): IterableIterator<MetricStorage> {
-    return this._metricStorageRegistry.values();
+  getStorages(): MetricStorage[] {
+    const storages = [];
+    for (const metricStorages of Array.from(this._metricStorageRegistry.values())) {
+      storages.push(...metricStorages);
+    }
+
+    return storages;
   }
 
   register<T extends MetricStorage>(storage: T): Maybe<T> {
     const expectedDescriptor = storage.getInstrumentDescriptor();
+    const existingStorages = this._metricStorageRegistry.get(expectedDescriptor.name);
 
-    // create and register a new one if it does not exist yet.
-    if (!this._metricStorageRegistry.has(expectedDescriptor.name)) {
-      this._metricStorageRegistry.set(expectedDescriptor.name, storage);
+    // Add storage if it does not exist.
+    if (existingStorages === undefined) {
+      this._metricStorageRegistry.set(expectedDescriptor.name, [storage]);
       return storage;
     }
 
-    // We already checked for existence with has()
-    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-    const existingStorage = this._metricStorageRegistry.get(expectedDescriptor.name)!;
-    const existingDescriptor = existingStorage.getInstrumentDescriptor();
+    const compatibleStorages = [];
+
+    for (const existingStorage of existingStorages) {
+      const existingDescriptor = existingStorage.getInstrumentDescriptor();
 
-    // Return storage if it is compatible.
-    if (isDescriptorCompatibleWith(existingDescriptor, expectedDescriptor)) {
-      // Is compatible, but views for async instruments cannot be registered twice.
-      if (isDescriptorAsync(existingDescriptor)) {
-        api.diag.warn(`A view for an async instrument with the name '${expectedDescriptor.name}' has already been registered.`);
-        return undefined;
+      if (isDescriptorCompatibleWith(existingDescriptor, expectedDescriptor)) {
+        // Use the longer description if it does not match.
+        if (existingDescriptor.description !== expectedDescriptor.description) {
+          if (expectedDescriptor.description.length > existingDescriptor.description.length) {
+            existingStorage.updateDescription(expectedDescriptor.description);
+          }
+
+          api.diag.warn('A view or instrument with the name ',
+            expectedDescriptor.name,
+            ' has already been registered, but has a different description and is incompatible with another registered view.\n',
+            'Details:\n',
+            getIncompatibilityDetails(existingDescriptor, expectedDescriptor),
+            'The longer description will be used.\nTo resolve the conflict:',
+            getConflictResolutionRecipe(existingDescriptor, expectedDescriptor));
+        }
+        // Storage is fully compatible.
+        compatibleStorages.push(existingStorage as T);
+      } else {
+        // The implementation SHOULD warn about duplicate instrument registration
+        // conflicts after applying View configuration.
+        api.diag.warn('A view or instrument with the name ',
+          expectedDescriptor.name,
+          ' has already been registered and is incompatible with another registered view.\n',
+          'Details:\n',
+          getIncompatibilityDetails(existingDescriptor, expectedDescriptor),
+          'To resolve the conflict:\n',
+          getConflictResolutionRecipe(existingDescriptor, expectedDescriptor));
       }
+    }
 
-      return (existingStorage as T);
+    // When one compatible storage is already present, another compatible one will not be pushed.
+    // Therefore this will never be > 1
+    if (compatibleStorages.length > 0) {
+      return compatibleStorages[0];
     }
 
-    // Throw an error if it is not compatible.
-    throw new Error('A view or instrument with the name'
-      + expectedDescriptor.name
-      + 'has already been registered and is incompatible with another registered view.\n'
-      + 'Details:\n'
-      + getDescriptorIncompatibilityDetails(existingDescriptor, expectedDescriptor));
+    // None of the storages were compatible, add the current one to the list.
+    existingStorages.push(storage);
+    return storage;
   }
 }
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts
index 9ed25a03aee..986448b53ba 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/SyncMetricStorage.ts
@@ -19,7 +19,10 @@ import { Attributes } from '@opentelemetry/api-metrics-wip';
 import { WritableMetricStorage } from './WritableMetricStorage';
 import { Accumulation, Aggregator } from '../aggregator/types';
 import { View } from '../view/View';
-import { createInstrumentDescriptorWithView, InstrumentDescriptor } from '../InstrumentDescriptor';
+import {
+  createInstrumentDescriptorWithView,
+  InstrumentDescriptor
+} from '../InstrumentDescriptor';
 import { AttributesProcessor } from '../view/AttributesProcessor';
 import { MetricStorage } from './MetricStorage';
 import { InstrumentationLibrary } from '@opentelemetry/core';
@@ -35,23 +38,20 @@ import { MetricCollectorHandle } from './MetricCollector';
  *
  * Stores and aggregates {@link MetricData} for synchronous instruments.
  */
-export class SyncMetricStorage<T extends Maybe<Accumulation>> implements WritableMetricStorage, MetricStorage {
+export class SyncMetricStorage<T extends Maybe<Accumulation>> extends MetricStorage implements WritableMetricStorage {
   private _deltaMetricStorage: DeltaMetricProcessor<T>;
   private _temporalMetricStorage: TemporalMetricProcessor<T>;
 
   constructor(
-    private _instrumentDescriptor: InstrumentDescriptor,
+    instrumentDescriptor: InstrumentDescriptor,
     aggregator: Aggregator<T>,
     private _attributesProcessor: AttributesProcessor
   ) {
+    super(instrumentDescriptor);
     this._deltaMetricStorage = new DeltaMetricProcessor(aggregator);
     this._temporalMetricStorage = new TemporalMetricProcessor(aggregator);
   }
 
-  getInstrumentDescriptor(): InstrumentDescriptor {
-    return this._instrumentDescriptor;
-  }
-
   record(value: number, attributes: Attributes, context: Context) {
     attributes = this._attributesProcessor.process(attributes, context);
     this._deltaMetricStorage.record(value, attributes, context);
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/view/RegistrationConflicts.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/view/RegistrationConflicts.ts
new file mode 100644
index 00000000000..bc0063fe21a
--- /dev/null
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/view/RegistrationConflicts.ts
@@ -0,0 +1,91 @@
+/*
+ * 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 { InstrumentSelectorCriteria } from './InstrumentSelector';
+import { InstrumentDescriptor } from '../InstrumentDescriptor';
+
+export function getIncompatibilityDetails(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  let incompatibility = '';
+  if (existing.unit !== otherDescriptor.unit) {
+    incompatibility += `\t- Unit '${existing.unit}' does not match '${otherDescriptor.unit}'\n`;
+  }
+  if (existing.type !== otherDescriptor.type) {
+    incompatibility += `\t- Type '${existing.type}' does not match '${otherDescriptor.type}'\n`;
+  }
+  if (existing.valueType !== otherDescriptor.valueType) {
+    incompatibility += `\t- Value Type '${existing.valueType}' does not match '${otherDescriptor.valueType}'\n`;
+  }
+  if (existing.description !== otherDescriptor.description) {
+    incompatibility += `\t- Description '${existing.description}' does not match '${otherDescriptor.description}'\n`;
+  }
+
+  return incompatibility;
+}
+
+export function getValueTypeConflictResolutionRecipe(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  return `\t- use valueType '${existing.valueType}' on instrument creation or use an instrument name other than '${otherDescriptor.name}'`;
+}
+
+export function getUnitConflictResolutionRecipe(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  return `\t- use unit '${existing.unit}' on instrument creation or use an instrument name other than '${otherDescriptor.name}'`;
+}
+
+export function getTypeConflictResolutionRecipe(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) {
+  const selector: InstrumentSelectorCriteria = {
+    name: otherDescriptor.name,
+    type: otherDescriptor.type
+  };
+
+  const selectorString = JSON.stringify(selector);
+
+  return `\t- create a new view with a name other than '${existing.name}' and InstrumentSelector '${selectorString}'`;
+}
+
+export function getDescriptionResolutionRecipe(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor): string {
+  const selector: InstrumentSelectorCriteria = {
+    name: otherDescriptor.name,
+    type: otherDescriptor.type
+  };
+
+  const selectorString = JSON.stringify(selector);
+
+  return `\t- create a new view with a name other than '${existing.name}' and InstrumentSelector '${selectorString}'
+    \t- OR - create a new view with the name ${existing.name} and description '${existing.description}' and InstrumentSelector ${selectorString}
+    \t- OR - create a new view with the name ${otherDescriptor.name} and description '${existing.description}' and InstrumentSelector ${selectorString}`;
+}
+
+export function getConflictResolutionRecipe(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor): string {
+  // Conflicts that cannot be solved via views.
+  if (existing.valueType !== otherDescriptor.valueType) {
+    return getValueTypeConflictResolutionRecipe(existing, otherDescriptor);
+  }
+
+  if (existing.unit !== otherDescriptor.unit) {
+    return getUnitConflictResolutionRecipe(existing, otherDescriptor);
+  }
+
+  // Conflicts that can be solved via views.
+  if (existing.type !== otherDescriptor.type) {
+    // this will automatically solve possible description conflicts.
+    return getTypeConflictResolutionRecipe(existing, otherDescriptor);
+  }
+
+  if (existing.description !== otherDescriptor.description) {
+    return getDescriptionResolutionRecipe(existing, otherDescriptor);
+  }
+
+  return '';
+}
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
index 3e8f271c7a9..f0ca6e67194 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricStorageRegistry.test.ts
@@ -23,13 +23,16 @@ import { Resource } from '@opentelemetry/resources';
 import { MetricCollectorHandle } from '../../src/state/MetricCollector';
 import { MetricData, InstrumentDescriptor, InstrumentType } from '../../src';
 import { Maybe } from '../../src/utils';
+import * as api from '@opentelemetry/api';
 import * as assert from 'assert';
+import * as sinon from 'sinon';
+import {
+  getDescriptionResolutionRecipe,
+  getTypeConflictResolutionRecipe, getUnitConflictResolutionRecipe,
+  getValueTypeConflictResolutionRecipe
+} from '../../src/view/RegistrationConflicts';
 
-
-class TestMetricStorage implements MetricStorage {
-  constructor(readonly _descriptor: InstrumentDescriptor) {
-  }
-
+class TestMetricStorage extends MetricStorage {
   collect(collector: MetricCollectorHandle,
           collectors: MetricCollectorHandle[],
           resource: Resource,
@@ -39,13 +42,19 @@ class TestMetricStorage implements MetricStorage {
   ): Promise<Maybe<MetricData>> {
     return Promise.resolve(undefined);
   }
-
-  getInstrumentDescriptor(): InstrumentDescriptor {
-    return this._descriptor;
-  }
 }
 
 describe('MetricStorageRegistry', () => {
+  let spyLoggerWarn: sinon.SinonStub<[message: string, ...args: unknown[]], void>;
+
+  beforeEach(() => {
+    spyLoggerWarn = sinon.stub(api.diag, 'warn');
+  });
+
+  afterEach(() => {
+    sinon.restore();
+  });
+
   describe('register', () => {
     it('should register MetricStorage if it does not exist', () => {
       const registry = new MetricStorageRegistry();
@@ -58,41 +67,141 @@ describe('MetricStorageRegistry', () => {
       });
 
       const registeredStorage = registry.register(storage);
-      const registeredStorages = Array.from(registry.getStorages());
+      const registeredStorages = registry.getStorages();
 
       // returned the same storage
       assert.strictEqual(registeredStorage, storage);
       // registered the actual storage
       assert.deepStrictEqual([storage], registeredStorages);
+      // no warning logs written
+      assert.strictEqual(spyLoggerWarn.args.length, 0);
     });
 
-    it('should throw when incompatible instrument is already registered', () => {
+    function testConflictingRegistration(existingDescriptor: InstrumentDescriptor,
+                                         otherDescriptor: InstrumentDescriptor,
+                                         expectedLog: string) {
       const registry = new MetricStorageRegistry();
-      const storage = new TestMetricStorage({
+
+      const storage = new TestMetricStorage(existingDescriptor);
+      const otherStorage = new TestMetricStorage(otherDescriptor);
+
+      assert.strictEqual(registry.register(storage), storage);
+      assert.strictEqual(registry.register(otherStorage), otherStorage);
+      const registeredStorages = registry.getStorages();
+
+      // registered both storages
+      assert.deepStrictEqual([storage, otherStorage], registeredStorages);
+      // warned
+      assertLogCalledOnce();
+      assertFirstLogContains(expectedLog);
+    }
+
+    it('warn when instrument with same name and different type is already registered', () => {
+      const existingDescriptor = {
         name: 'instrument',
         type: InstrumentType.COUNTER,
         description: 'description',
         unit: '1',
         valueType: ValueType.DOUBLE
-      });
+      };
 
-      const otherStorage = new TestMetricStorage({
+      const otherDescriptor = {
         name: 'instrument',
         type: InstrumentType.UP_DOWN_COUNTER,
         description: 'description',
         unit: '1',
         valueType: ValueType.DOUBLE
-      });
+      };
 
-      registry.register(storage);
-      assert.throws(() => registry.register(otherStorage));
-      const registeredStorages = Array.from(registry.getStorages());
+      testConflictingRegistration(existingDescriptor,
+        otherDescriptor,
+        getTypeConflictResolutionRecipe(existingDescriptor, otherDescriptor));
+    });
 
-      // registered the actual storage, but not more than that.
+    it('warn when instrument with same name and different value type is already registered', () => {
+      const existingDescriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      };
+
+      const otherDescriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.INT
+      };
+
+      testConflictingRegistration(existingDescriptor,
+        otherDescriptor,
+        getValueTypeConflictResolutionRecipe(existingDescriptor, otherDescriptor));
+    });
+
+    it('warn when instrument with same name and different unit is already registered', () => {
+      const existingDescriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      };
+
+      const otherDescriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: 'ms',
+        valueType: ValueType.DOUBLE
+      };
+
+      testConflictingRegistration(existingDescriptor,
+        otherDescriptor,
+        getUnitConflictResolutionRecipe(existingDescriptor, otherDescriptor));
+    });
+
+    it('warn when instrument with same name and different description is already registered', () => {
+      const existingDescriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      };
+
+      const otherDescriptor = {
+        name: 'instrument',
+        type: InstrumentType.COUNTER,
+        description: 'longer description',
+        unit: '1',
+        valueType: ValueType.DOUBLE
+      };
+
+      const registry = new MetricStorageRegistry();
+
+      const storage = new TestMetricStorage(existingDescriptor);
+      const otherStorage = new TestMetricStorage(otherDescriptor);
+
+      // returns the first registered storage.
+      assert.strictEqual(registry.register(storage), storage);
+      // returns the original storage
+      assert.strictEqual(registry.register(otherStorage), storage);
+      // original storage now has the updated (longer) description.
+      assert.strictEqual(otherStorage.getInstrumentDescriptor().description, otherDescriptor.description);
+
+      const registeredStorages = registry.getStorages();
+
+      // only the original storage has been added
       assert.deepStrictEqual([storage], registeredStorages);
+      // log called exactly once
+      assertLogCalledOnce();
+      // added resolution recipe to the log
+      assertFirstLogContains(getDescriptionResolutionRecipe(existingDescriptor, otherDescriptor));
     });
 
-    it('should not register when compatible async instrument is already registered', () => {
+    it('should return the existing instrument if a compatible async instrument is already registered', () => {
       const registry = new MetricStorageRegistry();
       const descriptor = {
         name: 'instrument',
@@ -105,12 +214,10 @@ describe('MetricStorageRegistry', () => {
       const storage = new TestMetricStorage(descriptor);
       const otherStorage = new TestMetricStorage(descriptor);
 
-      registry.register(storage);
-      const failedRegisteredStorage = registry.register(otherStorage);
-      const registeredStorages = Array.from(registry.getStorages());
+      assert.strictEqual(registry.register(storage), storage);
+      assert.strictEqual(registry.register(otherStorage), storage);
+      const registeredStorages = registry.getStorages();
 
-      // returned undefined
-      assert.strictEqual(failedRegisteredStorage, undefined);
       // registered the actual storage, but not more than that.
       assert.deepStrictEqual([storage], registeredStorages);
     });
@@ -130,12 +237,20 @@ describe('MetricStorageRegistry', () => {
 
       registry.register(storage);
       const previouslyRegisteredStorage = registry.register(otherStorage);
-      const registeredStorages = Array.from(registry.getStorages());
+      const registeredStorages = registry.getStorages();
 
       // returned undefined
       assert.strictEqual(previouslyRegisteredStorage, storage);
       // registered the actual storage, but not more than that.
       assert.deepStrictEqual([storage], registeredStorages);
     });
+
+    function assertLogCalledOnce() {
+      assert.strictEqual(spyLoggerWarn.args.length, 1);
+    }
+
+    function assertFirstLogContains(expectedString: string) {
+      assert.ok(spyLoggerWarn.args[0].includes(expectedString), 'Logs did not include: ' + expectedString);
+    }
   });
 });

From cf680f61c10ddeb8f407b87c9c3ec46b378f04ba Mon Sep 17 00:00:00 2001
From: "marc.pichler" <marc.pichler@dynatrace.com>
Date: Thu, 3 Mar 2022 13:34:00 +0100
Subject: [PATCH 5/6] fix(views): use concat instead of push with spread
 operator.

---
 .../src/state/MetricStorageRegistry.ts                        | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
index a761e29b6a4..43c53f3f32b 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
@@ -27,9 +27,9 @@ export class MetricStorageRegistry {
   private readonly _metricStorageRegistry = new Map<string, MetricStorage[]>();
 
   getStorages(): MetricStorage[] {
-    const storages = [];
+    let storages: MetricStorage[] = [];
     for (const metricStorages of Array.from(this._metricStorageRegistry.values())) {
-      storages.push(...metricStorages);
+      storages = storages.concat(metricStorages);
     }
 
     return storages;

From 2488a4d2ed441fe4dc6cc6248613c3eb8f1a9d41 Mon Sep 17 00:00:00 2001
From: "marc.pichler" <marc.pichler@dynatrace.com>
Date: Fri, 4 Mar 2022 10:31:00 +0100
Subject: [PATCH 6/6] fix(views): address PR comments.

---
 .../src/state/MetricStorageRegistry.ts             | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
index 43c53f3f32b..48171e8efd7 100644
--- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
+++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
@@ -28,7 +28,7 @@ export class MetricStorageRegistry {
 
   getStorages(): MetricStorage[] {
     let storages: MetricStorage[] = [];
-    for (const metricStorages of Array.from(this._metricStorageRegistry.values())) {
+    for (const metricStorages of this._metricStorageRegistry.values()) {
       storages = storages.concat(metricStorages);
     }
 
@@ -45,7 +45,7 @@ export class MetricStorageRegistry {
       return storage;
     }
 
-    const compatibleStorages = [];
+    let compatibleStorage = null;
 
     for (const existingStorage of existingStorages) {
       const existingDescriptor = existingStorage.getInstrumentDescriptor();
@@ -65,8 +65,8 @@ export class MetricStorageRegistry {
             'The longer description will be used.\nTo resolve the conflict:',
             getConflictResolutionRecipe(existingDescriptor, expectedDescriptor));
         }
-        // Storage is fully compatible.
-        compatibleStorages.push(existingStorage as T);
+        // Storage is fully compatible. There will never be more than one pre-existing fully compatible storage.
+        compatibleStorage = existingStorage as T;
       } else {
         // The implementation SHOULD warn about duplicate instrument registration
         // conflicts after applying View configuration.
@@ -80,10 +80,8 @@ export class MetricStorageRegistry {
       }
     }
 
-    // When one compatible storage is already present, another compatible one will not be pushed.
-    // Therefore this will never be > 1
-    if (compatibleStorages.length > 0) {
-      return compatibleStorages[0];
+    if (compatibleStorage != null) {
+      return compatibleStorage;
     }
 
     // None of the storages were compatible, add the current one to the list.