feat(metrics-api): use common attributes definitions (#3038)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
open-telemetry · Jul 20, 2022 · 1a0e0c4 · 1a0e0c4
1 parent 18dce78
commit 1a0e0c4
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 21 deletions.
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -13,6 +13,7 @@ All notable changes to experimental packages in this project will be documented
 
 ### :rocket: (Enhancement)
 
+* feat(metrics-api): use common attributes definitions #3038 @legendecas
 * feat(otlp-proto): pre-compile proto files [#3098](https://github.com/open-telemetry/opentelemetry-js/pull/3098) @legendecas
 
 ### :bug: (Bug Fix)

diff --git a/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts b/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { Context } from '@opentelemetry/api';
+import { Context, SpanAttributes, SpanAttributeValue } from '@opentelemetry/api';
 import { BatchObservableResult, ObservableResult } from './ObservableResult';
 
 /**
@@ -82,10 +82,21 @@ export interface Histogram {
   record(value: number, attributes?: MetricAttributes, context?: Context): void;
 }
 
+// api.SpanAttributes instead of api.Attributes is used here for api package backward compatibility.
 /**
- * key-value pairs passed by the user.
+ * Attributes is a map from string to attribute values.
+ *
+ * Note: only the own enumerable keys are counted as valid attribute keys.
+ */
+export type MetricAttributes = SpanAttributes;
+
+// api.SpanAttributeValue instead of api.AttributeValue is used here for api package backward compatibility.
+/**
+ * Attribute values may be any non-nullish primitive value except an object.
+ *
+ * null or undefined attribute values are invalid and will result in undefined behavior.
  */
-export type MetricAttributes = { [key: string]: string };
+export type MetricAttributeValue = SpanAttributeValue;
 
 /**
  * The observable callback for Observable instruments.

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts b/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts
@@ -24,7 +24,7 @@ import {
   DataPoint,
   Histogram,
 } from '@opentelemetry/sdk-metrics-base';
-import type { MetricAttributes } from '@opentelemetry/api-metrics';
+import type { MetricAttributes, MetricAttributeValue } from '@opentelemetry/api-metrics';
 import { hrTimeToMilliseconds } from '@opentelemetry/core';
 
 type PrometheusDataTypeLiteral =
@@ -38,9 +38,15 @@ function escapeString(str: string) {
   return str.replace(/\\/g, '\\\\').replace(/\n/g, '\\n');
 }
 
-function escapeAttributeValue(str: string) {
+/**
+ * String Attribute values are converted directly to Prometheus attribute values.
+ * Non-string values are represented as JSON-encoded strings.
+ *
+ * `undefined` is converted to an empty string.
+ */
+function escapeAttributeValue(str: MetricAttributeValue = '') {
   if (typeof str !== 'string') {
-    str = String(str);
+    str = JSON.stringify(str);
   }
   return escapeString(str).replace(/"/g, '\\"');
 }

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts
@@ -454,12 +454,16 @@ describe('PrometheusSerializer', () => {
       assert.strictEqual(result, `test_total 1 ${mockedHrTimeMs}\n`);
     });
 
-    it('should serialize non-string attribute values', async () => {
+    it('should serialize non-string attribute values in JSON representations', async () => {
       const serializer = new PrometheusSerializer();
 
       const result = await testSerializer(serializer, 'test_total', counter => {
         counter.add(1, ({
+          true: true,
+          false: false,
+          array: [1, undefined, null, 2],
           object: {},
+          Infinity: Infinity,
           NaN: NaN,
           null: null,
           undefined: undefined,
@@ -468,7 +472,7 @@ describe('PrometheusSerializer', () => {
 
       assert.strictEqual(
         result,
-        `test_total{object="[object Object]",NaN="NaN",null="null",undefined="undefined"} 1 ${mockedHrTimeMs}\n`
+        `test_total{true="true",false="false",array="[1,null,null,2]",object="{}",Infinity="null",NaN="null",null="null",undefined=""} 1 ${mockedHrTimeMs}\n`
       );
     });
 

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts
@@ -31,13 +31,9 @@ export function hashAttributes(attributes: MetricAttributes): string {
   let keys = Object.keys(attributes);
   if (keys.length === 0) return '';
 
+  // Return a string that is stable on key orders.
   keys = keys.sort();
-  return keys.reduce((result, key) => {
-    if (result.length > 2) {
-      result += ',';
-    }
-    return (result += key + ':' + attributes[key]);
-  }, '|#');
+  return JSON.stringify(keys.map(key => [key, attributes[key]]));
 }
 
 /**

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/utils.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/utils.test.ts
@@ -15,8 +15,10 @@
  */
 
 import * as sinon from 'sinon';
-import { callWithTimeout, TimeoutError } from '../src/utils';
+import * as assert from 'assert';
+import { callWithTimeout, hashAttributes, TimeoutError } from '../src/utils';
 import { assertRejects } from './test-utils';
+import { MetricAttributes } from '@opentelemetry/api-metrics';
 
 describe('utils', () => {
   afterEach(() => {
@@ -32,4 +34,23 @@ describe('utils', () => {
       assertRejects(callWithTimeout(promise, 100), TimeoutError);
     });
   });
+
+  describe('hashAttributes', () => {
+    it('should hash all types of attribute values', () => {
+      const cases: [MetricAttributes, string][] = [
+        [{ 'string': 'bar' }, '[["string","bar"]]'],
+        [{ 'number': 1 }, '[["number",1]]'],
+        [{ 'false': false, 'true': true }, '[["false",false],["true",true]]'],
+        [{ 'arrayOfString': ['foo','bar'] }, '[["arrayOfString",["foo","bar"]]]'],
+        [{ 'arrayOfNumber': [1,2] }, '[["arrayOfNumber",[1,2]]]'],
+        [{ 'arrayOfBool': [false,true] }, '[["arrayOfBool",[false,true]]]'],
+        [{ 'undefined': undefined }, '[["undefined",null]]'],
+        [{ 'arrayOfHoles': [undefined, null] }, '[["arrayOfHoles",[null,null]]]'],
+      ];
+
+      for (const [idx, it] of cases.entries()) {
+        assert.strictEqual(hashAttributes(it[0]), it[1], `cases[${idx}] failed`);
+      }
+    });
+  });
 });
diff --git a/experimental/packages/otlp-transformer/test/metrics.test.ts b/experimental/packages/otlp-transformer/test/metrics.test.ts
@@ -29,6 +29,13 @@ import { hrTime, hrTimeToNanoseconds } from '@opentelemetry/core';
 
 const START_TIME = hrTime();
 const END_TIME = hrTime();
+const ATTRIBUTES = {
+  'string-attribute': 'some attribute value',
+  'int-attribute': 1,
+  'double-attribute': 1.1,
+  'boolean-attribute': true,
+  'array-attribute': ['attribute value 1', 'attribute value 2'],
+};
 
 describe('Metrics', () => {
   describe('createExportMetricsServiceRequest', () => {
@@ -58,6 +65,39 @@ describe('Metrics', () => {
           stringValue: 'some attribute value',
         },
       },
+      {
+        key: 'int-attribute',
+        value: {
+          intValue: 1,
+        },
+      },
+      {
+        key: 'double-attribute',
+        value: {
+          doubleValue: 1.1,
+        },
+      },
+      {
+        key: 'boolean-attribute',
+        value: {
+          boolValue: true,
+        },
+      },
+      {
+        key: 'array-attribute',
+        value: {
+          arrayValue: {
+            values: [
+              {
+                stringValue: 'attribute value 1',
+              },
+              {
+                stringValue: 'attribute value 2',
+              }
+            ]
+          },
+        },
+      },
     ];
 
     function createCounterData(value: number, aggregationTemporality: AggregationTemporality): MetricData {
@@ -77,7 +117,7 @@ describe('Metrics', () => {
             value: value,
             startTime: START_TIME,
             endTime: END_TIME,
-            attributes: { 'string-attribute': 'some attribute value' }
+            attributes: ATTRIBUTES,
           }
         ]
       };
@@ -100,7 +140,7 @@ describe('Metrics', () => {
             value: value,
             startTime: START_TIME,
             endTime: END_TIME,
-            attributes: { 'string-attribute': 'some attribute value' }
+            attributes: ATTRIBUTES
           }
         ]
       };
@@ -123,7 +163,7 @@ describe('Metrics', () => {
             value: value,
             startTime: START_TIME,
             endTime: END_TIME,
-            attributes: { 'string-attribute': 'some attribute value' }
+            attributes: ATTRIBUTES,
           }
         ]
       };
@@ -146,7 +186,7 @@ describe('Metrics', () => {
             value: value,
             startTime: START_TIME,
             endTime: END_TIME,
-            attributes: { 'string-attribute': 'some attribute value' }
+            attributes: ATTRIBUTES,
           }
         ]
       };
@@ -169,7 +209,7 @@ describe('Metrics', () => {
             value: value,
             startTime: START_TIME,
             endTime: END_TIME,
-            attributes: { 'string-attribute': 'some attribute value' }
+            attributes: ATTRIBUTES,
           }
         ]
       };
@@ -207,7 +247,7 @@ describe('Metrics', () => {
             },
             startTime: START_TIME,
             endTime: END_TIME,
-            attributes: { 'string-attribute': 'some attribute value' },
+            attributes: ATTRIBUTES,
           }
         ]
       };