fix(opentelemetry-resources): Update the Env Var Parsing Logic to Match Spec

CHANGELOG.md

@@ -12,6 +12,11 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2
 
 ### :boom: Breaking Changes
 
+* fix(resources): update `OTEL_RESOURCE_ATTRIBUTES` parsing to match spec changes (open-telemetry/opentelemetry-specification#4856) [#6261](https://github.com/open-telemetry/opentelemetry-js/pull/6261) @jacksonweber
+  * **Important:** This fix is included in the "breaking changes" section because it can be breaking for some edge case usage of `OTEL_RESOURCE_ATTRIBUTES`:
+    * `export OTEL_RESOURCE_ATTRIBUTES=foo=bar,spam` will now be fully ignored, because the `spam` entry is invalid (missing `=`). Per spec, any parsing error results in ignoring the entire environment variable.
+    * `export OTEL_RESOURCE_ATTRIBUTES='wat=" spaces  "'` will now result in `{"wat": "\" spaces  \""}` *with* the double-quotes included in the value. Before this change the implementation included brittle double-quoting to allow leading and trailing whitespace in the value. To support leading or trailing whitespace now, you must percent-encode the whitespace. Internal whitespace still works without encoding, e.g. `export OTEL_RESOURCE_ATTRIBUTES='green=eggs and ham'`.
+
 ### :rocket: Features
 
 ### :bug: Bug Fixes

experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts

@@ -764,16 +764,8 @@ describe('Node SDK', () => {
     });
 
     describe('with a diag logger', () => {
-      // Local functions to test if a mocked method is ever called with a specific argument or regex matching for an argument.
+      // Local function to test if a mocked method is ever called with regex matching for an argument.
       // Needed because of race condition with parallel detectors.
-      const callArgsContains = (
-        mockedFunction: Sinon.SinonSpy,
-        arg: any
-      ): boolean => {
-        return mockedFunction.getCalls().some(call => {
-          return call.args.some(callarg => arg === callarg);
-        });
-      };
       const callArgsMatches = (
         mockedFunction: Sinon.SinonSpy,
         regex: RegExp
@@ -832,7 +824,7 @@ describe('Node SDK', () => {
 
       describe('with a faulty environment variable', () => {
         beforeEach(() => {
-          process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=\\attribute';
+          process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=' + 'x'.repeat(300);
         });
 
         it('prints correct error messages when EnvDetector has an invalid variable', async () => {
@@ -850,9 +842,9 @@ describe('Node SDK', () => {
           sdk.start();
 
           assert.ok(
-            callArgsContains(
+            callArgsMatches(
               mockedLoggerMethod,
-              'EnvDetector failed: Attribute value should be a ASCII string with a length not exceed 255 characters.'
+              /EnvDetector failed: Attribute value exceeds the maximum length of 255 characters/
             )
           );
           await sdk.shutdown();
@@ -954,7 +946,7 @@ describe('Node SDK', () => {
 
     it('should configure service instance id via OTEL_RESOURCE_ATTRIBUTES env var', async () => {
       process.env.OTEL_RESOURCE_ATTRIBUTES =
-        'service.instance.id=627cc493,service.name=my-service,service.namespace';
+        'service.instance.id=627cc493,service.name=my-service,service.namespace=default';
       const sdk = new NodeSDK();
 
       sdk.start();

packages/opentelemetry-resources/src/detectors/EnvDetector.ts

@@ -34,16 +34,6 @@ class EnvDetector implements ResourceDetector {
   // OTEL_RESOURCE_ATTRIBUTES contains key value pair separated by '='.
   private readonly _LABEL_KEY_VALUE_SPLITTER = '=';
 
-  private readonly _ERROR_MESSAGE_INVALID_CHARS =
-    'should be a ASCII string with a length greater than 0 and not exceed ' +
-    this._MAX_LENGTH +
-    ' characters.';
-
-  private readonly _ERROR_MESSAGE_INVALID_VALUE =
-    'should be a ASCII string with a length not exceed ' +
-    this._MAX_LENGTH +
-    ' characters.';
-
   /**
    * Returns a {@link Resource} populated with attributes from the
    * OTEL_RESOURCE_ATTRIBUTES environment variable. Note this is an async
@@ -62,7 +52,7 @@ class EnvDetector implements ResourceDetector {
         const parsedAttributes = this._parseResourceAttributes(rawAttributes);
         Object.assign(attributes, parsedAttributes);
       } catch (e) {
-        diag.debug(`EnvDetector failed: ${e.message}`);
+        diag.debug(`EnvDetector failed: ${e instanceof Error ? e.message : e}`);
       }
     }
 
@@ -77,78 +67,77 @@ class EnvDetector implements ResourceDetector {
    * Creates an attribute map from the OTEL_RESOURCE_ATTRIBUTES environment
    * variable.
    *
-   * OTEL_RESOURCE_ATTRIBUTES: A comma-separated list of attributes describing
-   * the source in more detail, e.g. “key1=val1,key2=val2”. Domain names and
-   * paths are accepted as attribute keys. Values may be quoted or unquoted in
-   * general. If a value contains whitespace, =, or " characters, it must
-   * always be quoted.
+   * OTEL_RESOURCE_ATTRIBUTES: A comma-separated list of attributes in the
+   * format "key1=value1,key2=value2". The ',' and '=' characters in keys
+   * and values MUST be percent-encoded. Other characters MAY be percent-encoded.
+   *
+   * Per the spec, on any error (e.g., decoding failure), the entire environment
+   * variable value is discarded.
    *
    * @param rawEnvAttributes The resource attributes as a comma-separated list
    * of key/value pairs.
-   * @returns The sanitized resource attributes.
+   * @returns The parsed resource attributes.
+   * @throws Error if parsing fails (caller handles by discarding all attributes)
    */
   private _parseResourceAttributes(rawEnvAttributes?: string): Attributes {
     if (!rawEnvAttributes) return {};
 
     const attributes: Attributes = {};
     const rawAttributes: string[] = rawEnvAttributes.split(
-      this._COMMA_SEPARATOR,
-      -1
+      this._COMMA_SEPARATOR
     );
+
     for (const rawAttribute of rawAttributes) {
       const keyValuePair: string[] = rawAttribute.split(
-        this._LABEL_KEY_VALUE_SPLITTER,
-        -1
+        this._LABEL_KEY_VALUE_SPLITTER
       );
+
+      // Per spec: ',' and '=' MUST be percent-encoded in keys and values.
+      // If we get != 2 parts, there's an unencoded '=' which is an error.
       if (keyValuePair.length !== 2) {
-        continue;
+        throw new Error(
+          `Invalid format for OTEL_RESOURCE_ATTRIBUTES: "${rawAttribute}". ` +
+            `Expected format: key=value. The ',' and '=' characters must be percent-encoded in keys and values.`
+        );
       }
-      let [key, value] = keyValuePair;
-      // Leading and trailing whitespaces are trimmed.
-      key = key.trim();
-      value = value.trim().split(/^"|"$/).join('');
-      if (!this._isValidAndNotEmpty(key)) {
-        throw new Error(`Attribute key ${this._ERROR_MESSAGE_INVALID_CHARS}`);
+
+      const [rawKey, rawValue] = keyValuePair;
+      const key = rawKey.trim();
+      const value = rawValue.trim();
+
+      if (key.length === 0) {
+        throw new Error(
+          `Invalid OTEL_RESOURCE_ATTRIBUTES: empty attribute key in "${rawAttribute}".`
+        );
       }
-      if (!this._isValid(value)) {
-        throw new Error(`Attribute value ${this._ERROR_MESSAGE_INVALID_VALUE}`);
+
+      let decodedKey: string;
+      let decodedValue: string;
+      try {
+        decodedKey = decodeURIComponent(key);
+        decodedValue = decodeURIComponent(value);
+      } catch (e) {
+        throw new Error(
+          `Failed to percent-decode OTEL_RESOURCE_ATTRIBUTES entry "${rawAttribute}": ${e instanceof Error ? e.message : e}`
+        );
       }
-      attributes[key] = decodeURIComponent(value);
-    }
-    return attributes;
-  }
 
-  /**
-   * Determines whether the given String is a valid printable ASCII string with
-   * a length not exceed _MAX_LENGTH characters.
-   *
-   * @param str The String to be validated.
-   * @returns Whether the String is valid.
-   */
-  private _isValid(name: string): boolean {
-    return name.length <= this._MAX_LENGTH && this._isBaggageOctetString(name);
-  }
+      if (decodedKey.length > this._MAX_LENGTH) {
+        throw new Error(
+          `Attribute key exceeds the maximum length of ${this._MAX_LENGTH} characters: "${decodedKey}".`
+        );
+      }
 
-  // https://www.w3.org/TR/baggage/#definition
-  private _isBaggageOctetString(str: string): boolean {
-    for (let i = 0; i < str.length; i++) {
-      const ch = str.charCodeAt(i);
-      if (ch < 0x21 || ch === 0x2c || ch === 0x3b || ch === 0x5c || ch > 0x7e) {
-        return false;
+      if (decodedValue.length > this._MAX_LENGTH) {
+        throw new Error(
+          `Attribute value exceeds the maximum length of ${this._MAX_LENGTH} characters for key "${decodedKey}".`
+        );
       }
+
+      attributes[decodedKey] = decodedValue;
     }
-    return true;
-  }
 
-  /**
-   * Determines whether the given String is a valid printable ASCII string with
-   * a length greater than 0 and not exceed _MAX_LENGTH characters.
-   *
-   * @param str The String to be validated.
-   * @returns Whether the String is valid and not empty.
-   */
-  private _isValidAndNotEmpty(str: string): boolean {
-    return str.length > 0 && this._isValid(str);
+    return attributes;
   }
 }