Fill HTTP_CLIENT_IP in ServerInstrumenter (#3756)

benchmark/src/jmh/java/io/opentelemetry/benchmark/InstrumenterBenchmark.java

@@ -113,11 +113,6 @@ public class InstrumenterBenchmark {
       return null;
     }
 
-    @Override
-    protected @Nullable String clientIp(Void unused, @Nullable Void unused2) {
-      return null;
-    }
-
     @Override
     protected @Nullable Integer statusCode(Void unused, Void unused2) {
       return 200;

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java

@@ -5,10 +5,15 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter;
 
+import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 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;
 
 final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST, RESPONSE> {
 
@@ -17,7 +22,7 @@ final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
 
   ServerInstrumenter(
       InstrumenterBuilder<REQUEST, RESPONSE> builder, TextMapGetter<REQUEST> getter) {
-    super(builder);
+    super(addClientIpExtractor(builder, getter));
     this.propagators = builder.openTelemetry.getPropagators();
     this.getter = getter;
   }
@@ -29,4 +34,133 @@ final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
     Context extracted = propagators.getTextMapPropagator().extract(parentContext, request, getter);
     return super.start(extracted, 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) {
+        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));
+    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) {
+      this.getter = getter;
+      this.netAttributesExtractor = netAttributesExtractor;
+    }
+
+    @Override
+    protected void onStart(AttributesBuilder attributes, REQUEST request) {}
+
+    @Override
+    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);
+    }
+
+    @Nullable
+    // Visible for testing
+    String getForwardedClientIp(REQUEST request) {
+      // try Forwarded
+      String forwarded = getter.get(request, "Forwarded");
+      if (forwarded != null) {
+        forwarded = extractForwarded(forwarded);
+        if (forwarded != null) {
+          return forwarded;
+        }
+      }
+
+      // try X-Forwarded-For
+      forwarded = getter.get(request, "X-Forwarded-For");
+      if (forwarded != null) {
+        forwarded = extractForwardedFor(forwarded);
+        if (forwarded != null) {
+          return forwarded;
+        }
+      }
+
+      return null;
+    }
+  }
+
+  // VisibleForTesting
+  @Nullable
+  static String extractForwarded(String forwarded) {
+    int start = forwarded.toLowerCase().indexOf("for=");
+    if (start < 0) {
+      return null;
+    }
+    start += 4; // start is now the index after for=
+    if (start >= forwarded.length() - 1) { // the value after for= must not be empty
+      return null;
+    }
+    return extractIpAddress(forwarded, start);
+  }
+
+  // VisibleForTesting
+  @Nullable
+  static String extractForwardedFor(String forwarded) {
+    return extractIpAddress(forwarded, 0);
+  }
+
+  // from https://www.rfc-editor.org/rfc/rfc7239
+  //  "Note that IPv6 addresses may not be quoted in
+  //   X-Forwarded-For and may not be enclosed by square brackets, but they
+  //   are quoted and enclosed in square brackets in Forwarded"
+  // and also (applying to Forwarded but not X-Forwarded-For)
+  //  "It is important to note that an IPv6 address and any nodename with
+  //   node-port specified MUST be quoted, since ':' is not an allowed
+  //   character in 'token'."
+  @Nullable
+  private static String extractIpAddress(String forwarded, int start) {
+    if (forwarded.length() == start) {
+      return null;
+    }
+    if (forwarded.charAt(start) == '"') {
+      return extractIpAddress(forwarded, start + 1);
+    }
+    if (forwarded.charAt(start) == '[') {
+      int end = forwarded.indexOf(']', start + 1);
+      if (end == -1) {
+        return null;
+      }
+      return forwarded.substring(start + 1, end);
+    }
+    boolean inIpv4 = false;
+    for (int i = start; i < forwarded.length() - 1; i++) {
+      char c = forwarded.charAt(i);
+      if (c == '.') {
+        inIpv4 = true;
+      } else if (c == ',' || c == ';' || c == '"' || (inIpv4 && c == ':')) {
+        if (i == start) { // empty string
+          return null;
+        }
+        return forwarded.substring(start, i);
+      }
+    }
+    return forwarded.substring(start);
+  }
 }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpAttributesExtractor.java

@@ -48,7 +48,6 @@ public abstract class HttpAttributesExtractor<REQUEST, RESPONSE>
         requestContentLengthUncompressed(request, response));
     set(attributes, SemanticAttributes.HTTP_FLAVOR, flavor(request, response));
     set(attributes, SemanticAttributes.HTTP_SERVER_NAME, serverName(request, response));
-    set(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request, response));
     if (response != null) {
       Integer statusCode = statusCode(request, response);
       if (statusCode != null) {
@@ -127,15 +126,6 @@ public abstract class HttpAttributesExtractor<REQUEST, RESPONSE>
   @Nullable
   protected abstract String serverName(REQUEST request, @Nullable RESPONSE response);
 
-  /**
-   * Extracts the {@code http.client_ip} span attribute.
-   *
-   * <p>This is called from {@link Instrumenter#end(Context, Object, Object, Throwable)}, whether
-   * {@code response} is {@code null} or not.
-   */
-  @Nullable
-  protected abstract String clientIp(REQUEST request, @Nullable RESPONSE response);
-
   /**
    * Extracts the {@code http.status_code} span attribute.
    *

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java

@@ -8,6 +8,7 @@ package io.opentelemetry.instrumentation.api.instrumenter;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
 import static org.assertj.core.api.Assertions.entry;
+import static org.mockito.Mockito.when;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.api.trace.Span;
@@ -20,11 +21,14 @@ import io.opentelemetry.api.trace.TraceState;
 import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
 import io.opentelemetry.context.Context;
 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.tracer.ServerSpan;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
 import io.opentelemetry.sdk.trace.data.LinkData;
 import io.opentelemetry.sdk.trace.data.StatusData;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.time.Instant;
 import java.util.Collections;
 import java.util.HashMap;
@@ -32,8 +36,12 @@ import java.util.Map;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 
+@ExtendWith(MockitoExtension.class)
 class InstrumenterTest {
   private static final String LINK_TRACE_ID = TraceId.fromLongs(0, 42);
   private static final String LINK_SPAN_ID = SpanId.fromLong(123);
@@ -46,7 +54,8 @@ class InstrumenterTest {
                   entry("req2_2", "req2_2_value"),
                   entry("req3", "req3_value"),
                   entry("linkTraceId", LINK_TRACE_ID),
-                  entry("linkSpanId", LINK_SPAN_ID))
+                  entry("linkSpanId", LINK_SPAN_ID),
+                  entry("Forwarded", "for=1.1.1.1"))
               .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
 
   private static final Map<String, String> RESPONSE =
@@ -119,6 +128,10 @@ class InstrumenterTest {
   @RegisterExtension
   static final OpenTelemetryExtension otelTesting = OpenTelemetryExtension.create();
 
+  @Mock HttpAttributesExtractor<Map<String, String>, Map<String, String>> mockHttpAttributes;
+
+  @Mock NetAttributesExtractor<Map<String, String>, Map<String, String>> mockNetAttributes;
+
   @Test
   void server() {
     Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
@@ -228,6 +241,130 @@ class InstrumenterTest {
                             .hasParentSpanId("090a0b0c0d0e0f00")));
   }
 
+  @Test
+  void server_http() {
+    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())
+            .addSpanLinkExtractor(new LinkExtractor())
+            .newServerInstrumenter(new MapGetter());
+
+    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(ServerSpan.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, "1.1.1.1"))));
+  }
+
+  @Test
+  void server_http_xForwardedFor() {
+    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())
+            .addSpanLinkExtractor(new LinkExtractor())
+            .newServerInstrumenter(new MapGetter());
+
+    Map<String, String> request = new HashMap<>(REQUEST);
+    request.remove("Forwarded");
+    request.put("X-Forwarded-For", "1.1.1.1");
+
+    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(ServerSpan.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, "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())
+            .addSpanLinkExtractor(new LinkExtractor())
+            .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(ServerSpan.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 =
@@ -384,6 +521,220 @@ class InstrumenterTest {
                     span -> span.hasName("test span").hasTotalRecordedLinks(0)));
   }
 
+  @Test
+  void extractForwarded() {
+    assertThat(ServerInstrumenter.extractForwarded("for=1.1.1.1")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedIpv6() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedWithPort() {
+    assertThat(ServerInstrumenter.extractForwarded("for=\"1.1.1.1:2222\"")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedIpv6WithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedCaps() {
+    assertThat(ServerInstrumenter.extractForwarded("For=1.1.1.1")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMalformed() {
+    assertThat(ServerInstrumenter.extractForwarded("for=;for=1.1.1.1")).isNull();
+  }
+
+  @Test
+  void extractForwardedEmpty() {
+    assertThat(ServerInstrumenter.extractForwarded("")).isNull();
+  }
+
+  @Test
+  void extractForwardedEmptyValue() {
+    assertThat(ServerInstrumenter.extractForwarded("for=")).isNull();
+  }
+
+  @Test
+  void extractForwardedEmptyValueWithSemicolon() {
+    assertThat(ServerInstrumenter.extractForwarded("for=;")).isNull();
+  }
+
+  @Test
+  void extractForwardedNoFor() {
+    assertThat(ServerInstrumenter.extractForwarded("by=1.1.1.1;test=1.1.1.1")).isNull();
+  }
+
+  @Test
+  void extractForwardedMultiple() {
+    assertThat(ServerInstrumenter.extractForwarded("for=1.1.1.1;for=1.2.3.4")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMultipleIpV6() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\";for=1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedMultipleWithPort() {
+    assertThat(ServerInstrumenter.extractForwarded("for=\"1.1.1.1:2222\";for=1.2.3.4"))
+        .isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMultipleIpV6WithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\";for=1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedMixedSplitter() {
+    assertThat(
+            ServerInstrumenter.extractForwarded("test=abcd; by=1.2.3.4, for=1.1.1.1;for=1.2.3.4"))
+        .isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMixedSplitterIpv6() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "test=abcd; by=1.2.3.4, for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\";for=1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedMixedSplitterWithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "test=abcd; by=1.2.3.4, for=\"1.1.1.1:2222\";for=1.2.3.4"))
+        .isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMixedSplitterIpv6WithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwarded(
+                "test=abcd; by=1.2.3.4, for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\";for=1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedFor() {
+    assertThat(ServerInstrumenter.extractForwardedFor("1.1.1.1")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForIpv6() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor("\"[1111:1111:1111:1111:1111:1111:1111:1111]\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForIpv6Unquoted() {
+    assertThat(ServerInstrumenter.extractForwardedFor("[1111:1111:1111:1111:1111:1111:1111:1111]"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForIpv6Unbracketed() {
+    assertThat(ServerInstrumenter.extractForwardedFor("1111:1111:1111:1111:1111:1111:1111:1111"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForWithPort() {
+    assertThat(ServerInstrumenter.extractForwardedFor("1.1.1.1:2222")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForIpv6WithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForIpv6UnquotedWithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "[1111:1111:1111:1111:1111:1111:1111:1111]:2222"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForEmpty() {
+    assertThat(ServerInstrumenter.extractForwardedFor("")).isNull();
+  }
+
+  @Test
+  void extractForwardedForMultiple() {
+    assertThat(ServerInstrumenter.extractForwardedFor("1.1.1.1,1.2.3.4")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "\"[1111:1111:1111:1111:1111:1111:1111:1111]\",1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6Unquoted() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "[1111:1111:1111:1111:1111:1111:1111:1111],1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6Unbracketed() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "1111:1111:1111:1111:1111:1111:1111:1111,1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForMultipleWithPort() {
+    assertThat(ServerInstrumenter.extractForwardedFor("1.1.1.1:2222,1.2.3.4")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6WithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\",1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6UnquotedWithPort() {
+    assertThat(
+            ServerInstrumenter.extractForwardedFor(
+                "[1111:1111:1111:1111:1111:1111:1111:1111]:2222,1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
   private static LinkData expectedSpanLink() {
     return LinkData.create(
         SpanContext.create(

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpAttributesExtractorTest.java

@@ -92,11 +92,6 @@ class HttpAttributesExtractorTest {
     protected String serverName(Map<String, String> request, Map<String, String> response) {
       return request.get("serverName");
     }
-
-    @Override
-    protected String clientIp(Map<String, String> request, Map<String, String> response) {
-      return request.get("clientIp");
-    }
   }
 
   @Test
@@ -113,7 +108,6 @@ class HttpAttributesExtractorTest {
     request.put("requestContentLengthUncompressed", "11");
     request.put("flavor", "http/2");
     request.put("serverName", "server");
-    request.put("clientIp", "1.2.3.4");
 
     Map<String, String> response = new HashMap<>();
     response.put("statusCode", "202");
@@ -147,7 +141,6 @@ class HttpAttributesExtractorTest {
             entry(SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED, 11L),
             entry(SemanticAttributes.HTTP_FLAVOR, "http/2"),
             entry(SemanticAttributes.HTTP_SERVER_NAME, "server"),
-            entry(SemanticAttributes.HTTP_CLIENT_IP, "1.2.3.4"),
             entry(SemanticAttributes.HTTP_STATUS_CODE, 202L),
             entry(SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH, 20L),
             entry(SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED, 21L));

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesExtractor.java

@@ -97,10 +97,4 @@ final class ApacheHttpAsyncClientHttpAttributesExtractor
   protected String route(ApacheHttpClientRequest request) {
     return null;
   }
-
-  @Override
-  @Nullable
-  protected String clientIp(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
-    return null;
-  }
 }

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v2_0/ApacheHttpClientHttpAttributesExtractor.java

@@ -116,12 +116,6 @@ final class ApacheHttpClientHttpAttributesExtractor
     return null;
   }
 
-  @Override
-  @Nullable
-  protected String clientIp(HttpMethod request, @Nullable HttpMethod response) {
-    return null;
-  }
-
   // mirroring implementation HttpMethodBase.getURI(), to avoid converting to URI and back to String
   private static String getUrl(HttpMethod request) {
     HostConfiguration hostConfiguration = request.getHostConfiguration();

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientHttpAttributesExtractor.java

@@ -94,10 +94,4 @@ final class ApacheHttpClientHttpAttributesExtractor
   protected String route(ApacheHttpClientRequest request) {
     return null;
   }
-
-  @Override
-  @Nullable
-  protected String clientIp(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
-    return null;
-  }
 }

instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientHttpAttributesExtractor.java

@@ -96,10 +96,4 @@ final class ApacheHttpClientHttpAttributesExtractor
   protected String route(ApacheHttpClientRequest request) {
     return null;
   }
-
-  @Override
-  @Nullable
-  protected String clientIp(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
-    return null;
-  }
 }

instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpClientHttpAttributesExtractor.java

@@ -156,10 +156,4 @@ final class ApacheHttpClientHttpAttributesExtractor
   protected String route(ClassicHttpRequest request) {
     return null;
   }
-
-  @Override
-  @Nullable
-  protected String clientIp(ClassicHttpRequest request, @Nullable HttpResponse response) {
-    return null;
-  }
 }

instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpAttributesExtractor.java

@@ -117,12 +117,6 @@ final class ArmeriaHttpAttributesExtractor
     return null;
   }
 
-  @Override
-  @Nullable
-  protected String clientIp(RequestContext ctx, @Nullable RequestLog requestLog) {
-    return null;
-  }
-
   private static HttpRequest request(RequestContext ctx) {
     HttpRequest request = ctx.request();
     if (request == null) {

instrumentation/async-http-client/async-http-client-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/asynchttpclient/v2_0/AsyncHttpClientHttpAttributesExtractor.java

@@ -99,10 +99,4 @@ final class AsyncHttpClientHttpAttributesExtractor
   protected String route(Request request) {
     return null;
   }
-
-  @Override
-  @Nullable
-  protected String clientIp(Request request, @Nullable Response response) {
-    return null;
-  }
 }

instrumentation/jetty-httpclient/jetty-httpclient-9.2/library/src/main/java/io/opentelemetry/instrumentation/jetty/httpclient/v9_2/internal/JettyClientHttpAttributesExtractor.java

@@ -111,12 +111,6 @@ final class JettyClientHttpAttributesExtractor extends HttpAttributesExtractor<R
     return null;
   }
 
-  @Override
-  @Nullable
-  protected String clientIp(Request request, @Nullable Response response) {
-    return null;
-  }
-
   @Override
   @Nullable
   protected Integer statusCode(Request request, Response response) {

instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpAttributesExtractor.java

@@ -82,11 +82,6 @@ final class OkHttpAttributesExtractor extends HttpAttributesExtractor<Request, R
     return null;
   }
 
-  @Override
-  protected @Nullable String clientIp(Request request, @Nullable Response response) {
-    return null;
-  }
-
   @Override
   protected Integer statusCode(Request request, Response response) {
     return response.code();

instrumentation/spring/spring-web-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/web/SpringWebHttpAttributesExtractor.java

@@ -74,12 +74,6 @@ final class SpringWebHttpAttributesExtractor
     return null;
   }
 
-  @Override
-  protected @Nullable String clientIp(
-      HttpRequest httpRequest, @Nullable ClientHttpResponse clientHttpResponse) {
-    return null;
-  }
-
   @Override
   protected Integer statusCode(HttpRequest httpRequest, ClientHttpResponse clientHttpResponse) {
     try {