fix review comments

1. Remove map -> constantLabels 2. Make labels values and keys merging code consistent. 3. Update error message
census-instrumentation · Apr 4, 2019 · 4fc9abc · 4fc9abc
1 parent 2737d8d
commit 4fc9abc
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 14 deletions.
diff --git a/packages/opencensus-core/src/common/validations.ts b/packages/opencensus-core/src/common/validations.ts
@@ -59,13 +59,11 @@ export function validateMapElementNotNull<T>(
 
 /** Throws an error if any of the array element present in the map. */
 export function validateDuplicateKeys(
-    keys: LabelKey[], map: Map<LabelKey, LabelValue>) {
-  const keyNames: Set<string> = new Set();
-  keys.forEach(key => keyNames.add(key.key));
-  Array.from(map.keys()).forEach(key => keyNames.add(key.key));
-
-  if (keyNames.size !== (keys.length + map.size)) {
+    keys: LabelKey[], constantLabels: Map<LabelKey, LabelValue>) {
+  const keysAndConstantKeys =
+      new Set([...keys, ...constantLabels.keys()].map(k => k.key));
+  if (keysAndConstantKeys.size !== (keys.length + constantLabels.size)) {
     throw new Error(
-        `The keys from LabelKeys should not be present in constantLabels`);
+        `The keys from LabelKeys should not be present in constantLabels or LabelKeys should not contains duplicate keys`);
   }
 }
diff --git a/packages/opencensus-core/src/metrics/gauges/derived-gauge.ts b/packages/opencensus-core/src/metrics/gauges/derived-gauge.ts
@@ -103,11 +103,11 @@ export class DerivedGauge implements types.Meter {
       type: MetricDescriptorType, labelKeys: LabelKey[],
       readonly constantLabels: Map<LabelKey, LabelValue>) {
     this.labelKeysLength = labelKeys.length;
-    const combinedLabelKeys = labelKeys.concat(...constantLabels.keys());
+    const keysAndConstantKeys = [...labelKeys, ...constantLabels.keys()];
     this.constantLabelValues = [...constantLabels.values()];
 
     this.metricDescriptor =
-        {name, description, unit, type, labelKeys: combinedLabelKeys};
+        {name, description, unit, type, labelKeys: keysAndConstantKeys};
   }
 
   // Checks if the specified collection is a LengthAttributeInterface.

diff --git a/packages/opencensus-core/src/metrics/gauges/gauge.ts b/packages/opencensus-core/src/metrics/gauges/gauge.ts
@@ -51,11 +51,11 @@ export class Gauge implements types.Meter {
       type: MetricDescriptorType, readonly labelKeys: LabelKey[],
       readonly constantLabels: Map<LabelKey, LabelValue>) {
     this.labelKeysLength = labelKeys.length;
-    const combinedLabelKeys = labelKeys.concat(...constantLabels.keys());
+    const keysAndConstantKeys = [...labelKeys, ...constantLabels.keys()];
     this.constantLabelValues = [...constantLabels.values()];
 
     this.metricDescriptor =
-        {name, description, unit, type, labelKeys: combinedLabelKeys};
+        {name, description, unit, type, labelKeys: keysAndConstantKeys};
     this.defaultLabelValues = initializeDefaultLabels(this.labelKeysLength);
   }
 

diff --git a/packages/opencensus-core/src/metrics/metric-registry.ts b/packages/opencensus-core/src/metrics/metric-registry.ts
@@ -16,7 +16,6 @@
 
 import {validateArrayElementsNotNull, validateDuplicateKeys, validateMapElementNotNull, validateNotNull} from '../common/validations';
 import {MeasureUnit} from '../stats/types';
-
 import {BaseMetricProducer} from './export/base-metric-producer';
 import {Metric, MetricDescriptorType, MetricProducer} from './export/types';
 import {DerivedGauge} from './gauges/derived-gauge';

diff --git a/packages/opencensus-core/test/test-metric-registry.ts b/packages/opencensus-core/test/test-metric-registry.ts
@@ -110,7 +110,7 @@ describe('addInt64Gauge', () => {
        const labelKeys = [{key: 'k1', description: 'desc'}];
        assert.throws(() => {
          registry.addInt64Gauge(METRIC_NAME, {constantLabels, labelKeys});
-       }, /^Error: The keys from LabelKeys should not be present in constantLabels$/);
+       }, /^Error: The keys from LabelKeys should not be present in constantLabels or LabelKeys should not contains duplicate keys$/);
      });
 });
 
@@ -273,7 +273,7 @@ describe('addDerivedInt64Gauge', () => {
        assert.throws(() => {
          registry.addDerivedInt64Gauge(
              METRIC_NAME, {constantLabels, labelKeys});
-       }, /^Error: The keys from LabelKeys should not be present in constantLabels$/);
+       }, /^Error: The keys from LabelKeys should not be present in constantLabels or LabelKeys should not contains duplicate keys$/);
      });
 
   it('should throw an error when the constant labels elements are null', () => {