-
Notifications
You must be signed in to change notification settings - Fork 873
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
/** | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The only attributes that are not covered by the new extractors are |
||
} | ||
|
||
@Override | ||
|
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(); | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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