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.

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 network.protocol.name from opt-in to conditionally required #9797

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading