Skip to content

Commit

Permalink
Change client.port from recommended to opt-in on HTTP server spans (#…
Browse files Browse the repository at this point in the history 
…9786)
  • Loading branch information
Mateusz Rzeszutek authored Nov 1, 2023
1 parent b453252 commit 182ee7e
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 93 deletions.
22 changes: 0 additions & 22 deletions ...ntelemetry/instrumentation/api/instrumenter/http/ForwardedForAddressAndPortExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import static io.opentelemetry.instrumentation.api.instrumenter.http.HeaderParsingHelper.notFound;
import static io.opentelemetry.instrumentation.api.instrumenter.http.HeaderParsingHelper.setPort;

import io.opentelemetry.instrumentation.api.instrumenter.network.internal.AddressAndPortExtractor;
import java.util.Locale;
Expand Down Expand Up @@ -92,12 +91,6 @@ private static boolean extractClientInfo(
}
sink.setAddress(forwarded.substring(start + 1, ipv6End));

int portStart = ipv6End + 1;
if (portStart < end && forwarded.charAt(portStart) == ':') {
int portEnd = findPortEnd(forwarded, portStart + 1, end);
setPort(sink, forwarded, portStart + 1, portEnd);
}

return true;
}

Expand All @@ -120,13 +113,6 @@ private static boolean extractClientInfo(
}

sink.setAddress(forwarded.substring(start, i));

if (isIpv4PortSeparator) {
int portStart = i;
int portEnd = findPortEnd(forwarded, portStart + 1, end);
setPort(sink, forwarded, portStart + 1, portEnd);
}

return true;
}
}
Expand All @@ -135,12 +121,4 @@ private static boolean extractClientInfo(
sink.setAddress(forwarded.substring(start, end));
return true;
}

private static int findPortEnd(String header, int start, int end) {
int numberEnd = start;
while (numberEnd < end && Character.isDigit(header.charAt(numberEnd))) {
++numberEnd;
}
return numberEnd;
}
}
4 changes: 4 additions & 0 deletions ...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
return new InternalNetworkAttributesExtractor<>(
netAttributesGetter,
clientAddressPortExtractor,
// network.type and network.transport are opt-in
/* captureNetworkTransportAndType= */ false,
// network.local.* are opt-in
/* captureLocalSocketAttributes= */ false,
/* captureOldPeerDomainAttribute= */ false,
SemconvStability.emitStableHttpSemconv(),
Expand All @@ -168,6 +170,8 @@ InternalServerAttributesExtractor<REQUEST> buildServerExtractor() {
InternalClientAttributesExtractor<REQUEST> buildClientExtractor() {
return new InternalClientAttributesExtractor<>(
clientAddressPortExtractor,
// client.port is opt-in
/* capturePort= */ false,
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv());
}
Expand Down
1 change: 1 addition & 0 deletions ...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST, RESPONSE
internalClientExtractor =
new InternalClientAttributesExtractor<>(
clientAddressAndPortExtractor,
/* capturePort= */ true,
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv());
}
Expand Down
1 change: 1 addition & 0 deletions .../io/opentelemetry/instrumentation/api/instrumenter/network/ClientAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public static <REQUEST, RESPONSE> ClientAttributesExtractor<REQUEST, RESPONSE> c
internalExtractor =
new InternalClientAttributesExtractor<>(
new ClientAddressAndPortExtractor<>(getter, AddressAndPortExtractor.noop()),
/* capturePort= */ true,
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv());
}
Expand Down
5 changes: 4 additions & 1 deletion .../instrumentation/api/instrumenter/network/internal/InternalClientAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@
public final class InternalClientAttributesExtractor<REQUEST> {

private final AddressAndPortExtractor<REQUEST> addressAndPortExtractor;
private final boolean capturePort;
private final boolean emitStableUrlAttributes;
private final boolean emitOldHttpAttributes;

public InternalClientAttributesExtractor(
AddressAndPortExtractor<REQUEST> addressAndPortExtractor,
boolean capturePort,
boolean emitStableUrlAttributes,
boolean emitOldHttpAttributes) {
this.addressAndPortExtractor = addressAndPortExtractor;
this.capturePort = capturePort;
this.emitStableUrlAttributes = emitStableUrlAttributes;
this.emitOldHttpAttributes = emitOldHttpAttributes;
}
Expand All @@ -36,7 +39,7 @@ public void onStart(AttributesBuilder attributes, REQUEST request) {
if (clientAddressAndPort.address != null) {
if (emitStableUrlAttributes) {
internalSet(attributes, SemanticAttributes.CLIENT_ADDRESS, clientAddressAndPort.address);
if (clientAddressAndPort.port != null && clientAddressAndPort.port > 0) {
if (capturePort && clientAddressAndPort.port != null && clientAddressAndPort.port > 0) {
internalSet(attributes, SemanticAttributes.CLIENT_PORT, (long) clientAddressAndPort.port);
}
}
Expand Down
122 changes: 60 additions & 62 deletions ...emetry/instrumentation/api/instrumenter/http/ForwardedForAddressAndPortExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,14 @@ class ForwardedForAddressAndPortExtractorTest {

@ParameterizedTest
@ArgumentsSource(ForwardedArgs.class)
void shouldParseForwarded(
List<String> headers, @Nullable String expectedAddress, @Nullable Integer expectedPort) {
void shouldParseForwarded(List<String> headers, @Nullable String expectedAddress) {
doReturn(headers).when(getter).getHttpRequestHeader("request", "forwarded");

AddressAndPort sink = new AddressAndPort();
underTest.extract(sink, "request");

assertThat(sink.getAddress()).isEqualTo(expectedAddress);
assertThat(sink.getPort()).isEqualTo(expectedPort);
assertThat(sink.getPort()).isNull();
}

static final class ForwardedArgs implements ArgumentsProvider {
Expand All @@ -52,56 +51,55 @@ static final class ForwardedArgs implements ArgumentsProvider {
public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
return Stream.of(
// empty/invalid headers
arguments(singletonList(""), null, null),
arguments(singletonList("for="), null, null),
arguments(singletonList("for=;"), null, null),
arguments(singletonList("for=\""), null, null),
arguments(singletonList("for=\"\""), null, null),
arguments(singletonList("for=\"1.2.3.4"), null, null),
arguments(singletonList("for=\"[::1]"), null, null),
arguments(singletonList("for=[::1"), null, null),
arguments(singletonList("for=\"[::1\""), null, null),
arguments(singletonList("for=\"[::1\"]"), null, null),
arguments(singletonList("by=1.2.3.4, test=abc"), null, null),
arguments(singletonList(""), null),
arguments(singletonList("for="), null),
arguments(singletonList("for=;"), null),
arguments(singletonList("for=\""), null),
arguments(singletonList("for=\"\""), null),
arguments(singletonList("for=\"1.2.3.4"), null),
arguments(singletonList("for=\"[::1]"), null),
arguments(singletonList("for=[::1"), null),
arguments(singletonList("for=\"[::1\""), null),
arguments(singletonList("for=\"[::1\"]"), null),
arguments(singletonList("by=1.2.3.4, test=abc"), null),

// ipv6
arguments(singletonList("for=[::1]"), "::1", null),
arguments(singletonList("For=[::1]"), "::1", null),
arguments(singletonList("for=\"[::1]\":42"), "::1", null),
arguments(singletonList("for=[::1]:42"), "::1", 42),
arguments(singletonList("for=\"[::1]:42\""), "::1", 42),
arguments(singletonList("for=[::1], for=1.2.3.4"), "::1", null),
arguments(singletonList("for=[::1]; for=1.2.3.4:42"), "::1", null),
arguments(singletonList("for=[::1]:42abc"), "::1", 42),
arguments(singletonList("for=[::1]:abc"), "::1", null),
arguments(singletonList("for=[::1]"), "::1"),
arguments(singletonList("For=[::1]"), "::1"),
arguments(singletonList("for=\"[::1]\":42"), "::1"),
arguments(singletonList("for=[::1]:42"), "::1"),
arguments(singletonList("for=\"[::1]:42\""), "::1"),
arguments(singletonList("for=[::1], for=1.2.3.4"), "::1"),
arguments(singletonList("for=[::1]; for=1.2.3.4:42"), "::1"),
arguments(singletonList("for=[::1]:42abc"), "::1"),
arguments(singletonList("for=[::1]:abc"), "::1"),

// ipv4
arguments(singletonList("for=1.2.3.4"), "1.2.3.4", null),
arguments(singletonList("FOR=1.2.3.4"), "1.2.3.4", null),
arguments(singletonList("for=1.2.3.4, :42"), "1.2.3.4", null),
arguments(singletonList("for=1.2.3.4;proto=https;by=4.3.2.1"), "1.2.3.4", null),
arguments(singletonList("for=1.2.3.4:42"), "1.2.3.4", 42),
arguments(singletonList("for=1.2.3.4:42abc"), "1.2.3.4", 42),
arguments(singletonList("for=1.2.3.4:abc"), "1.2.3.4", null),
arguments(singletonList("for=1.2.3.4; for=4.3.2.1:42"), "1.2.3.4", null),
arguments(singletonList("for=1.2.3.4"), "1.2.3.4"),
arguments(singletonList("FOR=1.2.3.4"), "1.2.3.4"),
arguments(singletonList("for=1.2.3.4, :42"), "1.2.3.4"),
arguments(singletonList("for=1.2.3.4;proto=https;by=4.3.2.1"), "1.2.3.4"),
arguments(singletonList("for=1.2.3.4:42"), "1.2.3.4"),
arguments(singletonList("for=1.2.3.4:42abc"), "1.2.3.4"),
arguments(singletonList("for=1.2.3.4:abc"), "1.2.3.4"),
arguments(singletonList("for=1.2.3.4; for=4.3.2.1:42"), "1.2.3.4"),

// multiple headers
arguments(asList("proto=https", "for=1.2.3.4", "for=[::1]:42"), "1.2.3.4", null));
arguments(asList("proto=https", "for=1.2.3.4", "for=[::1]:42"), "1.2.3.4"));
}
}

@ParameterizedTest
@ArgumentsSource(ForwardedForArgs.class)
void shouldParseForwardedFor(
List<String> headers, @Nullable String expectedAddress, @Nullable Integer expectedPort) {
void shouldParseForwardedFor(List<String> headers, @Nullable String expectedAddress) {
doReturn(emptyList()).when(getter).getHttpRequestHeader("request", "forwarded");
doReturn(headers).when(getter).getHttpRequestHeader("request", "x-forwarded-for");

AddressAndPort sink = new AddressAndPort();
underTest.extract(sink, "request");

assertThat(sink.getAddress()).isEqualTo(expectedAddress);
assertThat(sink.getPort()).isEqualTo(expectedPort);
assertThat(sink.getPort()).isNull();
}

static final class ForwardedForArgs implements ArgumentsProvider {
Expand All @@ -110,41 +108,41 @@ static final class ForwardedForArgs implements ArgumentsProvider {
public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
return Stream.of(
// empty/invalid headers
arguments(singletonList(""), null, null),
arguments(singletonList(";"), null, null),
arguments(singletonList("\""), null, null),
arguments(singletonList("\"\""), null, null),
arguments(singletonList("\"1.2.3.4"), null, null),
arguments(singletonList("\"[::1]"), null, null),
arguments(singletonList("[::1"), null, null),
arguments(singletonList("\"[::1\""), null, null),
arguments(singletonList("\"[::1\"]"), null, null),
arguments(singletonList(""), null),
arguments(singletonList(";"), null),
arguments(singletonList("\""), null),
arguments(singletonList("\"\""), null),
arguments(singletonList("\"1.2.3.4"), null),
arguments(singletonList("\"[::1]"), null),
arguments(singletonList("[::1"), null),
arguments(singletonList("\"[::1\""), null),
arguments(singletonList("\"[::1\"]"), null),

// ipv6
arguments(singletonList("[::1]"), "::1", null),
arguments(singletonList("\"[::1]\":42"), "::1", null),
arguments(singletonList("[::1]:42"), "::1", 42),
arguments(singletonList("\"[::1]:42\""), "::1", 42),
arguments(singletonList("[::1],1.2.3.4"), "::1", null),
arguments(singletonList("[::1];1.2.3.4:42"), "::1", null),
arguments(singletonList("[::1]:42abc"), "::1", 42),
arguments(singletonList("[::1]:abc"), "::1", null),
arguments(singletonList("[::1]"), "::1"),
arguments(singletonList("\"[::1]\":42"), "::1"),
arguments(singletonList("[::1]:42"), "::1"),
arguments(singletonList("\"[::1]:42\""), "::1"),
arguments(singletonList("[::1],1.2.3.4"), "::1"),
arguments(singletonList("[::1];1.2.3.4:42"), "::1"),
arguments(singletonList("[::1]:42abc"), "::1"),
arguments(singletonList("[::1]:abc"), "::1"),

// ipv4
arguments(singletonList("1.2.3.4"), "1.2.3.4", null),
arguments(singletonList("1.2.3.4, :42"), "1.2.3.4", null),
arguments(singletonList("1.2.3.4,4.3.2.1"), "1.2.3.4", null),
arguments(singletonList("1.2.3.4:42"), "1.2.3.4", 42),
arguments(singletonList("1.2.3.4:42abc"), "1.2.3.4", 42),
arguments(singletonList("1.2.3.4:abc"), "1.2.3.4", null),
arguments(singletonList("1.2.3.4"), "1.2.3.4"),
arguments(singletonList("1.2.3.4, :42"), "1.2.3.4"),
arguments(singletonList("1.2.3.4,4.3.2.1"), "1.2.3.4"),
arguments(singletonList("1.2.3.4:42"), "1.2.3.4"),
arguments(singletonList("1.2.3.4:42abc"), "1.2.3.4"),
arguments(singletonList("1.2.3.4:abc"), "1.2.3.4"),

// ipv6 without brackets
arguments(singletonList("::1"), "::1", null),
arguments(singletonList("::1,::2,1.2.3.4"), "::1", null),
arguments(singletonList("::1;::2;1.2.3.4"), "::1", null),
arguments(singletonList("::1"), "::1"),
arguments(singletonList("::1,::2,1.2.3.4"), "::1"),
arguments(singletonList("::1;::2;1.2.3.4"), "::1"),

// multiple headers
arguments(asList("1.2.3.4", "::1"), "1.2.3.4", null));
arguments(asList("1.2.3.4", "::1"), "1.2.3.4"));
}
}
}
4 changes: 1 addition & 3 deletions ...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,9 +520,7 @@ void shouldExtractPeerAddressEvenIfItDuplicatesClientAddress() {
AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build())
.containsOnly(
entry(SemanticAttributes.CLIENT_ADDRESS, "1.2.3.4"),
entry(SemanticAttributes.CLIENT_PORT, 123L));
.containsOnly(entry(SemanticAttributes.CLIENT_ADDRESS, "1.2.3.4"));

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
Expand Down
8 changes: 3 additions & 5 deletions ...main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,11 +784,9 @@ protected SpanDataAssert assertServerSpan(
.satisfiesAnyOf(
value -> assertThat(value).isNull(),
value -> assertThat(value).isEqualTo(TEST_CLIENT_IP)));
if (SemconvStability.emitStableHttpSemconv()
&& attrs.get(SemanticAttributes.CLIENT_PORT) != null) {
assertThat(attrs)
.hasEntrySatisfying(
SemanticAttributes.CLIENT_PORT, port -> assertThat(port).isGreaterThan(0));
if (SemconvStability.emitStableHttpSemconv()) {
// client.port is opt-in
assertThat(attrs).doesNotContainKey(SemanticAttributes.CLIENT_PORT);
}
assertThat(attrs).containsEntry(getAttributeKey(SemanticAttributes.HTTP_METHOD), method);

Expand Down

0 comments on commit 182ee7e

Please sign in to comment.