`server.port` is required on HTTP client spans by trask · Pull Request #16388 · open-telemetry/opentelemetry-java-instrumentation

trask · 2026-03-04T18:51:37Z

We missed this when moving to stable HTTP semantic conventions.

breedx-splk · 2026-03-04T23:26:38Z

+    if (serverAddressAndPort.getAddress() != null) {
+      attributes.put(SERVER_ADDRESS, serverAddressAndPort.getAddress());
+      Integer port = serverAddressAndPort.getPort();
+      if (port == null || port <= 0) {


pedantic, but....

Suggested change

if (port == null || port <= 0) {

if (port == null || port <= 0 || port > 65535) {

breedx-splk

I know it's Draft but I'll leave a review anyway. This seems good to me, the port is often helpful. I think saying it's "required" might be overstating it (spec says SHOULD?)

trask · 2026-03-04T23:40:13Z

spec says SHOULD?

this is for HTTP server spans

for HTTP client spans it's marked as required

laurit · 2026-03-09T10:03:35Z

-
    String fullUrl = stripSensitiveData(getter.getUrlFull(request));
+
+    if (SemconvStability.v3Preview()) {


just curious why should we make this change only in the next major version, is there something that prevents us from doing it right now?

yeah, it's confusing, due to this:

Adding new attributes to existing metrics that do not "break apart" existing timeseries, such that alert thresholds would break / need to change.

for some clarification what this means, see open-telemetry/semantic-conventions#722

it got annoying, let's call it a bug fix 😅

it will only "break apart" existing timeseries if the app was connecting to both 80 and 443 on the same target server.address, which is pretty uncommon in practice

The declarative config approach via GlobalOpenTelemetry fails because: 1. ExtendedOpenTelemetry may not be on classpath (old API tests) 2. GlobalOpenTelemetry returns noop during static init Restore direct system property and env var reading.

Fix connectionErrorNonRoutableAddress test failures by adding default port resolution via HttpConstants.defaultPortForScheme() to getters that were missed in the initial pass: - async-http-client 1.8 and 1.9 - armeria 1.3 - reactor-netty 1.0

laurit · 2026-03-10T07:37:03Z

  public Integer getServerPort(HttpMethod request) {
    HostConfiguration hostConfiguration = request.getHostConfiguration();
-    return hostConfiguration != null ? hostConfiguration.getPort() : null;
+    if (hostConfiguration == null || hostConfiguration.getProtocol() == null) {


why is the hostConfiguration.getProtocol() value relevant here?

laurit · 2026-03-10T07:49:48Z

+    }
+    return null;
+  }
+


wondering whether something like

@Nullable public static Integer portOrDefaultFromScheme(@Nullable Integer port, Supplier<String> scheme) { if (port != null && port > 0) { return port; } return defaultPortForScheme(scheme.get()); }

would help

added, looks nice, does introduce a capturing lambda, but really should get jitted away

The lambda is only useful when getting the scheme does something complicated that is worth skipping when the port is already known. I'd guess that in most (if not all) cases we could pass the scheme directly. If you don't like the capturing lambda you could add an overload with plain string.

laurit

I think we should add the preview flag and use it in the PRs tagged for 3.0.0 so we could merge them immediately.

… async-http-client-2.0, and aws-sdk-1.11

…rg.asynchttpclient.uri.Uri instead of java.net.URI)

…r simplify refactor

…java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesGetter.java

laurit · 2026-03-12T09:41:37Z

+  }
+
+  @Nullable
+  public static Integer portOrDefaultFromScheme(int port, Supplier<String> scheme) {


not sure having this overload is worth it. Or could make the other one call this one.

*Issue description:* *Description of changes:* ADOT Java is upgrading its OpenTelemetry Java Instrumentation dependency to 2.26.1, which includes a change that adds the `server.port` attribute to all HTTP client spans ([upstream PR](open-telemetry/opentelemetry-java-instrumentation#16388)). the ADOT logic appends server.port to RemoteService if it exists, so this PR updates the java E2E tests to match the expected behavior. *Rollback procedure:* <Can we safely revert this commit if needed? If not, detail what must be done to safely revert and why it is needed.> *Ensure you've run the following tests on your changes and include the link below:* To do so, create a `test.yml` file with `name: Test` and workflow description to test your changes, then remove the file for your PR. Link your test run in your PR description. This process is a short term solution while we work on creating a staging environment for testing. NOTE: TESTS RUNNING ON A SINGLE EKS CLUSTER CANNOT BE RUN IN PARALLEL. See the [needs](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds) keyword to run tests in succession. - Run Java EKS on `e2e-playground` in us-east-1 and eu-central-2 - Run Python EKS on `e2e-playground` in us-east-1 and eu-central-2 - Run metric limiter on EKS cluster `e2e-playground` in us-east-1 and eu-central-2 - Run EC2 tests in all regions - Run K8s on a separate K8s cluster (check IAD test account for master node endpoints; these will change as we create and destroy clusters for OS patching) By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: ADOT Patch workflow <[email protected]>

breedx-splk reviewed Mar 4, 2026

View reviewed changes

breedx-splk approved these changes Mar 4, 2026

View reviewed changes

laurit reviewed Mar 5, 2026

View reviewed changes

Comment thread ...rc/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java

laurit reviewed Mar 9, 2026

View reviewed changes

trask mentioned this pull request Mar 9, 2026

Support db.system.name in vertx sql instrumentation #16254

Merged

trask added 5 commits March 9, 2026 14:19

server.port is required on HTTP client spans

6c5e385

Fix S3ClientTest to expect server.port=443 for https URLs

ba3d06b

Fix Aws0ClientTest to expect server.port=443 for https URLs

36d1cdb

hide behind v3 preview

8ee298a

trask force-pushed the http-client-default-port branch from 0414767 to c48bc26 Compare March 9, 2026 21:53

trask added 3 commits March 9, 2026 16:29

update

65a8a16

fix

84122ba

move to getter

4f6dee1

github-actions Bot added the test native This label can be applied to PRs to trigger them to run native tests label Mar 10, 2026

remove v3 preview flag

2661572

trask mentioned this pull request Mar 10, 2026

Better fallbacks for apache http client server.address and server.port when uri is invalid #16426

Closed

laurit reviewed Mar 10, 2026

View reviewed changes

laurit approved these changes Mar 10, 2026

View reviewed changes

trask added 4 commits March 10, 2026 08:13

feedback

1a9b74e

simplify getServerPort callsites

961f10f

optimization

9d046f9

remove unused

14f4976

trask mentioned this pull request Mar 10, 2026

Automated code review for apache-httpclient-4.0:javaagent #16368

Closed

trask added 3 commits March 10, 2026 14:55

some instrumentations don't need fallback

b6646e9

fix: restore portOrDefaultFromScheme fallback for akka-http, play-ws,…

69320c3

… async-http-client-2.0, and aws-sdk-1.11

simplify

b35271a

trask added 2 commits March 11, 2026 10:42

fix: correct type error in async-http-client-2.0 getServerPort (use o…

be90143

…rg.asynchttpclient.uri.Uri instead of java.net.URI)

fix: correct type errors in play-ws and okhttp-2.2 getServerPort afte…

e0c9dba

…r simplify refactor

trask commented Mar 11, 2026

View reviewed changes

Comment thread ...vaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesGetter.java

trask added 3 commits March 11, 2026 12:58

Update instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/…

dfc31ca

…java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesGetter.java

updates

10b1724

Better fallbacks for Apache HttpClient server/port

387f8de

trask force-pushed the http-client-default-port branch 2 times, most recently from b99206d to f775c81 Compare March 12, 2026 02:45

better fallback

299f166

trask force-pushed the http-client-default-port branch from f775c81 to 299f166 Compare March 12, 2026 02:47

another

5d6fb60

trask marked this pull request as ready for review March 12, 2026 04:10

trask requested a review from a team as a code owner March 12, 2026 04:10

laurit reviewed Mar 12, 2026

View reviewed changes

trask added 2 commits March 12, 2026 10:21

simplify

5aa3156

last

a83a575

trask enabled auto-merge (squash) March 12, 2026 20:34

Fix OkHttp 2.2: use java.net.URL instead of HttpUrl

ec716e9

trask merged commit 257ebe6 into open-telemetry:main Mar 13, 2026
94 of 95 checks passed

trask deleted the http-client-default-port branch March 13, 2026 02:23

breedx-splk mentioned this pull request Mar 13, 2026

Fix khttp client instrumentation signalfx/splunk-otel-java#2705

Merged

ezhang6811 mentioned this pull request Mar 27, 2026

Update remote service for Java HTTP calls aws-observability/aws-application-signals-test-framework#605

Merged

	if (port == null \|\| port <= 0) {
	if (port == null \|\| port <= 0 \|\| port > 65535) {


		String fullUrl = stripSensitiveData(getter.getUrlFull(request));

		if (SemconvStability.v3Preview()) {

Conversation

trask commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breedx-splk left a comment

Choose a reason for hiding this comment

Uh oh!

trask commented Mar 4, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trask Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trask Mar 10, 2026 •

edited

Loading