Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change client.port from recommended to opt-in on HTTP server spans #9786

Merged
merged 1 commit into from
Nov 1, 2023
Merged

Change client.port from recommended to opt-in on HTTP server spans #9786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions ...ntelemetry/instrumentation/api/instrumenter/http/ForwardedForAddressAndPortExtractor.java
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,9 @@ InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
netAttributesGetter,
serverAddressPortExtractor,
clientAddressPortExtractor,
// network.type and network.transport are opt-in
/* captureNetworkTransportAndType= */ false,
// network.local.* are opt-in
/* captureLocalSocketAttributes= */ false,
/* captureOldPeerDomainAttribute= */ false,
SemconvStability.emitStableHttpSemconv(),
Expand All @@ -169,6 +171,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,9 +520,7 @@ void shouldNotExtractDuplicatePeerAddress() {
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,11 +788,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
Loading