Move clientIp extraction to HttpServerAttributesExtractor (#4301)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

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

@@ -5,14 +5,10 @@
 
 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.HttpServerAttributesExtractor;
 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> {
 
@@ -21,7 +17,7 @@ final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
 
   ServerInstrumenter(
       InstrumenterBuilder<REQUEST, RESPONSE> builder, TextMapGetter<REQUEST> getter) {
-    super(addClientIpExtractor(builder, getter));
+    super(builder);
     this.propagators = builder.openTelemetry.getPropagators();
     this.getter = getter;
   }
@@ -33,125 +29,4 @@ final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
     Context extracted = propagators.getTextMapPropagator().extract(parentContext, request, getter);
     return super.start(extracted, request);
   }
-
-  // TODO: move that to HttpServerAttributesExtractor, now that we have a method for extracting
-  // header values there
-  private static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> addClientIpExtractor(
-      InstrumenterBuilder<REQUEST, RESPONSE> builder, TextMapGetter<REQUEST> getter) {
-    HttpServerAttributesExtractor<REQUEST, RESPONSE> httpAttributesExtractor = null;
-    for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor :
-        builder.attributesExtractors) {
-      if (extractor instanceof HttpServerAttributesExtractor) {
-        httpAttributesExtractor = (HttpServerAttributesExtractor<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));
-    return builder;
-  }
-
-  private static class HttpClientIpExtractor<REQUEST, RESPONSE>
-      extends AttributesExtractor<REQUEST, RESPONSE> {
-
-    private final TextMapGetter<REQUEST> getter;
-
-    HttpClientIpExtractor(TextMapGetter<REQUEST> getter) {
-      this.getter = getter;
-    }
-
-    @Override
-    protected void onStart(AttributesBuilder attributes, REQUEST request) {}
-
-    @Override
-    protected void onEnd(
-        AttributesBuilder attributes,
-        REQUEST request,
-        @Nullable RESPONSE response,
-        @Nullable Throwable error) {
-      String clientIp = getForwardedClientIp(request);
-      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) {
-        return extractForwardedFor(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/ForwarderHeaderParser.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.http;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+final class ForwarderHeaderParser {
+
+  // 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);
+  }
+
+  private ForwarderHeaderParser() {}
+}

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

@@ -89,8 +89,7 @@ public abstract class HttpCommonAttributesExtractor<REQUEST, RESPONSE>
 
   @Nullable
   private String userAgent(REQUEST request) {
-    List<String> values = requestHeader(request, "user-agent");
+    return firstHeaderValue(requestHeader(request, "user-agent"));
-    return values.isEmpty() ? null : values.get(0);
   }
 
   /**
@@ -161,4 +160,9 @@ public abstract class HttpCommonAttributesExtractor<REQUEST, RESPONSE>
    * returned instead.
    */
   protected abstract List<String> responseHeader(REQUEST request, RESPONSE response, String name);
+
+  @Nullable
+  static String firstHeaderValue(List<String> values) {
+    return values.isEmpty() ? null : values.get(0);
+  }
 }

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

@@ -5,11 +5,13 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
+import static io.opentelemetry.instrumentation.api.instrumenter.http.ForwarderHeaderParser.extractForwarded;
+import static io.opentelemetry.instrumentation.api.instrumenter.http.ForwarderHeaderParser.extractForwardedFor;
+
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
-import java.util.List;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 /**
@@ -43,6 +45,7 @@ public abstract class HttpServerAttributesExtractor<REQUEST, RESPONSE>
     set(attributes, SemanticAttributes.HTTP_HOST, host(request));
     set(attributes, SemanticAttributes.HTTP_TARGET, target(request));
     set(attributes, SemanticAttributes.HTTP_ROUTE, route(request));
+    set(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request));
   }
 
   @Override
@@ -66,8 +69,7 @@ public abstract class HttpServerAttributesExtractor<REQUEST, RESPONSE>
 
   @Nullable
   private String host(REQUEST request) {
-    List<String> values = requestHeader(request, "host");
+    return firstHeaderValue(requestHeader(request, "host"));
-    return values.isEmpty() ? null : values.get(0);
   }
 
   @Nullable
@@ -76,6 +78,26 @@ public abstract class HttpServerAttributesExtractor<REQUEST, RESPONSE>
   @Nullable
   protected abstract String scheme(REQUEST request);
 
+  @Nullable
+  private String clientIp(REQUEST request) {
+    // try Forwarded
+    String forwarded = firstHeaderValue(requestHeader(request, "forwarded"));
+    if (forwarded != null) {
+      forwarded = extractForwarded(forwarded);
+      if (forwarded != null) {
+        return forwarded;
+      }
+    }
+
+    // try X-Forwarded-For
+    forwarded = firstHeaderValue(requestHeader(request, "x-forwarded-for"));
+    if (forwarded != null) {
+      return extractForwardedFor(forwarded);
+    }
+
+    return null;
+  }
+
   // Attributes which are not always available when the request is ready.
 
   /**

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

@@ -60,8 +60,7 @@ 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 =
@@ -297,49 +296,8 @@ class InstrumenterTest {
                             .hasAttributesSatisfying(
                                 attributes ->
                                     assertThat(attributes)
-                                        .containsEntry(SemanticAttributes.NET_PEER_IP, "2.2.2.2")
                                         .containsEntry(
-                                            SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"))));
+                                            SemanticAttributes.NET_PEER_IP, "2.2.2.2"))));
-  }
-
-  @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(
-                mockHttpServerAttributes,
-                new ConstantNetPeerIpExtractor<>("2.2.2.2"),
-                new AttributesExtractor1(),
-                new AttributesExtractor2())
-            .addSpanLinksExtractor(new LinksExtractor())
-            .newServerInstrumenter(new MapGetter());
-
-    Map<String, String> request = new HashMap<>(REQUEST);
-    request.remove("Forwarded");
-    request.put("X-Forwarded-For", "1.1.1.1");
-
-    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, "1.1.1.1"))));
   }
 
   @Test
@@ -517,220 +475,6 @@ class InstrumenterTest {
     assertThat(context.get(testKey)).isEqualTo("testVal");
   }
 
-  @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");
-  }
-
   @Test
   void clientNestedSpansSuppressed_whenInstrumentationTypeDisabled() {
     // this test depends on default config option for InstrumentationType

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

@@ -0,0 +1,232 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.http;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.jupiter.api.Test;
+
+class ForwarderHeaderParserTest {
+
+  @Test
+  void extractForwarded() {
+    assertThat(ForwarderHeaderParser.extractForwarded("for=1.1.1.1")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedIpv6() {
+    assertThat(
+            ForwarderHeaderParser.extractForwarded(
+                "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedWithPort() {
+    assertThat(ForwarderHeaderParser.extractForwarded("for=\"1.1.1.1:2222\"")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedIpv6WithPort() {
+    assertThat(
+            ForwarderHeaderParser.extractForwarded(
+                "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedCaps() {
+    assertThat(ForwarderHeaderParser.extractForwarded("For=1.1.1.1")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMalformed() {
+    assertThat(ForwarderHeaderParser.extractForwarded("for=;for=1.1.1.1")).isNull();
+  }
+
+  @Test
+  void extractForwardedEmpty() {
+    assertThat(ForwarderHeaderParser.extractForwarded("")).isNull();
+  }
+
+  @Test
+  void extractForwardedEmptyValue() {
+    assertThat(ForwarderHeaderParser.extractForwarded("for=")).isNull();
+  }
+
+  @Test
+  void extractForwardedEmptyValueWithSemicolon() {
+    assertThat(ForwarderHeaderParser.extractForwarded("for=;")).isNull();
+  }
+
+  @Test
+  void extractForwardedNoFor() {
+    assertThat(ForwarderHeaderParser.extractForwarded("by=1.1.1.1;test=1.1.1.1")).isNull();
+  }
+
+  @Test
+  void extractForwardedMultiple() {
+    assertThat(ForwarderHeaderParser.extractForwarded("for=1.1.1.1;for=1.2.3.4"))
+        .isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMultipleIpV6() {
+    assertThat(
+            ForwarderHeaderParser.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(ForwarderHeaderParser.extractForwarded("for=\"1.1.1.1:2222\";for=1.2.3.4"))
+        .isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedMultipleIpV6WithPort() {
+    assertThat(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.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(ForwarderHeaderParser.extractForwardedFor("1.1.1.1")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForIpv6() {
+    assertThat(
+            ForwarderHeaderParser.extractForwardedFor(
+                "\"[1111:1111:1111:1111:1111:1111:1111:1111]\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForIpv6Unquoted() {
+    assertThat(
+            ForwarderHeaderParser.extractForwardedFor("[1111:1111:1111:1111:1111:1111:1111:1111]"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForIpv6Unbracketed() {
+    assertThat(ForwarderHeaderParser.extractForwardedFor("1111:1111:1111:1111:1111:1111:1111:1111"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForWithPort() {
+    assertThat(ForwarderHeaderParser.extractForwardedFor("1.1.1.1:2222")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForIpv6WithPort() {
+    assertThat(
+            ForwarderHeaderParser.extractForwardedFor(
+                "\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\""))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForIpv6UnquotedWithPort() {
+    assertThat(
+            ForwarderHeaderParser.extractForwardedFor(
+                "[1111:1111:1111:1111:1111:1111:1111:1111]:2222"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+
+  @Test
+  void extractForwardedForEmpty() {
+    assertThat(ForwarderHeaderParser.extractForwardedFor("")).isNull();
+  }
+
+  @Test
+  void extractForwardedForMultiple() {
+    assertThat(ForwarderHeaderParser.extractForwardedFor("1.1.1.1,1.2.3.4")).isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6() {
+    assertThat(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.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(ForwarderHeaderParser.extractForwardedFor("1.1.1.1:2222,1.2.3.4"))
+        .isEqualTo("1.1.1.1");
+  }
+
+  @Test
+  void extractForwardedForMultipleIpv6WithPort() {
+    assertThat(
+            ForwarderHeaderParser.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(
+            ForwarderHeaderParser.extractForwardedFor(
+                "[1111:1111:1111:1111:1111:1111:1111:1111]:2222,1.2.3.4"))
+        .isEqualTo("1111:1111:1111:1111:1111:1111:1111:1111");
+  }
+}

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

@@ -7,6 +7,7 @@ package io.opentelemetry.instrumentation.api.instrumenter.http;
 
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.entry;
 
@@ -55,23 +56,27 @@ class HttpServerAttributesExtractorTest {
 
     @Override
     protected List<String> requestHeader(Map<String, String> request, String name) {
-      return asList(request.get("header." + name).split(","));
+      String values = request.get("header." + name);
+      return values == null ? emptyList() : asList(values.split(","));
     }
 
     @Override
     protected Long requestContentLength(Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(request.get("requestContentLength"));
+      String value = request.get("requestContentLength");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected Long requestContentLengthUncompressed(
         Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(request.get("requestContentLengthUncompressed"));
+      String value = request.get("requestContentLengthUncompressed");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected Integer statusCode(Map<String, String> request, Map<String, String> response) {
-      return Integer.parseInt(response.get("statusCode"));
+      String value = response.get("statusCode");
+      return value == null ? null : Integer.parseInt(value);
     }
 
     @Override
@@ -82,19 +87,22 @@ class HttpServerAttributesExtractorTest {
     @Override
     protected Long responseContentLength(
         Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(response.get("responseContentLength"));
+      String value = response.get("responseContentLength");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected Long responseContentLengthUncompressed(
         Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(response.get("responseContentLengthUncompressed"));
+      String value = response.get("responseContentLengthUncompressed");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected List<String> responseHeader(
         Map<String, String> request, Map<String, String> response, String name) {
-      return asList(response.get("header." + name).split(","));
+      String values = response.get("header." + name);
+      return values == null ? emptyList() : asList(values.split(","));
     }
   }
 
@@ -112,6 +120,7 @@ class HttpServerAttributesExtractorTest {
     request.put("serverName", "server");
     request.put("header.user-agent", "okhttp 3.x");
     request.put("header.host", "github.com");
+    request.put("header.forwarded", "for=1.1.1.1");
     request.put("header.custom-request-header", "123,456");
 
     Map<String, String> response = new HashMap<>();
@@ -136,6 +145,7 @@ class HttpServerAttributesExtractorTest {
             entry(SemanticAttributes.HTTP_TARGET, "/repositories/1"),
             entry(SemanticAttributes.HTTP_USER_AGENT, "okhttp 3.x"),
             entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{id}"),
+            entry(SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"),
             entry(
                 AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
                 asList("123", "456")));
@@ -149,6 +159,7 @@ class HttpServerAttributesExtractorTest {
             entry(SemanticAttributes.HTTP_TARGET, "/repositories/1"),
             entry(SemanticAttributes.HTTP_USER_AGENT, "okhttp 3.x"),
             entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{id}"),
+            entry(SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"),
             entry(
                 AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
                 asList("123", "456")),
@@ -163,4 +174,22 @@ class HttpServerAttributesExtractorTest {
                 AttributeKey.stringArrayKey("http.response.header.custom_response_header"),
                 asList("654", "321")));
   }
+
+  @Test
+  void extractClientIpFromX_Forwarded_For() {
+    Map<String, String> request = new HashMap<>();
+    request.put("header.x-forwarded-for", "1.1.1.1");
+
+    TestHttpServerAttributesExtractor extractor =
+        new TestHttpServerAttributesExtractor(CapturedHttpHeaders.empty());
+
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, request);
+    assertThat(attributes.build())
+        .containsOnly(entry(SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"));
+
+    extractor.onEnd(attributes, request, null, null);
+    assertThat(attributes.build())
+        .containsOnly(entry(SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"));
+  }
 }