Update Env var getter / setter to reflect spec changes (#8233)

Author: jack-berg

api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java

@@ -9,8 +9,8 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
@@ -46,7 +46,8 @@
  */
 public final class EnvironmentGetter implements TextMapGetter<Map<String, String>> {
 
-  private static final Logger logger = Logger.getLogger(EnvironmentGetter.class.getName());
+  private static final AtomicBoolean LOG_KEYS_CALLED = new AtomicBoolean(false);
+  private static final Logger LOGGER = Logger.getLogger(EnvironmentGetter.class.getName());
   private static final EnvironmentGetter INSTANCE = new EnvironmentGetter();
 
   private EnvironmentGetter() {}
@@ -58,14 +59,21 @@ public static EnvironmentGetter getInstance() {
 
   @Override
   public Iterable<String> keys(Map<String, String> carrier) {
+    if (LOG_KEYS_CALLED.compareAndSet(false, true)) {
+      LOGGER.log(
+          Level.WARNING,
+          "keys() called on EnvironmentGetter. "
+              + "This may produce unexpected results with propagators which depend on case sensitivity or special characters in keys.",
+          new Throwable());
+    }
     if (carrier == null) {
       return Collections.emptyList();
     }
     List<String> result = new ArrayList<>(carrier.size());
     for (String key : carrier.keySet()) {
-      result.add(key.toLowerCase(Locale.ROOT));
+      result.add(EnvironmentSetter.normalizeKey(key));
     }
-    return result;
+    return Collections.unmodifiableList(result);
   }
 
   @Nullable
@@ -74,20 +82,21 @@ public String get(@Nullable Map<String, String> carrier, String key) {
     if (carrier == null || key == null) {
       return null;
     }
-    // Spec recommends using uppercase and underscores for environment variable
-    // names for maximum
-    // cross-platform compatibility.
-    String sanitizedKey = key.replace('.', '_').replace('-', '_').toUpperCase(Locale.ROOT);
-    String value = carrier.get(sanitizedKey);
-    if (value != null && !EnvironmentSetter.isValidHttpHeaderValue(value)) {
-      logger.log(
-          Level.FINE,
-          "Ignoring environment variable '{0}': "
-              + "value contains characters not valid in HTTP header fields per RFC 9110.",
-          sanitizedKey);
-      return null;
+    String normalizedKey = EnvironmentSetter.normalizeKey(key);
+    // first, perform an optimistic lookup for an exact match on the normalized key
+    String value = carrier.get(normalizedKey);
+    if (value != null) {
+      return value;
+    }
+    // next, iterate over the carrier normalizing each entry and evaluating for a match
+    // if memory allocation becomes an issue, can implement using iterative normalization, comparing
+    // an entry character by character to the normalized key, normalizing along the way.
+    for (Map.Entry<String, String> entry : carrier.entrySet()) {
+      if (EnvironmentSetter.normalizeKey(entry.getKey()).equals(normalizedKey)) {
+        return entry.getValue();
+      }
     }
-    return value;
+    return null;
   }
 
   @Override

api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetter.java

@@ -6,10 +6,7 @@
 package io.opentelemetry.api.incubator.propagation;
 
 import io.opentelemetry.context.propagation.TextMapSetter;
-import java.util.Locale;
 import java.util.Map;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 /**
@@ -30,8 +27,10 @@
  * conventions:
  *
  * <ul>
- *   <li>Converts keys to uppercase (e.g., {@code traceparent} becomes {@code TRACEPARENT})
- *   <li>Replaces {@code .} and {@code -} with underscores
+ *   <li>ASCII letters are converted to uppercase
+ *   <li>Any character that is not an ASCII letter, digit, or underscore is replaced with an
+ *       underscore
+ *   <li>If the result would start with a digit, an underscore is prepended
  * </ul>
  *
  * <p>Values are validated to contain only characters valid in HTTP header fields per <a
@@ -49,7 +48,6 @@
  */
 public final class EnvironmentSetter implements TextMapSetter<Map<String, String>> {
 
-  private static final Logger logger = Logger.getLogger(EnvironmentSetter.class.getName());
   private static final EnvironmentSetter INSTANCE = new EnvironmentSetter();
 
   private EnvironmentSetter() {}
@@ -64,35 +62,36 @@ public void set(@Nullable Map<String, String> carrier, String key, String value)
     if (carrier == null || key == null || value == null) {
       return;
     }
-    if (!isValidHttpHeaderValue(value)) {
-      logger.log(
-          Level.FINE,
-          "Skipping environment variable injection for key ''{0}'': "
-              + "value contains characters not valid in HTTP header fields per RFC 9110.",
-          key);
-      return;
-    }
-    // Spec recommends using uppercase and underscores for environment variable
-    // names for maximum
-    // cross-platform compatibility.
-    String sanitizedKey = key.replace('.', '_').replace('-', '_').toUpperCase(Locale.ROOT);
-    carrier.put(sanitizedKey, value);
+    String normalizedKey = normalizeKey(key);
+    carrier.put(normalizedKey, value);
   }
 
   /**
-   * Checks whether a string contains only characters valid in HTTP header field values per <a
-   * href="https://datatracker.ietf.org/doc/html/rfc9110#section-5.5">RFC 9110 Section 5.5</a>.
-   * Valid characters are: visible ASCII (0x21-0x7E), space (0x20), and horizontal tab (0x09).
+   * Normalizes a key to be a valid environment variable name.
+   *
+   * <ul>
+   *   <li>ASCII letters are converted to uppercase
+   *   <li>Any character that is not an ASCII letter, digit, or underscore is replaced with an
+   *       underscore (including {@code .}, {@code -}, whitespace, and control characters)
+   *   <li>If the result would start with a digit, an underscore is prepended
+   * </ul>
    */
-  static boolean isValidHttpHeaderValue(String value) {
-    for (int i = 0; i < value.length(); i++) {
-      char ch = value.charAt(i);
-      // VCHAR (0x21-0x7E), SP (0x20), HTAB (0x09)
-      if (ch != '\t' && (ch < ' ' || ch > '~')) {
-        return false;
+  static String normalizeKey(String key) {
+    StringBuilder sb = new StringBuilder(key.length() + 1);
+    for (int i = 0; i < key.length(); i++) {
+      char ch = key.charAt(i);
+      if (ch >= 'a' && ch <= 'z') {
+        sb.append((char) (ch - 32));
+      } else if ((ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '_') {
+        sb.append(ch);
+      } else {
+        sb.append('_');
       }
     }
-    return true;
+    if (sb.length() > 0 && sb.charAt(0) >= '0' && sb.charAt(0) <= '9') {
+      sb.insert(0, '_');
+    }
+    return sb.toString();
   }
 
   @Override

api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetterTest.java

@@ -7,13 +7,19 @@
 
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 
+import io.github.netmikey.logunit.api.LogCapturer;
+import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 class EnvironmentGetterTest {
 
+  @RegisterExtension
+  LogCapturer logCapturer = LogCapturer.create().captureForType(EnvironmentGetter.class);
+
   @Test
   void get() {
     Map<String, String> carrier = new HashMap<>();
@@ -29,10 +35,10 @@ void get() {
   }
 
   @Test
-  void get_sanitization() {
+  void get_normalization() {
     Map<String, String> carrier = new HashMap<>();
     carrier.put("OTEL_TRACE_ID", "val1");
-    carrier.put("OTEL_BAGGAGE_KEY", "val2");
+    carrier.put("otel-baggage-key", "val2");
 
     assertThat(EnvironmentGetter.getInstance().get(carrier, "otel.trace.id")).isEqualTo("val1");
     assertThat(EnvironmentGetter.getInstance().get(carrier, "otel-baggage-key")).isEqualTo("val2");
@@ -45,37 +51,45 @@ void get_null() {
   }
 
   @Test
-  void keys() {
+  @SuppressLogger(EnvironmentGetter.class)
+  void keys_valuesAreNormalized() {
     Map<String, String> carrier = new HashMap<>();
-    carrier.put("K1", "V1");
-    carrier.put("K2", "V2");
-
-    assertThat(EnvironmentGetter.getInstance().keys(carrier)).containsExactlyInAnyOrder("k1", "k2");
+    carrier.put("otel.trace.id", "V1");
+    carrier.put("otel-baggage-key", "V2");
+    carrier.put("OTEL_FOO", "V2");
+
+    // For a carrier containing keys that are both normalized and not normalized, verify all results
+    // from keys() return values for get.
+    assertThat(EnvironmentGetter.getInstance().keys(carrier))
+        .containsExactlyInAnyOrder("OTEL_TRACE_ID", "OTEL_BAGGAGE_KEY", "OTEL_FOO");
+    for (String key : EnvironmentGetter.getInstance().keys(carrier)) {
+      assertThat(EnvironmentGetter.getInstance().get(carrier, key)).isNotNull();
+    }
     assertThat(EnvironmentGetter.getInstance().keys(null)).isEmpty();
+
+    assertThat(logCapturer.size()).isEqualTo(1);
+    logCapturer.assertContains("keys() called on EnvironmentGetter");
   }
 
   @Test
-  void get_validHeaderValues() {
+  void get_valuesAreUnmodified() {
     Map<String, String> carrier = new HashMap<>();
     carrier.put("KEY1", "simple-value");
     carrier.put("KEY2", "value with spaces");
     carrier.put("KEY3", "value\twith\ttabs");
+    carrier.put("KEY4", "value\u0000with\u0001control");
+    carrier.put("KEY5", "value\nwith\nnewlines");
+    carrier.put("KEY6", "value\u0080non-ascii");
 
     assertThat(EnvironmentGetter.getInstance().get(carrier, "key1")).isEqualTo("simple-value");
     assertThat(EnvironmentGetter.getInstance().get(carrier, "key2")).isEqualTo("value with spaces");
     assertThat(EnvironmentGetter.getInstance().get(carrier, "key3")).isEqualTo("value\twith\ttabs");
-  }
-
-  @Test
-  void get_invalidHeaderValues() {
-    Map<String, String> carrier = new HashMap<>();
-    carrier.put("KEY1", "value\u0000with\u0001control");
-    carrier.put("KEY2", "value\nwith\nnewlines");
-    carrier.put("KEY3", "value\u0080non-ascii");
-
-    assertThat(EnvironmentGetter.getInstance().get(carrier, "key1")).isNull();
-    assertThat(EnvironmentGetter.getInstance().get(carrier, "key2")).isNull();
-    assertThat(EnvironmentGetter.getInstance().get(carrier, "key3")).isNull();
+    assertThat(EnvironmentGetter.getInstance().get(carrier, "key4"))
+        .isEqualTo("value\u0000with\u0001control");
+    assertThat(EnvironmentGetter.getInstance().get(carrier, "key5"))
+        .isEqualTo("value\nwith\nnewlines");
+    assertThat(EnvironmentGetter.getInstance().get(carrier, "key6"))
+        .isEqualTo("value\u0080non-ascii");
   }
 
   @Test

api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetterTest.java

@@ -45,28 +45,21 @@ void set_null() {
   }
 
   @Test
-  void set_validHeaderValues() {
+  void set_valuesAreUnmodified() {
     Map<String, String> carrier = new HashMap<>();
-    // Printable ASCII and tab are valid per RFC 9110
     EnvironmentSetter.getInstance().set(carrier, "key1", "simple-value");
     EnvironmentSetter.getInstance().set(carrier, "key2", "value with spaces");
     EnvironmentSetter.getInstance().set(carrier, "key3", "value\twith\ttabs");
+    EnvironmentSetter.getInstance().set(carrier, "key4", "value\u0000with\u0001control");
+    EnvironmentSetter.getInstance().set(carrier, "key5", "value\nwith\nnewlines");
+    EnvironmentSetter.getInstance().set(carrier, "key6", "value\u0080non-ascii");
 
     assertThat(carrier).containsEntry("KEY1", "simple-value");
     assertThat(carrier).containsEntry("KEY2", "value with spaces");
     assertThat(carrier).containsEntry("KEY3", "value\twith\ttabs");
-  }
-
-  @Test
-  void set_invalidHeaderValues() {
-    Map<String, String> carrier = new HashMap<>();
-    // Control characters and non-ASCII are invalid per RFC 9110
-    EnvironmentSetter.getInstance().set(carrier, "key1", "value\u0000with\u0001control");
-    EnvironmentSetter.getInstance().set(carrier, "key2", "value\nwith\nnewlines");
-    EnvironmentSetter.getInstance().set(carrier, "key3", "value\rwith\rcarriage");
-    EnvironmentSetter.getInstance().set(carrier, "key4", "value\u0080non-ascii");
-
-    assertThat(carrier).isEmpty();
+    assertThat(carrier).containsEntry("KEY4", "value\u0000with\u0001control");
+    assertThat(carrier).containsEntry("KEY5", "value\nwith\nnewlines");
+    assertThat(carrier).containsEntry("KEY6", "value\u0080non-ascii");
   }
 
   @Test