Skip to content

Commit

Permalink
Change network.protocol.name from opt-in to conditionally required (o…
Browse files Browse the repository at this point in the history
  • Loading branch information
Mateusz Rzeszutek authored and Abhishekkr3003 committed Nov 7, 2023
1 parent 2fcddfe commit 8575620
Show file tree
Hide file tree
Showing 18 changed files with 107 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST,
HttpClientAttributesExtractor(HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> builder) {
super(
builder.httpAttributesGetter,
builder.netAttributesGetter,
HttpStatusCodeConverter.CLIENT,
builder.capturedRequestHeaders,
builder.capturedResponseHeaders,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
return new InternalNetworkAttributesExtractor<>(
netAttributesGetter,
serverAddressAndPortExtractor,
/* captureNetworkTransportAndType= */ false,
// network.{transport,type} are opt-in, network.protocol.* have HTTP-specific logic
/* captureProtocolAttributes= */ false,
/* captureLocalSocketAttributes= */ false,
/* captureOldPeerDomainAttribute= */ true,
SemconvStability.emitStableHttpSemconv(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.internal.HttpAttributes;
import io.opentelemetry.instrumentation.api.instrumenter.network.NetworkAttributesGetter;
import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import io.opentelemetry.semconv.SemanticAttributes;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import javax.annotation.Nullable;

Expand All @@ -34,18 +36,21 @@ abstract class HttpCommonAttributesExtractor<
implements AttributesExtractor<REQUEST, RESPONSE> {

final GETTER getter;
final NetworkAttributesGetter<REQUEST, RESPONSE> networkGetter;
private final HttpStatusCodeConverter statusCodeConverter;
private final List<String> capturedRequestHeaders;
private final List<String> capturedResponseHeaders;
private final Set<String> knownMethods;

HttpCommonAttributesExtractor(
GETTER getter,
NetworkAttributesGetter<REQUEST, RESPONSE> networkGetter,
HttpStatusCodeConverter statusCodeConverter,
List<String> capturedRequestHeaders,
List<String> capturedResponseHeaders,
Set<String> knownMethods) {
this.getter = getter;
this.networkGetter = networkGetter;
this.statusCodeConverter = statusCodeConverter;
this.capturedRequestHeaders = lowercase(capturedRequestHeaders);
this.capturedResponseHeaders = lowercase(capturedResponseHeaders);
Expand Down Expand Up @@ -143,6 +148,19 @@ public void onEnd(
}
internalSet(attributes, HttpAttributes.ERROR_TYPE, errorType);
}

if (SemconvStability.emitStableHttpSemconv()) {
String protocolName = lowercaseStr(networkGetter.getNetworkProtocolName(request, response));
String protocolVersion =
lowercaseStr(networkGetter.getNetworkProtocolVersion(request, response));

if (protocolVersion != null) {
if (!"http".equals(protocolName)) {
internalSet(attributes, SemanticAttributes.NETWORK_PROTOCOL_NAME, protocolName);
}
internalSet(attributes, SemanticAttributes.NETWORK_PROTOCOL_VERSION, protocolVersion);
}
}
}

@Nullable
Expand Down Expand Up @@ -173,4 +191,9 @@ private static Long parseNumber(@Nullable String number) {
return null;
}
}

@Nullable
private static String lowercaseStr(@Nullable String str) {
return str == null ? null : str.toLowerCase(Locale.ROOT);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
HttpServerAttributesExtractor(HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> builder) {
super(
builder.httpAttributesGetter,
builder.netAttributesGetter,
HttpStatusCodeConverter.SERVER,
builder.capturedRequestHeaders,
builder.capturedResponseHeaders,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
return new InternalNetworkAttributesExtractor<>(
netAttributesGetter,
clientAddressPortExtractor,
// network.type and network.transport are opt-in
/* captureNetworkTransportAndType= */ false,
// network.{transport,type} are opt-in, network.protocol.* have HTTP-specific logic
/* captureProtocolAttributes= */ false,
// network.local.* are opt-in
/* captureLocalSocketAttributes= */ false,
/* captureOldPeerDomainAttribute= */ false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ private NetClientAttributesExtractor(NetClientAttributesGetter<REQUEST, RESPONSE
new InternalNetworkAttributesExtractor<>(
getter,
serverAddressAndPortExtractor,
/* captureNetworkTransportAndType= */ true,
/* captureProtocolAttributes= */ true,
/* captureLocalSocketAttributes= */ false,
/* captureOldPeerDomainAttribute= */ true,
SemconvStability.emitStableHttpSemconv(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST, RESPONSE
new InternalNetworkAttributesExtractor<>(
getter,
clientAddressAndPortExtractor,
/* captureNetworkTransportAndType= */ true,
/* captureProtocolAttributes= */ true,
/* captureLocalSocketAttributes= */ true,
/* captureOldPeerDomainAttribute= */ false,
SemconvStability.emitStableHttpSemconv(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public static <REQUEST, RESPONSE> NetworkAttributesExtractor<REQUEST, RESPONSE>
new InternalNetworkAttributesExtractor<>(
getter,
AddressAndPortExtractor.noop(),
/* captureNetworkTransportAndType= */ true,
/* captureProtocolAttributes= */ true,
/* captureLocalSocketAttributes= */ true,
// capture the old net.sock.peer.name attr for backwards compatibility
/* captureOldPeerDomainAttribute= */ true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public final class InternalNetworkAttributesExtractor<REQUEST, RESPONSE> {

private final NetworkAttributesGetter<REQUEST, RESPONSE> getter;
private final AddressAndPortExtractor<REQUEST> logicalPeerAddressAndPortExtractor;
private final boolean captureNetworkTransportAndType;
private final boolean captureProtocolAttributes;
private final boolean captureLocalSocketAttributes;
private final boolean captureOldPeerDomainAttribute;
private final boolean emitStableUrlAttributes;
Expand All @@ -32,14 +32,14 @@ public final class InternalNetworkAttributesExtractor<REQUEST, RESPONSE> {
public InternalNetworkAttributesExtractor(
NetworkAttributesGetter<REQUEST, RESPONSE> getter,
AddressAndPortExtractor<REQUEST> logicalPeerAddressAndPortExtractor,
boolean captureNetworkTransportAndType,
boolean captureProtocolAttributes,
boolean captureLocalSocketAttributes,
boolean captureOldPeerDomainAttribute,
boolean emitStableUrlAttributes,
boolean emitOldHttpAttributes) {
this.getter = getter;
this.logicalPeerAddressAndPortExtractor = logicalPeerAddressAndPortExtractor;
this.captureNetworkTransportAndType = captureNetworkTransportAndType;
this.captureProtocolAttributes = captureProtocolAttributes;
this.captureLocalSocketAttributes = captureLocalSocketAttributes;
this.captureOldPeerDomainAttribute = captureOldPeerDomainAttribute;
this.emitStableUrlAttributes = emitStableUrlAttributes;
Expand All @@ -51,15 +51,13 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
String protocolName = lowercase(getter.getNetworkProtocolName(request, response));
String protocolVersion = lowercase(getter.getNetworkProtocolVersion(request, response));

if (emitStableUrlAttributes) {
if (emitStableUrlAttributes && captureProtocolAttributes) {
String transport = lowercase(getter.getNetworkTransport(request, response));
if (captureNetworkTransportAndType) {
internalSet(attributes, SemanticAttributes.NETWORK_TRANSPORT, transport);
internalSet(
attributes,
SemanticAttributes.NETWORK_TYPE,
lowercase(getter.getNetworkType(request, response)));
}
internalSet(attributes, SemanticAttributes.NETWORK_TRANSPORT, transport);
internalSet(
attributes,
SemanticAttributes.NETWORK_TYPE,
lowercase(getter.getNetworkType(request, response)));
internalSet(attributes, SemanticAttributes.NETWORK_PROTOCOL_NAME, protocolName);
internalSet(attributes, SemanticAttributes.NETWORK_PROTOCOL_VERSION, protocolVersion);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ void normal() {
asList("654", "321")),
entry(SemanticAttributes.NET_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NET_PROTOCOL_VERSION, "1.1"),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "1.1"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ void normal() {
.containsOnly(
entry(SemanticAttributes.NET_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NET_PROTOCOL_VERSION, "2.0"),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "2.0"),
entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{repoId}"),
entry(SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH, 10L),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ void normal() {
entry(
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "1.1"),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "4.3.2.1"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
Expand Down Expand Up @@ -397,4 +396,29 @@ void shouldExtractPeerAddressEvenIfItDuplicatesServerAddress() {
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
}

@Test
void shouldExtractProtocolNameDifferentFromHttp() {
Map<String, String> request = new HashMap<>();
request.put("networkProtocolName", "spdy");
request.put("networkProtocolVersion", "3.1");

Map<String, String> response = new HashMap<>();
response.put("statusCode", "200");

AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpClientAttributesExtractor.create(new TestHttpClientAttributesGetter());

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build()).isEmpty();

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(
entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "spdy"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "3.1"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ void normal() {
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "2.0"),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "4.3.2.1"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L),
Expand Down Expand Up @@ -528,4 +527,29 @@ void shouldExtractPeerAddressEvenIfItDuplicatesClientAddress() {
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
}

@Test
void shouldExtractProtocolNameDifferentFromHttp() {
Map<String, String> request = new HashMap<>();
request.put("networkProtocolName", "spdy");
request.put("networkProtocolVersion", "3.1");

Map<String, String> response = new HashMap<>();
response.put("statusCode", "200");

AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpServerAttributesExtractor.create(new TestHttpServerAttributesGetter());

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build()).isEmpty();

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(
entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "spdy"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "3.1"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ public void traceRequest(boolean useCache) throws IOException {
List<AttributeAssertion> attributes =
new ArrayList<>(
Arrays.asList(
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_NAME), "http"),
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_VERSION), "1.1"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_NAME), "localhost"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_PORT), url.getPort()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_URL), url.toString()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_METHOD), "GET"),
equalTo(getAttributeKey(SemanticAttributes.HTTP_STATUS_CODE), STATUS)));
if (SemconvStability.emitOldHttpSemconv()) {
attributes.add(equalTo(SemanticAttributes.NET_PROTOCOL_NAME, "http"));
attributes.add(
satisfies(
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH, AbstractLongAssert::isNotNegative));
Expand Down Expand Up @@ -176,14 +176,14 @@ public void testBrokenApiUsage() throws IOException {
List<AttributeAssertion> attributes =
new ArrayList<>(
Arrays.asList(
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_NAME), "http"),
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_VERSION), "1.1"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_NAME), "localhost"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_PORT), url.getPort()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_URL), url.toString()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_METHOD), "GET"),
equalTo(getAttributeKey(SemanticAttributes.HTTP_STATUS_CODE), STATUS)));
if (SemconvStability.emitOldHttpSemconv()) {
attributes.add(equalTo(SemanticAttributes.NET_PROTOCOL_NAME, "http"));
attributes.add(
satisfies(
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH, AbstractLongAssert::isNotNegative));
Expand Down Expand Up @@ -234,14 +234,14 @@ public void testPostRequest() throws IOException {
List<AttributeAssertion> attributes =
new ArrayList<>(
Arrays.asList(
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_NAME), "http"),
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_VERSION), "1.1"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_NAME), "localhost"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_PORT), url.getPort()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_URL), url.toString()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_METHOD), "POST"),
equalTo(getAttributeKey(SemanticAttributes.HTTP_STATUS_CODE), STATUS)));
if (SemconvStability.emitOldHttpSemconv()) {
attributes.add(equalTo(SemanticAttributes.NET_PROTOCOL_NAME, "http"));
attributes.add(
satisfies(
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH, AbstractLongAssert::isNotNegative));
Expand Down Expand Up @@ -297,14 +297,14 @@ public void getOutputStreamShouldTransformGetIntoPost() throws IOException {
List<AttributeAssertion> attributes =
new ArrayList<>(
Arrays.asList(
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_NAME), "http"),
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_VERSION), "1.1"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_NAME), "localhost"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_PORT), url.getPort()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_URL), url.toString()),
equalTo(getAttributeKey(SemanticAttributes.HTTP_METHOD), "POST"),
equalTo(getAttributeKey(SemanticAttributes.HTTP_STATUS_CODE), STATUS)));
if (SemconvStability.emitOldHttpSemconv()) {
attributes.add(equalTo(SemanticAttributes.NET_PROTOCOL_NAME, "http"));
attributes.add(
satisfies(
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH, AbstractLongAssert::isNotNegative));
Expand Down Expand Up @@ -349,12 +349,14 @@ public void traceRequestWithConnectionFailure(String scheme) {
List<AttributeAssertion> attributes =
new ArrayList<>(
Arrays.asList(
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_NAME), "http"),
equalTo(getAttributeKey(SemanticAttributes.NET_PROTOCOL_VERSION), "1.1"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_NAME), "localhost"),
equalTo(getAttributeKey(SemanticAttributes.NET_PEER_PORT), PortUtils.UNUSABLE_PORT),
equalTo(getAttributeKey(SemanticAttributes.HTTP_URL), uri),
equalTo(getAttributeKey(SemanticAttributes.HTTP_METHOD), "GET")));
if (SemconvStability.emitOldHttpSemconv()) {
attributes.add(equalTo(SemanticAttributes.NET_PROTOCOL_NAME, "http"));
}
if (SemconvStability.emitStableHttpSemconv()) {
attributes.add(equalTo(HttpAttributes.ERROR_TYPE, "java.net.ConnectException"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ class UndertowServerTest extends HttpServerTest<Undertow> implements AgentTestTr
"$SemanticAttributes.HTTP_REQUEST_METHOD" "GET"
"$SemanticAttributes.HTTP_RESPONSE_STATUS_CODE" 200
"$SemanticAttributes.USER_AGENT_ORIGINAL" TEST_USER_AGENT
"$SemanticAttributes.NETWORK_PROTOCOL_NAME" "http"
"$SemanticAttributes.NETWORK_PROTOCOL_VERSION" "1.1"
"$SemanticAttributes.SERVER_ADDRESS" uri.host
"$SemanticAttributes.SERVER_PORT" uri.port
Expand Down Expand Up @@ -242,7 +241,6 @@ class UndertowServerTest extends HttpServerTest<Undertow> implements AgentTestTr
"$SemanticAttributes.HTTP_REQUEST_METHOD" "GET"
"$SemanticAttributes.HTTP_RESPONSE_STATUS_CODE" 200
"$SemanticAttributes.USER_AGENT_ORIGINAL" TEST_USER_AGENT
"$SemanticAttributes.NETWORK_PROTOCOL_NAME" "http"
"$SemanticAttributes.NETWORK_PROTOCOL_VERSION" "1.1"
"$SemanticAttributes.SERVER_ADDRESS" uri.host
"$SemanticAttributes.SERVER_PORT" uri.port
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1024,9 +1024,13 @@ SpanDataAssert assertClientSpan(
.doesNotContainKey(SemanticAttributes.NETWORK_TRANSPORT)
.doesNotContainKey(SemanticAttributes.NETWORK_TYPE);
}

AttributeKey<String> netProtocolKey =
getAttributeKey(SemanticAttributes.NET_PROTOCOL_NAME);
if (httpClientAttributes.contains(netProtocolKey)) {
if (SemconvStability.emitStableHttpSemconv()) {
// only protocol names different from "http" are emitted
assertThat(attrs).doesNotContainKey(netProtocolKey);
} else if (attrs.get(netProtocolKey) != null) {
assertThat(attrs).containsEntry(netProtocolKey, "http");
}
AttributeKey<String> netProtocolVersionKey =
Expand Down
Loading

0 comments on commit 8575620

Please sign in to comment.