Follow http.client_ip spec clarification (#4063)

open-telemetry · Sep 8, 2021 · 400d994 · 400d994
1 parent 1a994b9
commit 400d994
Show file tree

Hide file tree

Showing 12 changed files with 7 additions and 95 deletions.
diff --git a/...i/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java b/...i/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java
@@ -10,7 +10,6 @@
 import io.opentelemetry.context.propagation.ContextPropagators;
 import io.opentelemetry.context.propagation.TextMapGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
-import io.opentelemetry.instrumentation.api.instrumenter.net.NetAttributesExtractor;
 import io.opentelemetry.instrumentation.api.internal.ContextPropagationDebug;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -38,34 +37,27 @@ public Context start(Context parentContext, REQUEST request) {
   private static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> addClientIpExtractor(
       InstrumenterBuilder<REQUEST, RESPONSE> builder, TextMapGetter<REQUEST> getter) {
     HttpAttributesExtractor<REQUEST, RESPONSE> httpAttributesExtractor = null;
-    NetAttributesExtractor<REQUEST, RESPONSE> netAttributesExtractor = null;
     for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor :
         builder.attributesExtractors) {
-      if (extractor instanceof NetAttributesExtractor) {
-        netAttributesExtractor = (NetAttributesExtractor<REQUEST, RESPONSE>) extractor;
-      } else if (extractor instanceof HttpAttributesExtractor) {
+      if (extractor instanceof HttpAttributesExtractor) {
         httpAttributesExtractor = (HttpAttributesExtractor<REQUEST, RESPONSE>) extractor;
       }
     }
     if (httpAttributesExtractor == null) {
       // Don't add HTTP_CLIENT_IP if there are no HTTP attributes registered.
       return builder;
     }
-    builder.addAttributesExtractor(new HttpClientIpExtractor(getter, netAttributesExtractor));
+    builder.addAttributesExtractor(new HttpClientIpExtractor<>(getter));
     return builder;
   }
 
   private static class HttpClientIpExtractor<REQUEST, RESPONSE>
       extends AttributesExtractor<REQUEST, RESPONSE> {
 
     private final TextMapGetter<REQUEST> getter;
-    @Nullable private final NetAttributesExtractor<REQUEST, RESPONSE> netAttributesExtractor;
 
-    HttpClientIpExtractor(
-        TextMapGetter<REQUEST> getter,
-        @Nullable NetAttributesExtractor<REQUEST, RESPONSE> netAttributesExtractor) {
+    HttpClientIpExtractor(TextMapGetter<REQUEST> getter) {
       this.getter = getter;
-      this.netAttributesExtractor = netAttributesExtractor;
     }
 
     @Override
@@ -75,9 +67,6 @@ protected void onStart(AttributesBuilder attributes, REQUEST request) {}
     protected void onEnd(
         AttributesBuilder attributes, REQUEST request, @Nullable RESPONSE response) {
       String clientIp = getForwardedClientIp(request);
-      if (clientIp == null && netAttributesExtractor != null) {
-        clientIp = netAttributesExtractor.peerIp(request, response);
-      }
       set(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp);
     }
 
@@ -96,10 +85,7 @@ String getForwardedClientIp(REQUEST request) {
       // try X-Forwarded-For
       forwarded = getter.get(request, "X-Forwarded-For");
       if (forwarded != null) {
-        forwarded = extractForwardedFor(forwarded);
-        if (forwarded != null) {
-          return forwarded;
-        }
+        return extractForwardedFor(forwarded);
       }
 
       return null;

diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java
@@ -199,10 +199,10 @@ protected void onConnectionAndRequest(
       }
       spanBuilder.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
     }
-    spanBuilder.setAttribute(SemanticAttributes.HTTP_CLIENT_IP, clientIp(connection, request));
+    spanBuilder.setAttribute(SemanticAttributes.HTTP_CLIENT_IP, clientIp(request));
   }
 
-  private String clientIp(CONNECTION connection, REQUEST request) {
+  private String clientIp(REQUEST request) {
     // try Forwarded
     String forwarded = requestHeader(request, "Forwarded");
     if (forwarded != null) {
@@ -221,8 +221,7 @@ private String clientIp(CONNECTION connection, REQUEST request) {
       }
     }
 
-    // fallback to peer IP if there are no proxy headers
-    return peerHostIp(connection);
+    return null;
   }
 
   // VisibleForTesting

diff --git a/...api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java b/...api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java
@@ -333,48 +333,6 @@ void server_http_xForwardedFor() {
                                             SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"))));
   }
 
-  @Test
-  void server_http_noForwarded() {
-    Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
-        Instrumenter.<Map<String, String>, Map<String, String>>newBuilder(
-                otelTesting.getOpenTelemetry(), "test", unused -> "span")
-            .addAttributesExtractors(
-                mockHttpAttributes,
-                mockNetAttributes,
-                new AttributesExtractor1(),
-                new AttributesExtractor2())
-            .addSpanLinksExtractor(new LinksExtractor())
-            .newServerInstrumenter(new MapGetter());
-
-    Map<String, String> request = new HashMap<>(REQUEST);
-    request.remove("Forwarded");
-
-    when(mockNetAttributes.peerIp(request, null)).thenReturn("2.2.2.2");
-    when(mockNetAttributes.peerIp(request, RESPONSE)).thenReturn("2.2.2.2");
-
-    Context context = instrumenter.start(Context.root(), request);
-    SpanContext spanContext = Span.fromContext(context).getSpanContext();
-
-    assertThat(spanContext.isValid()).isTrue();
-    assertThat(SpanKey.SERVER.fromContextOrNull(context).getSpanContext()).isEqualTo(spanContext);
-
-    instrumenter.end(context, request, RESPONSE, null);
-
-    otelTesting
-        .assertTraces()
-        .hasTracesSatisfyingExactly(
-            trace ->
-                trace.hasSpansSatisfyingExactly(
-                    span ->
-                        span.hasName("span")
-                            .hasAttributesSatisfying(
-                                attributes ->
-                                    assertThat(attributes)
-                                        .containsEntry(SemanticAttributes.NET_PEER_IP, "2.2.2.2")
-                                        .containsEntry(
-                                            SemanticAttributes.HTTP_CLIENT_IP, "2.2.2.2"))));
-  }
-
   @Test
   void client() {
     Instrumenter<Map<String, String>, Map<String, String>> instrumenter =

diff --git a/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy b/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy
@@ -89,7 +89,6 @@ class RestCamelTest extends AgentInstrumentationSpecification implements RetryOn
           attributes {
             "$SemanticAttributes.HTTP_URL.key" "http://localhost:$port/api/firstModule/unit/unitOne"
             "$SemanticAttributes.HTTP_STATUS_CODE.key" 200
-            "$SemanticAttributes.HTTP_CLIENT_IP.key" "127.0.0.1"
             "$SemanticAttributes.HTTP_USER_AGENT.key" String
             "$SemanticAttributes.HTTP_FLAVOR.key" "1.1"
             "$SemanticAttributes.HTTP_METHOD.key" "GET"

diff --git a/...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy b/...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy
@@ -129,7 +129,6 @@ class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecifica
             "$SemanticAttributes.NET_PEER_IP.key" "127.0.0.1"
             "$SemanticAttributes.HTTP_USER_AGENT.key" "Jakarta Commons-HttpClient/3.1"
             "$SemanticAttributes.HTTP_FLAVOR.key" "1.1"
-            "$SemanticAttributes.HTTP_CLIENT_IP.key" "127.0.0.1"
           }
         }
         it.span(5) {

diff --git a/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationBasicTests.groovy b/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationBasicTests.groovy
@@ -93,7 +93,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -144,7 +143,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -191,7 +189,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -247,7 +244,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 500
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -308,7 +304,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -350,7 +345,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -424,7 +418,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 500
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -467,7 +460,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
       }

diff --git a/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationForwardTests.groovy b/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationForwardTests.groovy
@@ -90,7 +90,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -153,7 +152,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -195,7 +193,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -285,7 +282,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -361,7 +357,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 500
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -416,7 +411,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 404
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {

diff --git a/...n/groovy/io/opentelemetry/instrumentation/ratpack/server/AbstractRatpackRoutesTest.groovy b/...n/groovy/io/opentelemetry/instrumentation/ratpack/server/AbstractRatpackRoutesTest.groovy
@@ -109,7 +109,6 @@ abstract class AbstractRatpackRoutesTest extends InstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" { it == null || it == "127.0.0.1" }
 
             if (extraAttributes.contains(SemanticAttributes.HTTP_HOST)) {
               "${SemanticAttributes.HTTP_HOST}" "localhost:${app.bindPort}"

diff --git a/instrumentation/spark-2.3/javaagent/src/test/groovy/SparkJavaBasedTest.groovy b/instrumentation/spark-2.3/javaagent/src/test/groovy/SparkJavaBasedTest.groovy
@@ -53,7 +53,6 @@ class SparkJavaBasedTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
       }

diff --git a/instrumentation/spring/spring-webflux-5.0/javaagent/src/test/groovy/SpringWebfluxTest.groovy b/instrumentation/spring/spring-webflux-5.0/javaagent/src/test/groovy/SpringWebfluxTest.groovy
@@ -90,7 +90,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -153,7 +152,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -235,7 +233,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -296,7 +293,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 404
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -336,7 +332,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 202
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -381,7 +376,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 500
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -444,7 +438,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 307
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -472,7 +465,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -515,7 +507,6 @@ class SpringWebfluxTest extends AgentInstrumentationSpecification {
               "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
               "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
               "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-              "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
             }
           }
           span(1) {

diff --git a/...vertx-reactive-3.5/javaagent/src/latestDepTest/groovy/VertxReactivePropagationTest.groovy b/...vertx-reactive-3.5/javaagent/src/latestDepTest/groovy/VertxReactivePropagationTest.groovy
@@ -68,7 +68,6 @@ class VertxReactivePropagationTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -155,7 +154,6 @@ class VertxReactivePropagationTest extends AgentInstrumentationSpecification {
               "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
               "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
               "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-              "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
               "${TEST_REQUEST_ID_ATTRIBUTE}" requestId
             }
           }

diff --git a/...vertx-reactive-3.5/javaagent/src/version35Test/groovy/VertxReactivePropagationTest.groovy b/...vertx-reactive-3.5/javaagent/src/version35Test/groovy/VertxReactivePropagationTest.groovy
@@ -68,7 +68,6 @@ class VertxReactivePropagationTest extends AgentInstrumentationSpecification {
             "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
             "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
             "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-            "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
           }
         }
         span(1) {
@@ -155,7 +154,6 @@ class VertxReactivePropagationTest extends AgentInstrumentationSpecification {
               "${SemanticAttributes.HTTP_STATUS_CODE.key}" 200
               "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"
               "${SemanticAttributes.HTTP_USER_AGENT.key}" String
-              "${SemanticAttributes.HTTP_CLIENT_IP.key}" "127.0.0.1"
               "${TEST_REQUEST_ID_ATTRIBUTE}" requestId
             }
           }