code review comments

open-telemetry · Jun 26, 2023 · b350a13 · b350a13
1 parent e1452e8
commit b350a13
Show file tree

Hide file tree

Showing 12 changed files with 118 additions and 46 deletions.
diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java
@@ -10,6 +10,7 @@
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import java.util.List;
 import java.util.Set;
 
@@ -20,7 +21,7 @@ public final class HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> {
   final NetClientAttributesGetter<REQUEST, RESPONSE> netAttributesGetter;
   List<String> capturedRequestHeaders = emptyList();
   List<String> capturedResponseHeaders = emptyList();
-  Set<String> knownMethods = HttpRequestMethodUtil.KNOWN_METHODS;
+  Set<String> knownMethods = HttpConstants.KNOWN_METHODS;
 
   HttpClientAttributesExtractorBuilder(
       HttpClientAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,
@@ -72,9 +73,9 @@ public HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> setCapturedRespon
    * <p>By default, this extractor defines "known" methods as the ones listed in <a
    * href="https://www.rfc-editor.org/rfc/rfc9110.html#name-methods">RFC9110</a> and the PATCH
    * method defined in <a href="https://www.rfc-editor.org/rfc/rfc5789.html">RFC5789</a>. If an
-   * unknown method is encountered, the extractor will use the value {@value
-   * HttpRequestMethodUtil#_OTHER} instead of it and put the original value in an extra {@code
-   * http.request.method_original} attribute.
+   * unknown method is encountered, the extractor will use the value {@value HttpConstants#_OTHER}
+   * instead of it and put the original value in an extra {@code http.request.method_original}
+   * attribute.
    *
    * <p>Note: calling this method <b>overrides</b> the default known method sets completely; it does
    * not supplement it.

diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java
@@ -8,8 +8,8 @@
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.lowercase;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.requestAttributeKey;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.responseAttributeKey;
-import static io.opentelemetry.instrumentation.api.instrumenter.http.HttpRequestMethodUtil._OTHER;
 import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
+import static io.opentelemetry.instrumentation.api.internal.HttpConstants._OTHER;
 import static java.util.logging.Level.FINE;
 
 import io.opentelemetry.api.common.AttributesBuilder;

diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
@@ -10,6 +10,7 @@
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import java.util.List;
 import java.util.Set;
 
@@ -20,7 +21,7 @@ public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {
   final NetServerAttributesGetter<REQUEST, RESPONSE> netAttributesGetter;
   List<String> capturedRequestHeaders = emptyList();
   List<String> capturedResponseHeaders = emptyList();
-  Set<String> knownMethods = HttpRequestMethodUtil.KNOWN_METHODS;
+  Set<String> knownMethods = HttpConstants.KNOWN_METHODS;
 
   HttpServerAttributesExtractorBuilder(
       HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,
@@ -72,9 +73,9 @@ public HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> setCapturedRespon
    * <p>By default, this extractor defines "known" methods as the ones listed in <a
    * href="https://www.rfc-editor.org/rfc/rfc9110.html#name-methods">RFC9110</a> and the PATCH
    * method defined in <a href="https://www.rfc-editor.org/rfc/rfc5789.html">RFC5789</a>. If an
-   * unknown method is encountered, the extractor will use the value {@value
-   * HttpRequestMethodUtil#_OTHER} instead of it and put the original value in an extra {@code
-   * http.request.method_original} attribute.
+   * unknown method is encountered, the extractor will use the value {@value HttpConstants#_OTHER}
+   * instead of it and put the original value in an extra {@code http.request.method_original}
+   * attribute.
    *
    * <p>Note: calling this method <b>overrides</b> the default known method sets completely; it does
    * not supplement it.

diff --git a/...pentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java b/...pentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java
@@ -19,6 +19,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.net.internal.NetAttributes;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.List;
@@ -141,7 +142,7 @@ void normal() {
             new TestNetClientAttributesGetter(),
             singletonList("Custom-Request-Header"),
             singletonList("Custom-Response-Header"),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             resendCountFromContext);
 
     AttributesBuilder startAttributes = Attributes.builder();
@@ -312,7 +313,7 @@ void zeroResends() {
             new TestNetClientAttributesGetter(),
             emptyList(),
             emptyList(),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             resendCountFromContext);
 
     AttributesBuilder attributes = Attributes.builder();

diff --git a/...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java b/...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java
@@ -19,6 +19,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.net.internal.NetAttributes;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.List;
@@ -162,7 +163,7 @@ void normal() {
             new TestNetServerAttributesGetter(),
             singletonList("Custom-Request-Header"),
             singletonList("Custom-Response-Header"),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             routeFromContext);
 
     AttributesBuilder startAttributes = Attributes.builder();

diff --git a/...y/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBothSemconvTest.java b/...y/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBothSemconvTest.java
@@ -20,6 +20,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.net.internal.NetAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.NetworkAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.url.internal.UrlAttributes;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.List;
@@ -136,7 +137,7 @@ void normal() {
             new TestNetClientAttributesGetter(),
             singletonList("Custom-Request-Header"),
             singletonList("Custom-Response-Header"),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             resendCountFromContext);
 
     AttributesBuilder startAttributes = Attributes.builder();

diff --git a/...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java b/...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java
@@ -19,6 +19,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.net.internal.NetAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.NetworkAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.url.internal.UrlAttributes;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.List;
@@ -156,7 +157,7 @@ void normal() {
             new TestNetServerAttributesGetter(),
             singletonList("Custom-Request-Header"),
             singletonList("Custom-Response-Header"),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             routeFromContext);
 
     AttributesBuilder startAttributes = Attributes.builder();

diff --git a/...instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java b/...instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java
@@ -21,6 +21,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.NetworkAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.url.internal.UrlAttributes;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -146,7 +147,7 @@ void normal() {
             new TestNetClientAttributesGetter(),
             singletonList("Custom-Request-Header"),
             singletonList("Custom-Response-Header"),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             resendCountFromContext);
 
     AttributesBuilder startAttributes = Attributes.builder();
@@ -224,8 +225,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
   }
 
   @ParameterizedTest
-  @ValueSource(
-      strings = {"CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE"})
+  @ArgumentsSource(ValidRequestMethodsProvider.class)
   void shouldExtractKnownMethods(String requestMethod) {
     Map<String, String> request = new HashMap<>();
     request.put("method", requestMethod);
@@ -238,11 +238,32 @@ void shouldExtractKnownMethods(String requestMethod) {
     extractor.onStart(attributes, Context.root(), request);
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
-    assertThat(attributes.build()).containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod);
+    assertThat(attributes.build())
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod)
+        .doesNotContainKey(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL);
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"get", "Get"})
+  void shouldTreatMethodsAsCaseSensitive(String requestMethod) {
+    Map<String, String> request = new HashMap<>();
+    request.put("method", requestMethod);
+
+    AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
+        HttpClientAttributesExtractor.create(
+            new TestHttpClientAttributesGetter(), new TestNetClientAttributesGetter());
+
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, Context.root(), request);
+    extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
+
+    assertThat(attributes.build())
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpConstants._OTHER)
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL, requestMethod);
   }
 
   @ParameterizedTest
-  @ValueSource(strings = {"get", "PURGE", "Get", "not a method really"})
+  @ValueSource(strings = {"PURGE", "not a method really"})
   void shouldUseOtherForUnknownMethods(String requestMethod) {
     Map<String, String> request = new HashMap<>();
     request.put("method", requestMethod);
@@ -256,7 +277,7 @@ void shouldUseOtherForUnknownMethods(String requestMethod) {
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
     assertThat(attributes.build())
-        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpRequestMethodUtil._OTHER)
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpConstants._OTHER)
         .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL, requestMethod);
   }
 
@@ -276,12 +297,13 @@ void shouldExtractKnownMethods_override(String requestMethod) {
     extractor.onStart(attributes, Context.root(), request);
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
-    assertThat(attributes.build()).containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod);
+    assertThat(attributes.build())
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod)
+        .doesNotContainKey(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL);
   }
 
   @ParameterizedTest
-  @ValueSource(
-      strings = {"CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE"})
+  @ArgumentsSource(ValidRequestMethodsProvider.class)
   void shouldUseOtherForUnknownMethods_override(String requestMethod) {
     Map<String, String> request = new HashMap<>();
     request.put("method", requestMethod);
@@ -297,7 +319,7 @@ void shouldUseOtherForUnknownMethods_override(String requestMethod) {
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
     assertThat(attributes.build())
-        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpRequestMethodUtil._OTHER)
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpConstants._OTHER)
         .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL, requestMethod);
   }
 }
diff --git a/...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java b/...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java
@@ -21,6 +21,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.NetworkAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.url.internal.UrlAttributes;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -167,7 +168,7 @@ void normal() {
             new TestNetServerAttributesGetter(),
             singletonList("Custom-Request-Header"),
             singletonList("Custom-Response-Header"),
-            HttpRequestMethodUtil.KNOWN_METHODS,
+            HttpConstants.KNOWN_METHODS,
             routeFromContext);
 
     AttributesBuilder startAttributes = Attributes.builder();
@@ -248,8 +249,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
   }
 
   @ParameterizedTest
-  @ValueSource(
-      strings = {"CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE"})
+  @ArgumentsSource(ValidRequestMethodsProvider.class)
   void shouldExtractKnownMethods(String requestMethod) {
     Map<String, String> request = new HashMap<>();
     request.put("method", requestMethod);
@@ -262,11 +262,32 @@ void shouldExtractKnownMethods(String requestMethod) {
     extractor.onStart(attributes, Context.root(), request);
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
-    assertThat(attributes.build()).containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod);
+    assertThat(attributes.build())
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod)
+        .doesNotContainKey(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL);
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"get", "Get"})
+  void shouldTreatMethodsAsCaseSensitive(String requestMethod) {
+    Map<String, String> request = new HashMap<>();
+    request.put("method", requestMethod);
+
+    AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
+        HttpServerAttributesExtractor.create(
+            new TestHttpServerAttributesGetter(), new TestNetServerAttributesGetter());
+
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, Context.root(), request);
+    extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
+
+    assertThat(attributes.build())
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpConstants._OTHER)
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL, requestMethod);
   }
 
   @ParameterizedTest
-  @ValueSource(strings = {"get", "PURGE", "Get", "not a method really"})
+  @ValueSource(strings = {"PURGE", "not a method really"})
   void shouldUseOtherForUnknownMethods(String requestMethod) {
     Map<String, String> request = new HashMap<>();
     request.put("method", requestMethod);
@@ -280,7 +301,7 @@ void shouldUseOtherForUnknownMethods(String requestMethod) {
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
     assertThat(attributes.build())
-        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpRequestMethodUtil._OTHER)
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpConstants._OTHER)
         .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL, requestMethod);
   }
 
@@ -300,12 +321,13 @@ void shouldExtractKnownMethods_override(String requestMethod) {
     extractor.onStart(attributes, Context.root(), request);
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
-    assertThat(attributes.build()).containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod);
+    assertThat(attributes.build())
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, requestMethod)
+        .doesNotContainKey(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL);
   }
 
   @ParameterizedTest
-  @ValueSource(
-      strings = {"CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE"})
+  @ArgumentsSource(ValidRequestMethodsProvider.class)
   void shouldUseOtherForUnknownMethods_override(String requestMethod) {
     Map<String, String> request = new HashMap<>();
     request.put("method", requestMethod);
@@ -321,7 +343,7 @@ void shouldUseOtherForUnknownMethods_override(String requestMethod) {
     extractor.onEnd(attributes, Context.root(), request, emptyMap(), null);
 
     assertThat(attributes.build())
-        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpRequestMethodUtil._OTHER)
+        .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD, HttpConstants._OTHER)
         .containsEntry(HttpAttributes.HTTP_REQUEST_METHOD_ORIGINAL, requestMethod);
   }
 }
diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/http/ValidRequestMethodsProvider.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/http/ValidRequestMethodsProvider.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.http;
+
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.ArgumentsProvider;
+
+final class ValidRequestMethodsProvider implements ArgumentsProvider {
+
+  @Override
+  public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
+    return HttpConstants.KNOWN_METHODS.stream().map(Arguments::of);
+  }
+}
diff --git a/...trumenter/http/HttpRequestMethodUtil.java → ...mentation/api/internal/HttpConstants.java b/...trumenter/http/HttpRequestMethodUtil.java → ...mentation/api/internal/HttpConstants.java
@@ -3,23 +3,27 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.instrumentation.api.instrumenter.http;
+package io.opentelemetry.instrumentation.api.internal;
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.unmodifiableSet;
 
 import java.util.HashSet;
 import java.util.Set;
 
-final class HttpRequestMethodUtil {
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public final class HttpConstants {
 
-  static final Set<String> KNOWN_METHODS =
+  public static final Set<String> KNOWN_METHODS =
       unmodifiableSet(
           new HashSet<>(
               asList(
                   "CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE")));
 
-  static final String _OTHER = "_OTHER";
+  public static final String _OTHER = "_OTHER";
 
-  private HttpRequestMethodUtil() {}
+  private HttpConstants() {}
 }