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

Deprecate NetServerAttributesExtractor #9156

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 @@ -10,7 +10,6 @@
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you removed the import and just use the lengthy package name below. You did this in several places, so it looks intentional....it's just not clear to me how/why this helps the deprecation effort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errorprone requires that when you use deprecated types.
I don't like this either, but I can kinda agree with that: using deprecated classes should look ugly, to encourage you to refactor the code that uses them

import io.opentelemetry.instrumentation.api.instrumenter.net.internal.InternalNetServerAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalNetworkAttributesExtractor;
Expand Down Expand Up @@ -52,7 +51,9 @@ public static <REQUEST, RESPONSE> AttributesExtractor<REQUEST, RESPONSE> create(
@Deprecated
public static <REQUEST, RESPONSE> AttributesExtractor<REQUEST, RESPONSE> create(
HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,
NetServerAttributesGetter<REQUEST, RESPONSE> netAttributesGetter) {
io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>
netAttributesGetter) {
return builder(httpAttributesGetter, netAttributesGetter).build();
}

Expand All @@ -76,7 +77,9 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
@Deprecated
public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> builder(
HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,
NetServerAttributesGetter<REQUEST, RESPONSE> netAttributesGetter) {
io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>
netAttributesGetter) {
return new HttpServerAttributesExtractorBuilder<>(httpAttributesGetter, netAttributesGetter);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
import io.opentelemetry.instrumentation.api.instrumenter.net.internal.InternalNetServerAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalNetworkAttributesExtractor;
Expand All @@ -28,7 +27,12 @@
public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {

final HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter;
final NetServerAttributesGetter<REQUEST, RESPONSE> netAttributesGetter;

@SuppressWarnings("deprecation") // using the net extractor for the old->stable semconv story
final io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>
netAttributesGetter;

final HttpAddressPortExtractor<REQUEST> addressPortExtractor;
List<String> capturedRequestHeaders = emptyList();
List<String> capturedResponseHeaders = emptyList();
Expand All @@ -37,7 +41,10 @@ public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {

HttpServerAttributesExtractorBuilder(
HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,
NetServerAttributesGetter<REQUEST, RESPONSE> netAttributesGetter) {
@SuppressWarnings("deprecation") // using the net extractor for the old->stable semconv story
io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>
netAttributesGetter) {
this.httpAttributesGetter = httpAttributesGetter;
this.netAttributesGetter = netAttributesGetter;
addressPortExtractor = new HttpAddressPortExtractor<>(httpAttributesGetter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
import io.opentelemetry.instrumentation.api.instrumenter.network.ClientAttributesGetter;
import io.opentelemetry.instrumentation.api.instrumenter.network.NetworkAttributesGetter;
import io.opentelemetry.instrumentation.api.instrumenter.network.ServerAttributesGetter;
Expand All @@ -19,10 +18,14 @@
* library/framework. It will be used by the {@link HttpServerAttributesExtractor} to obtain the
* various HTTP server attributes in a type-generic way.
*/
@SuppressWarnings(
"deprecation") // implementing the NetServerAttributesGetter for the old->stable semconv story;
// will be removed in 2.0
public interface HttpServerAttributesGetter<REQUEST, RESPONSE>
extends HttpCommonAttributesGetter<REQUEST, RESPONSE>,
UrlAttributesGetter<REQUEST>,
NetServerAttributesGetter<REQUEST, RESPONSE>,
io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>,
NetworkAttributesGetter<REQUEST, RESPONSE>,
ServerAttributesGetter<REQUEST, RESPONSE>,
ClientAttributesGetter<REQUEST, RESPONSE> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@
* Extractor of <a
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/span-general.md#general-network-connection-attributes">Network
* attributes</a>.
*
* @deprecated Make sure that your instrumentation uses the extractors from the {@code ...network}
* package instead. This class will be removed in the 2.0 release.
*/
@Deprecated
public final class NetServerAttributesExtractor<REQUEST, RESPONSE>
implements AttributesExtractor<REQUEST, RESPONSE> {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
* <p>Instrumentation authors will create implementations of this interface for their specific
* server library/framework. It will be used by the {@link NetServerAttributesExtractor} to obtain
* the various network attributes in a type-generic way.
*
* @deprecated Make sure that your instrumentation implements the getters from the {@code
* ...network} package instead. This class will be removed in the 2.0 release.
*/
@Deprecated
public interface NetServerAttributesGetter<REQUEST, RESPONSE>
extends NetworkAttributesGetter<REQUEST, RESPONSE>,
ServerAttributesGetter<REQUEST, RESPONSE>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.AddressAndPort;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.FallbackAddressPortExtractor;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
Expand All @@ -17,14 +16,19 @@
* This class is internal and is hence not for public use. Its APIs are unstable and can change at
* any time.
*/
@SuppressWarnings("deprecation") // this class will be removed in the 2.0 version
public final class InternalNetServerAttributesExtractor<REQUEST, RESPONSE> {

private final NetServerAttributesGetter<REQUEST, RESPONSE> getter;
private final io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>
getter;
private final FallbackAddressPortExtractor<REQUEST> fallbackAddressPortExtractor;
private final boolean emitOldHttpAttributes;

public InternalNetServerAttributesExtractor(
NetServerAttributesGetter<REQUEST, RESPONSE> getter,
io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
REQUEST, RESPONSE>
getter,
FallbackAddressPortExtractor<REQUEST> fallbackAddressPortExtractor,
boolean emitOldHttpAttributes) {
this.getter = getter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.FallbackAddressPortExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import javax.annotation.Nullable;

/**
Expand All @@ -32,13 +33,12 @@ public static <REQUEST, RESPONSE> ClientAttributesExtractor<REQUEST, RESPONSE> c
private final InternalClientAttributesExtractor<REQUEST, RESPONSE> internalExtractor;

ClientAttributesExtractor(ClientAttributesGetter<REQUEST, RESPONSE> getter) {
// the ClientAttributesExtractor will always emit new semconv
internalExtractor =
new InternalClientAttributesExtractor<>(
getter,
FallbackAddressPortExtractor.noop(),
/* emitStableUrlAttributes= */ true,
/* emitOldHttpAttributes= */ false);
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalNetworkAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.NetworkTransportFilter;
import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import javax.annotation.Nullable;

/**
Expand All @@ -32,13 +33,12 @@ public static <REQUEST, RESPONSE> NetworkAttributesExtractor<REQUEST, RESPONSE>
private final InternalNetworkAttributesExtractor<REQUEST, RESPONSE> internalExtractor;

NetworkAttributesExtractor(NetworkAttributesGetter<REQUEST, RESPONSE> getter) {
// the NetworkAttributesExtractor will always emit new semconv
internalExtractor =
new InternalNetworkAttributesExtractor<>(
getter,
NetworkTransportFilter.alwaysTrue(),
/* emitStableUrlAttributes= */ true,
/* emitOldHttpAttributes= */ false);
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv());
Comment on lines +40 to +41
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to have the new extractors emit the old spec as well: this will ease the transition from the old Net* classes, and will reduce the @SuppressWarnings("deprecation") annotations in our codebase.

The only attributes that are not covered by the new extractors are net.transport and net.sock.family (because the new attributes have different values), but there's like ~5 instrumentations using these anyway, so we'll just suppress deprecations there.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.FallbackAddressPortExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalServerAttributesExtractor;
import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import javax.annotation.Nullable;

/**
Expand All @@ -26,22 +27,38 @@ public final class ServerAttributesExtractor<REQUEST, RESPONSE>
*/
public static <REQUEST, RESPONSE> ServerAttributesExtractor<REQUEST, RESPONSE> create(
ServerAttributesGetter<REQUEST, RESPONSE> getter) {
return new ServerAttributesExtractor<>(getter);
return new ServerAttributesExtractor<>(getter, InternalServerAttributesExtractor.Mode.PEER);
}

/**
* Returns a new {@link ServerAttributesExtractor} that will use the passed {@link
* ServerAttributesGetter}.
*
* @deprecated This method will be removed in the 2.0 release. It was only introduced to ease the
* transition from using the old {@code NetServerAttributesGetter} to the new {@code
* ...network} attribute getter classes.
*/
@Deprecated
public static <REQUEST, RESPONSE>
ServerAttributesExtractor<REQUEST, RESPONSE> createForServerSide(
ServerAttributesGetter<REQUEST, RESPONSE> getter) {
return new ServerAttributesExtractor<>(getter, InternalServerAttributesExtractor.Mode.HOST);
}

private final InternalServerAttributesExtractor<REQUEST, RESPONSE> internalExtractor;

ServerAttributesExtractor(ServerAttributesGetter<REQUEST, RESPONSE> getter) {
ServerAttributesExtractor(
ServerAttributesGetter<REQUEST, RESPONSE> getter,
InternalServerAttributesExtractor.Mode mode) {
// the ServerAttributesExtractor will always emit new semconv
internalExtractor =
new InternalServerAttributesExtractor<>(
getter,
(port, request) -> true,
FallbackAddressPortExtractor.noop(),
/* emitStableUrlAttributes= */ true,
/* emitOldHttpAttributes= */ false,
// this param does not matter when old semconv is off
InternalServerAttributesExtractor.Mode.HOST,
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv(),
mode,
/* captureServerSocketAttributes= */ true);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

@SuppressWarnings("deprecation") // testing deprecated class
@ExtendWith(MockitoExtension.class)
class InetSocketAddressNetServerAttributesGetterTest {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

@SuppressWarnings("deprecation") // testing deprecated class
class NetServerAttributesExtractorTest {

static class TestNetServerAttributesGetter
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter.network;

import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
import static java.util.Collections.emptyMap;
import static org.assertj.core.api.Assertions.entry;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

class ClientAttributesExtractorOldSemconvTest {

static class TestClientAttributesGetter
implements ClientAttributesGetter<Map<String, String>, Void> {

@Nullable
@Override
public String getClientAddress(Map<String, String> request) {
return request.get("address");
}

@Nullable
@Override
public Integer getClientPort(Map<String, String> request) {
String value = request.get("port");
return value == null ? null : Integer.parseInt(value);
}

@Nullable
@Override
public String getClientSocketAddress(Map<String, String> request, @Nullable Void response) {
return request.get("socketAddress");
}

@Nullable
@Override
public Integer getClientSocketPort(Map<String, String> request, @Nullable Void response) {
String value = request.get("socketPort");
return value == null ? null : Integer.parseInt(value);
}
}

@Test
void allAttributes() {
Map<String, String> request = new HashMap<>();
request.put("address", "opentelemetry.io");
request.put("port", "80");
request.put("socketAddress", "1.2.3.4");
request.put("socketPort", "8080");

AttributesExtractor<Map<String, String>, Void> extractor =
ClientAttributesExtractor.create(new TestClientAttributesGetter());

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build())
.containsOnly(entry(SemanticAttributes.HTTP_CLIENT_IP, "opentelemetry.io"));

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, null, null);
assertThat(endAttributes.build())
.containsOnly(
entry(SemanticAttributes.NET_SOCK_PEER_ADDR, "1.2.3.4"),
entry(SemanticAttributes.NET_SOCK_PEER_PORT, 8080L));
}

@Test
void noAttributes() {
AttributesExtractor<Map<String, String>, Void> extractor =
ClientAttributesExtractor.create(new TestClientAttributesGetter());

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

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), emptyMap(), null, null);
assertThat(endAttributes.build()).isEmpty();
}
}
Loading