Define url.scheme in terms of logical operation in HTTP server semconv

open-telemetry · Oct 17, 2023 · 2d4a518 · 2d4a518
1 parent 48d3956
commit 2d4a518
Show file tree

Hide file tree

Showing 7 changed files with 34 additions and 29 deletions.
diff --git a/instrumentation-api-semconv/build.gradle.kts b/instrumentation-api-semconv/build.gradle.kts
@@ -75,10 +75,6 @@ tasks {
     dependsOn("generateJflex")
   }
 
-  test {
-    jvmArgs("-Dotel.instrumentation.http.prefer-forwarded-url-scheme=true")
-  }
-
   check {
     dependsOn(testing.suites)
   }

diff --git a/...nter/http/AlternateUrlSchemeProvider.java → ...nter/http/ForwardedUrlSchemeProvider.java b/...nter/http/AlternateUrlSchemeProvider.java → ...nter/http/ForwardedUrlSchemeProvider.java
@@ -5,32 +5,20 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil;
 import java.util.Locale;
 import java.util.function.Function;
 import javax.annotation.Nullable;
 
-final class AlternateUrlSchemeProvider<REQUEST> implements Function<REQUEST, String> {
-
-  // if set to true, the instrumentation will prefer the scheme from Forwarded/X-Forwarded-Proto
-  // headers over the one extracted from the URL
-  private static final boolean PREFER_FORWARDED_URL_SCHEME =
-      ConfigPropertiesUtil.getBoolean(
-          "otel.instrumentation.http.prefer-forwarded-url-scheme", false);
+final class ForwardedUrlSchemeProvider<REQUEST> implements Function<REQUEST, String> {
 
   private final HttpServerAttributesGetter<REQUEST, ?> getter;
 
-  AlternateUrlSchemeProvider(HttpServerAttributesGetter<REQUEST, ?> getter) {
+  ForwardedUrlSchemeProvider(HttpServerAttributesGetter<REQUEST, ?> getter) {
     this.getter = getter;
   }
 
   @Override
   public String apply(REQUEST request) {
-    if (!PREFER_FORWARDED_URL_SCHEME) {
-      // don't parse headers, extract scheme from the URL
-      return null;
-    }
-
     // try Forwarded
     for (String forwarded : getter.getHttpRequestHeader(request, "forwarded")) {
       String proto = extractProtoFromForwardedHeader(forwarded);

diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
@@ -128,7 +128,7 @@ public AttributesExtractor<REQUEST, RESPONSE> build() {
   InternalUrlAttributesExtractor<REQUEST> buildUrlExtractor() {
     return new InternalUrlAttributesExtractor<>(
         httpAttributesGetter,
-        new AlternateUrlSchemeProvider<>(httpAttributesGetter),
+        new ForwardedUrlSchemeProvider<>(httpAttributesGetter),
         SemconvStability.emitStableHttpSemconv(),
         SemconvStability.emitOldHttpSemconv());
   }

diff --git a/.../http/AlternateUrlSchemeProviderTest.java → .../http/ForwardedUrlSchemeProviderTest.java b/.../http/AlternateUrlSchemeProviderTest.java → .../http/ForwardedUrlSchemeProviderTest.java
@@ -28,13 +28,13 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 
 @ExtendWith(MockitoExtension.class)
-class AlternateUrlSchemeProviderTest {
+class ForwardedUrlSchemeProviderTest {
 
   private static final String REQUEST = "request";
 
   @Mock HttpServerAttributesGetter<String, String> getter;
 
-  @InjectMocks AlternateUrlSchemeProvider<String> underTest;
+  @InjectMocks ForwardedUrlSchemeProvider<String> underTest;
 
   @Test
   void noHeaders() {

diff --git a/...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java b/...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java
@@ -122,10 +122,10 @@ public Integer getServerPort(Map<String, Object> request) {
   void normal() {
     Map<String, Object> request = new HashMap<>();
     request.put("method", "POST");
-    request.put("urlFull", "http://github.com");
+    request.put("urlFull", "https://github.com");
     request.put("urlPath", "/repositories/1");
     request.put("urlQuery", "details=true");
-    request.put("urlScheme", "http");
+    request.put("urlScheme", "https");
     request.put("header.content-length", "10");
     request.put("route", "/repositories/{id}");
     request.put("header.user-agent", "okhttp 3.x");
@@ -159,9 +159,9 @@ void normal() {
             entry(SemanticAttributes.SERVER_ADDRESS, "github.com"),
             entry(SemanticAttributes.HTTP_METHOD, "POST"),
             entry(SemanticAttributes.HTTP_REQUEST_METHOD, "POST"),
-            entry(SemanticAttributes.HTTP_SCHEME, "http"),
+            entry(SemanticAttributes.HTTP_SCHEME, "https"),
             entry(SemanticAttributes.HTTP_TARGET, "/repositories/1?details=true"),
-            entry(SemanticAttributes.URL_SCHEME, "http"),
+            entry(SemanticAttributes.URL_SCHEME, "https"),
             entry(SemanticAttributes.URL_PATH, "/repositories/1"),
             entry(SemanticAttributes.URL_QUERY, "details=true"),
             entry(SemanticAttributes.USER_AGENT_ORIGINAL, "okhttp 3.x"),

diff --git a/...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java b/...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java
@@ -159,10 +159,10 @@ public String getErrorType(
   void normal() {
     Map<String, String> request = new HashMap<>();
     request.put("method", "POST");
-    request.put("urlFull", "http://github.com");
+    request.put("urlFull", "https://github.com");
     request.put("urlPath", "/repositories/1");
     request.put("urlQuery", "details=true");
-    request.put("urlScheme", "http");
+    request.put("urlScheme", "https");
     request.put("header.content-length", "10");
     request.put("route", "/repositories/{id}");
     request.put("header.user-agent", "okhttp 3.x");
@@ -196,7 +196,7 @@ void normal() {
         .containsOnly(
             entry(SemanticAttributes.SERVER_ADDRESS, "github.com"),
             entry(SemanticAttributes.HTTP_REQUEST_METHOD, "POST"),
-            entry(SemanticAttributes.URL_SCHEME, "http"),
+            entry(SemanticAttributes.URL_SCHEME, "https"),
             entry(SemanticAttributes.URL_PATH, "/repositories/1"),
             entry(SemanticAttributes.URL_QUERY, "details=true"),
             entry(SemanticAttributes.USER_AGENT_ORIGINAL, "okhttp 3.x"),
@@ -417,4 +417,26 @@ void shouldExtractErrorType_other() {
 
     assertThat(attributes.build()).containsEntry(HttpAttributes.ERROR_TYPE, HttpConstants._OTHER);
   }
+
+  @Test
+  void shouldPreferUrlSchemeFromForwardedHeader() {
+    Map<String, String> request = new HashMap<>();
+    request.put("urlScheme", "http");
+    request.put("header.forwarded", "proto=https");
+
+    Map<String, String> response = new HashMap<>();
+    response.put("statusCode", "202");
+
+    AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
+        HttpServerAttributesExtractor.create(new TestHttpServerAttributesGetter());
+
+    AttributesBuilder startAttributes = Attributes.builder();
+    extractor.onStart(startAttributes, Context.root(), request);
+    assertThat(startAttributes.build()).containsOnly(entry(SemanticAttributes.URL_SCHEME, "https"));
+
+    AttributesBuilder endAttributes = Attributes.builder();
+    extractor.onEnd(endAttributes, Context.root(), request, response, null);
+    assertThat(endAttributes.build())
+        .containsOnly(entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 202L));
+  }
 }
diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java
@@ -200,7 +200,6 @@ private static void copyNecessaryConfigToSystemProperties(ConfigProperties confi
     for (String property :
         asList(
             "otel.instrumentation.experimental.span-suppression-strategy",
-            "otel.instrumentation.http.prefer-forwarded-url-scheme",
             "otel.semconv-stability.opt-in")) {
       String value = config.getString(property);
       if (value != null) {
-Original file line number
+Diff line change
@@ Expand Up / @@ -75,10 +75,6 @@ tasks { @@
         dependsOn("generateJflex")
       }
-      test {
-        jvmArgs("-Dotel.instrumentation.http.prefer-forwarded-url-scheme=true")
-      }
       check {
         dependsOn(testing.suites)
       }
@@ Expand Down @@