Address review comments on httpclient-5.x exclude-port fix

- Use effective port (port(host)) instead of host.getPort() in skipIntercept
  so default ports (-1) resolve correctly to 80/443
- Fix Javadoc on PROPAGATION_EXCLUDE_PORTS to state that tracing is fully
  skipped (no span, no headers), not just header injection
- Update CHANGES.md to match actual behaviour
- Replace atLeastOnce() with times(3) in tests for consistency with
  existing module tests
- Fix misleading Javadoc + add missing non-excluded-port assertion in
  multipleExcludedPorts test

Author: omertt27

CHANGES.md

@@ -7,7 +7,7 @@ Release Notes.
 
 * Added support for Lettuce reactive Redis commands.
 * Add Spring AI 1.x plugin and GenAI layer.
-* Fix httpclient-5.x plugin injecting sw8 propagation headers into ClickHouse HTTP requests (port 8123), causing HTTP 400. Add `PROPAGATION_EXCLUDE_PORTS` config to skip header injection for specified ports.
+* Fix httpclient-5.x plugin injecting sw8 propagation headers into ClickHouse HTTP requests (port 8123), causing HTTP 400. Add `PROPAGATION_EXCLUDE_PORTS` config to skip tracing (including header injection) for specified ports in the classic client interceptor.
 
 All issues and pull requests are [here](https://github.com/apache/skywalking/milestone/249?closed=1)
 

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpClient5PluginConfig.java

@@ -25,14 +25,16 @@ public static class Plugin {
         @PluginConfig(root = HttpClient5PluginConfig.class)
         public static class HttpClient5 {
             /**
-             * Comma-separated list of destination ports whose HTTP requests
-             * should NOT have SkyWalking propagation headers injected.
+             * Comma-separated list of destination ports whose outbound HTTP requests
+             * will be completely skipped by the classic client interceptor: no exit
+             * span is created and no SkyWalking propagation headers are injected.
              *
              * <p>Some HTTP-based database protocols (e.g. ClickHouse on port 8123)
              * reject requests that contain unknown HTTP headers, returning HTTP 400.
-             * Adding such ports here prevents the agent from injecting the {@code sw8}
-             * tracing headers into those outbound requests while leaving all other
-             * HTTP calls fully traced.
+             * Adding such ports here prevents the agent from creating exit spans
+             * and from injecting the {@code sw8} tracing headers into those outbound
+             * requests, meaning these requests are completely untraced by SkyWalking,
+             * while leaving all other HTTP calls fully traced.
              *
              * <p>Default: {@code "8123"} (ClickHouse HTTP interface).
              *

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpClientDoExecuteInterceptor.java

@@ -95,7 +95,7 @@ protected boolean skipIntercept(EnhancedInstance objInst, Method method, Object[
         if (host == null) {
             return true;
         }
-        return isExcludedPort(host.getPort());
+        return isExcludedPort(port(host));
     }
 
     /**

apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpClientPropagationExcludePortTest.java

@@ -151,8 +151,8 @@ public void internalClient_requestToRegularPort_spanCreatedAndHeadersInjected()
 
         List<TraceSegment> segments = segmentStorage.getTraceSegments();
         assertThat("A trace segment must be created for a non-excluded port", segments.size(), is(1));
-        // sw8, sw8-correlation, sw8-x are the 3 propagation headers
-        verify(request, org.mockito.Mockito.atLeastOnce()).setHeader(anyString(), anyString());
+        // sw8, sw8-correlation, sw8-x – exactly 3 propagation headers, consistent with existing tests
+        verify(request, org.mockito.Mockito.times(3)).setHeader(anyString(), anyString());
     }
 
     // -----------------------------------------------------------------------
@@ -183,7 +183,7 @@ public void minimalClient_requestToRegularPort_spanCreatedAndHeadersInjected() t
 
         List<TraceSegment> segments = segmentStorage.getTraceSegments();
         assertThat("A trace segment must be created for a non-excluded port", segments.size(), is(1));
-        verify(request, org.mockito.Mockito.atLeastOnce()).setHeader(anyString(), anyString());
+        verify(request, org.mockito.Mockito.times(3)).setHeader(anyString(), anyString());
     }
 
     // -----------------------------------------------------------------------
@@ -209,20 +209,20 @@ public void whenExcludePortsEmpty_allPortsAreTraced() throws Throwable {
 
     /**
      * Multiple ports can be listed: verify that both excluded ports are silently
-     * skipped while a third, non-excluded port is still traced.
+     * skipped while a non-excluded port is still traced under the same config.
      */
     @Test
-    public void multipleExcludedPorts_allSkipped() throws Throwable {
+    public void multipleExcludedPorts_allSkippedAndNonExcludedStillTraced() throws Throwable {
         HttpClient5PluginConfig.Plugin.HttpClient5.PROPAGATION_EXCLUDE_PORTS = "8123,9200";
 
         HttpClientDoExecuteInterceptor freshInterceptor = new MinimalClientDoExecuteInterceptor();
 
         // 8123 – must be excluded
         freshInterceptor.beforeMethod(enhancedInstance, null, clickHouseArgs, argumentsType, null);
         freshInterceptor.afterMethod(enhancedInstance, null, clickHouseArgs, argumentsType, httpResponse);
-        assertThat(segmentStorage.getTraceSegments().size(), is(0));
+        assertThat("Port 8123 should be excluded", segmentStorage.getTraceSegments().size(), is(0));
 
-        // Set up a mock host on port 9200 (Elasticsearch)
+        // 9200 (Elasticsearch) – must also be excluded
         HttpHost esHost = org.mockito.Mockito.mock(HttpHost.class);
         when(esHost.getHostName()).thenReturn("es-server");
         when(esHost.getSchemeName()).thenReturn("http");
@@ -233,5 +233,12 @@ public void multipleExcludedPorts_allSkipped() throws Throwable {
         freshInterceptor.beforeMethod(enhancedInstance, null, esArgs, argumentsType, null);
         freshInterceptor.afterMethod(enhancedInstance, null, esArgs, argumentsType, httpResponse);
         assertThat("Port 9200 should also be excluded", segmentStorage.getTraceSegments().size(), is(0));
+
+        // 8080 (regular service) – must still be traced under the same multi-port config
+        freshInterceptor = new MinimalClientDoExecuteInterceptor();
+        freshInterceptor.beforeMethod(enhancedInstance, null, regularArgs, argumentsType, null);
+        freshInterceptor.afterMethod(enhancedInstance, null, regularArgs, argumentsType, httpResponse);
+        assertThat("Non-excluded port 8080 must still produce a trace segment",
+                segmentStorage.getTraceSegments().size(), is(1));
     }
 }