diff --git a/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java b/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java index ec3c6c4d6d9..5d379a6794b 100644 --- a/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java +++ b/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java @@ -71,7 +71,9 @@ public Iterable keys(Map carrier) { } List result = new ArrayList<>(carrier.size()); for (String key : carrier.keySet()) { - result.add(EnvironmentSetter.normalizeKey(key)); + if (EnvironmentSetter.isNormalizedKey(key)) { + result.add(key); + } } return Collections.unmodifiableList(result); } @@ -83,20 +85,7 @@ public String get(@Nullable Map carrier, String key) { 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 entry : carrier.entrySet()) { - if (EnvironmentSetter.normalizeKey(entry.getKey()).equals(normalizedKey)) { - return entry.getValue(); - } - } - return null; + return carrier.get(normalizedKey); } @Override diff --git a/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetter.java b/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetter.java index bb754b3f616..ba4f3638a6a 100644 --- a/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetter.java +++ b/api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetter.java @@ -66,6 +66,28 @@ public void set(@Nullable Map carrier, String key, String value) carrier.put(normalizedKey, value); } + /** + * Determine if a key is a valid normalized environment variable name. Returns {@code true} if + * {@code key} is non-empty, contains only uppercase ASCII letters, digits, and underscores, and + * does not start with a digit. + */ + static boolean isNormalizedKey(String key) { + if (key.isEmpty()) { + return false; + } + char first = key.charAt(0); + if (first >= '0' && first <= '9') { + return false; + } + for (int i = 0; i < key.length(); i++) { + char ch = key.charAt(i); + if (!((ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '_')) { + return false; + } + } + return true; + } + /** * Normalizes a key to be a valid environment variable name. * @@ -77,6 +99,9 @@ public void set(@Nullable Map carrier, String key, String value) * */ static String normalizeKey(String key) { + if (isNormalizedKey(key)) { + return key; + } StringBuilder sb = new StringBuilder(key.length() + 1); for (int i = 0; i < key.length(); i++) { char ch = key.charAt(i); diff --git a/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetterTest.java b/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetterTest.java index 91fed7a6943..7a2948e0eda 100644 --- a/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetterTest.java +++ b/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetterTest.java @@ -37,11 +37,17 @@ void get() { @Test void get_normalization() { Map carrier = new HashMap<>(); + // Carrier entries are expected to have normalized keys (i.e. set via EnvironmentSetter). carrier.put("OTEL_TRACE_ID", "val1"); - carrier.put("otel-baggage-key", "val2"); + carrier.put("OTEL_BAGGAGE_KEY", "val2"); + // Carrier entry with an unnormalized key is not reachable via get(). + carrier.put("otel-unreachable-key", "val3"); + // Lookup keys are normalized before the carrier map is consulted. assertThat(EnvironmentGetter.getInstance().get(carrier, "otel.trace.id")).isEqualTo("val1"); assertThat(EnvironmentGetter.getInstance().get(carrier, "otel-baggage-key")).isEqualTo("val2"); + // Carrier entries with unnormalized keys are not enumerated. + assertThat(EnvironmentGetter.getInstance().get(carrier, "otel-unreachable-key")).isNull(); } @Test @@ -52,16 +58,14 @@ void get_null() { @Test @SuppressLogger(EnvironmentGetter.class) - void keys_valuesAreNormalized() { + void keys_onlyNormalizedKeysIncluded() { Map carrier = new HashMap<>(); carrier.put("otel.trace.id", "V1"); carrier.put("otel-baggage-key", "V2"); - carrier.put("OTEL_FOO", "V2"); + carrier.put("OTEL_FOO", "V3"); - // 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"); + // Non-normalized keys are excluded; only already-normalized keys are returned. + assertThat(EnvironmentGetter.getInstance().keys(carrier)).containsExactlyInAnyOrder("OTEL_FOO"); for (String key : EnvironmentGetter.getInstance().keys(carrier)) { assertThat(EnvironmentGetter.getInstance().get(carrier, key)).isNotNull(); } diff --git a/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetterTest.java b/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetterTest.java index c401c02d28b..f0d2b2e73ee 100644 --- a/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetterTest.java +++ b/api/incubator/src/test/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetterTest.java @@ -9,7 +9,11 @@ import java.util.HashMap; import java.util.Map; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; class EnvironmentSetterTest { @@ -62,6 +66,27 @@ void set_valuesAreUnmodified() { assertThat(carrier).containsEntry("KEY6", "value\u0080non-ascii"); } + @ParameterizedTest + @MethodSource("isNormalizedKeyCases") + void isNormalizedKey(String key, boolean expected) { + assertThat(EnvironmentSetter.isNormalizedKey(key)).isEqualTo(expected); + } + + static Stream isNormalizedKeyCases() { + return Stream.of( + Arguments.argumentSet("uppercase letters", "TRACEPARENT", true), + Arguments.argumentSet("uppercase with underscores", "OTEL_TRACE_ID", true), + Arguments.argumentSet("single letter", "A", true), + Arguments.argumentSet("letter then digit", "A0", true), + Arguments.argumentSet("mixed uppercase digits underscores", "A_B_0", true), + Arguments.argumentSet("empty string", "", false), + Arguments.argumentSet("starts with digit", "0ABC", false), + Arguments.argumentSet("lowercase letters", "traceparent", false), + Arguments.argumentSet("dots", "otel.trace.id", false), + Arguments.argumentSet("hyphens", "otel-baggage-key", false), + Arguments.argumentSet("space", "OTEL TRACE", false)); + } + @Test void testToString() { assertThat(EnvironmentSetter.getInstance().toString()).isEqualTo("EnvironmentSetter");