Skip to content

Commit

Permalink
Remove conditional requirement on network.peer.address and `network…
Browse files Browse the repository at this point in the history 
….peer.port` (#9775)
  • Loading branch information
Mateusz Rzeszutek authored Oct 31, 2023
1 parent 05ae636 commit b453252
Show file tree
Hide file tree
Showing 12 changed files with 26 additions and 89 deletions.
1 change: 0 additions & 1 deletion ...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ InternalNetClientAttributesExtractor<REQUEST, RESPONSE> buildNetExtractor() {
InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
return new InternalNetworkAttributesExtractor<>(
netAttributesGetter,
AddressAndPortExtractor.noop(),
serverAddressAndPortExtractor,
/* captureNetworkTransportAndType= */ false,
/* captureLocalSocketAttributes= */ false,
Expand Down
1 change: 0 additions & 1 deletion ...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ InternalNetServerAttributesExtractor<REQUEST, RESPONSE> buildNetExtractor() {
InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
return new InternalNetworkAttributesExtractor<>(
netAttributesGetter,
serverAddressPortExtractor,
clientAddressPortExtractor,
/* captureNetworkTransportAndType= */ false,
/* captureLocalSocketAttributes= */ false,
Expand Down
1 change: 0 additions & 1 deletion ...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ private NetClientAttributesExtractor(NetClientAttributesGetter<REQUEST, RESPONSE
internalNetworkExtractor =
new InternalNetworkAttributesExtractor<>(
getter,
AddressAndPortExtractor.noop(),
serverAddressAndPortExtractor,
/* captureNetworkTransportAndType= */ true,
/* captureLocalSocketAttributes= */ false,
Expand Down
1 change: 0 additions & 1 deletion ...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST, RESPONSE
internalNetworkExtractor =
new InternalNetworkAttributesExtractor<>(
getter,
serverAddressAndPortExtractor,
clientAddressAndPortExtractor,
/* captureNetworkTransportAndType= */ true,
/* captureLocalSocketAttributes= */ true,
Expand Down
1 change: 0 additions & 1 deletion ...io/opentelemetry/instrumentation/api/instrumenter/network/NetworkAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ public static <REQUEST, RESPONSE> NetworkAttributesExtractor<REQUEST, RESPONSE>
new InternalNetworkAttributesExtractor<>(
getter,
AddressAndPortExtractor.noop(),
AddressAndPortExtractor.noop(),
/* captureNetworkTransportAndType= */ true,
/* captureLocalSocketAttributes= */ true,
// capture the old net.sock.peer.name attr for backwards compatibility
Expand Down
10 changes: 3 additions & 7 deletions ...instrumentation/api/instrumenter/network/internal/InternalNetworkAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
public final class InternalNetworkAttributesExtractor<REQUEST, RESPONSE> {

private final NetworkAttributesGetter<REQUEST, RESPONSE> getter;
private final AddressAndPortExtractor<REQUEST> logicalLocalAddressAndPortExtractor;
private final AddressAndPortExtractor<REQUEST> logicalPeerAddressAndPortExtractor;
private final boolean captureNetworkTransportAndType;
private final boolean captureLocalSocketAttributes;
Expand All @@ -32,15 +31,13 @@ public final class InternalNetworkAttributesExtractor<REQUEST, RESPONSE> {

public InternalNetworkAttributesExtractor(
NetworkAttributesGetter<REQUEST, RESPONSE> getter,
AddressAndPortExtractor<REQUEST> logicalLocalAddressAndPortExtractor,
AddressAndPortExtractor<REQUEST> logicalPeerAddressAndPortExtractor,
boolean captureNetworkTransportAndType,
boolean captureLocalSocketAttributes,
boolean captureOldPeerDomainAttribute,
boolean emitStableUrlAttributes,
boolean emitOldHttpAttributes) {
this.getter = getter;
this.logicalLocalAddressAndPortExtractor = logicalLocalAddressAndPortExtractor;
this.logicalPeerAddressAndPortExtractor = logicalPeerAddressAndPortExtractor;
this.captureNetworkTransportAndType = captureNetworkTransportAndType;
this.captureLocalSocketAttributes = captureLocalSocketAttributes;
Expand Down Expand Up @@ -74,8 +71,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
}

String localAddress = getter.getNetworkLocalAddress(request, response);
String logicalLocalAddress = logicalLocalAddressAndPortExtractor.extract(request).address;
if (localAddress != null && !localAddress.equals(logicalLocalAddress)) {
if (localAddress != null) {
if (emitStableUrlAttributes && captureLocalSocketAttributes) {
internalSet(attributes, NetworkAttributes.NETWORK_LOCAL_ADDRESS, localAddress);
}
Expand All @@ -95,8 +91,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
}

String peerAddress = getter.getNetworkPeerAddress(request, response);
String logicalPeerAddress = logicalPeerAddressAndPortExtractor.extract(request).address;
if (peerAddress != null && !peerAddress.equals(logicalPeerAddress)) {
if (peerAddress != null) {
if (emitStableUrlAttributes) {
internalSet(attributes, NetworkAttributes.NETWORK_PEER_ADDRESS, peerAddress);
}
Expand All @@ -119,6 +114,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
getter.getNetworkPeerInetSocketAddress(request, response);
if (peerSocketAddress != null) {
String peerSocketDomain = InetSocketAddressUtil.getDomainName(peerSocketAddress);
String logicalPeerAddress = logicalPeerAddressAndPortExtractor.extract(request).address;
if (peerSocketDomain != null && !peerSocketDomain.equals(logicalPeerAddress)) {
internalSet(attributes, SemanticAttributes.NET_SOCK_PEER_NAME, peerSocketDomain);
}
Expand Down
30 changes: 0 additions & 30 deletions .../opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,36 +147,6 @@ void empty() {
assertThat(endAttributes.build()).isEmpty();
}

@Test
void doesNotSetDuplicateSocketAddress() {
// given
Map<String, String> map = new HashMap<>();
map.put("netTransport", IP_TCP);
map.put("peerName", "1:2:3:4::");
map.put("peerPort", "42");
map.put("sockFamily", "inet6");
map.put("sockPeerAddr", "1:2:3:4::");
map.put("sockPeerName", "proxy.opentelemetry.io");
map.put("sockPeerPort", "123");

Context context = Context.root();

// when
AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, context, map);

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, context, map, map, null);

// then
assertThat(startAttributes.build())
.containsOnly(
entry(SemanticAttributes.NET_PEER_NAME, "1:2:3:4::"),
entry(SemanticAttributes.NET_PEER_PORT, 42L));

assertThat(endAttributes.build()).containsOnly(entry(SemanticAttributes.NET_TRANSPORT, IP_TCP));
}

@Test
void doesNotSetNegativePortValues() {
// given
Expand Down
30 changes: 0 additions & 30 deletions .../opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,36 +163,6 @@ void empty() {
assertThat(endAttributes.build()).isEmpty();
}

@Test
void doesNotSetDuplicatesSocketAddress() {
// given
Map<String, String> map = new HashMap<>();
map.put("netTransport", IP_TCP);
map.put("hostName", "4:3:2:1::");
map.put("hostPort", "80");
map.put("sockFamily", "inet6");
map.put("sockHostAddr", "4:3:2:1::");
map.put("sockHostPort", "8080");

Context context = Context.root();

// when
AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, context, map);

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, context, map, null, null);

// then
assertThat(startAttributes.build())
.containsOnly(
entry(SemanticAttributes.NET_TRANSPORT, IP_TCP),
entry(SemanticAttributes.NET_HOST_NAME, "4:3:2:1::"),
entry(SemanticAttributes.NET_HOST_PORT, 80L));

assertThat(endAttributes.build()).isEmpty();
}

@Test
void doesNotSetNegativePort() {
// given
Expand Down
7 changes: 5 additions & 2 deletions ...instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ void shouldExtractServerAddressAndPortFromHostHeader() {
}

@Test
void shouldNotExtractDuplicatePeerAddress() {
void shouldExtractPeerAddressEvenIfItDuplicatesServerAddress() {
Map<String, String> request = new HashMap<>();
request.put("networkPeerAddress", "1.2.3.4");
request.put("networkPeerPort", "456");
Expand All @@ -394,6 +394,9 @@ void shouldNotExtractDuplicatePeerAddress() {
AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L));
.containsOnly(
entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
}
}
7 changes: 5 additions & 2 deletions ...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ void shouldExtractServerAddressAndPortFromHostHeader() {
}

@Test
void shouldNotExtractDuplicatePeerAddress() {
void shouldExtractPeerAddressEvenIfItDuplicatesClientAddress() {
Map<String, String> request = new HashMap<>();
request.put("networkPeerAddress", "1.2.3.4");
request.put("networkPeerPort", "456");
Expand All @@ -527,6 +527,9 @@ void shouldNotExtractDuplicatePeerAddress() {
AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L));
.containsOnly(
entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
}
}
8 changes: 6 additions & 2 deletions ...telemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,17 +153,21 @@ void twoCamelServiceSpans() throws Exception {
equalTo(
SemanticAttributes.USER_AGENT_ORIGINAL,
"Jakarta Commons-HttpClient/3.1"),
satisfies(
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
val -> val.isInstanceOf(Long.class)),
equalTo(SemanticAttributes.HTTP_ROUTE, "/serviceTwo"),
equalTo(SemanticAttributes.NET_PROTOCOL_NAME, "http"),
equalTo(SemanticAttributes.NET_PROTOCOL_VERSION, "1.1"),
equalTo(SemanticAttributes.NET_HOST_NAME, "127.0.0.1"),
equalTo(SemanticAttributes.NET_HOST_PORT, portTwo),
equalTo(SemanticAttributes.NET_SOCK_PEER_ADDR, "127.0.0.1"),
satisfies(
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
SemanticAttributes.NET_SOCK_PEER_PORT,
val -> val.isInstanceOf(Long.class)),
equalTo(SemanticAttributes.NET_SOCK_HOST_ADDR, "127.0.0.1"),
satisfies(
SemanticAttributes.NET_SOCK_PEER_PORT,
SemanticAttributes.NET_SOCK_HOST_PORT,
val -> val.isInstanceOf(Long.class))),
span ->
span.hasName("POST /serviceTwo")
Expand Down
18 changes: 7 additions & 11 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 @@ -757,6 +757,13 @@ protected SpanDataAssert assertServerSpan(
if (attrs.get(netHostPortKey) != null) {
assertThat(attrs).containsEntry(netHostPortKey, port);
}

AttributeKey<String> netSockPeerAddrKey =
getAttributeKey(SemanticAttributes.NET_SOCK_PEER_ADDR);
if (attrs.get(netSockPeerAddrKey) != null) {
assertThat(attrs)
.containsEntry(netSockPeerAddrKey, options.sockPeerAddr.apply(endpoint));
}
AttributeKey<Long> netSockPeerPortKey =
getAttributeKey(SemanticAttributes.NET_SOCK_PEER_PORT);
if (attrs.get(netSockPeerPortKey) != null) {
Expand All @@ -768,17 +775,6 @@ protected SpanDataAssert assertServerSpan(
.isInstanceOf(Long.class)
.isNotEqualTo(Long.valueOf(port)));
}
AttributeKey<String> netSockPeerAddrKey =
getAttributeKey(SemanticAttributes.NET_SOCK_PEER_ADDR);
if (attrs.get(netSockPeerAddrKey) != null) {
assertThat(attrs)
.containsEntry(netSockPeerAddrKey, options.sockPeerAddr.apply(endpoint));
}
AttributeKey<String> netSockHostAddrKey =
getAttributeKey(SemanticAttributes.NET_SOCK_PEER_ADDR);
if (attrs.get(netSockHostAddrKey) != null) {
assertThat(attrs).containsEntry(netSockHostAddrKey, "127.0.0.1");
}

assertThat(attrs)
.hasEntrySatisfying(
Expand Down

0 comments on commit b453252

Please sign in to comment.