Rename and move PeerServiceResolver and friends#16071
Rename and move PeerServiceResolver and friends#16071trask merged 3 commits intoopen-telemetry:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR renames classes from PeerService* to ServicePeer* to align with the new service.peer.name semantic convention attribute (renamed from peer.service). The PR follows a clean deprecation strategy where old classes delegate to new implementations, making this a safe refactoring.
Changes:
- Created new
ServicePeerResolver,ServicePeerResolverImpl, andServicePeerAttributesExtractorclasses in thesemconv.servicepackage - Created new
HttpClientServicePeerAttributesExtractorin thesemconv.httppackage - Deprecated old
PeerService*classes which now extend/delegate to the new classes - Updated all usages across instrumentation modules to use the new classes
- Added test coverage for new classes and updated tests for deprecated classes with proper annotations
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ServicePeerResolver.java | New interface for service peer resolution with factory methods |
| ServicePeerResolverImpl.java | Internal implementation of ServicePeerResolver (moved from PeerServiceResolverImpl) |
| ServicePeerAttributesExtractor.java | New attributes extractor for service.peer.name attribute |
| HttpClientServicePeerAttributesExtractor.java | New HTTP client-specific attributes extractor with Host header fallback |
| PeerServiceResolver.java | Deprecated interface now extending ServicePeerResolver |
| PeerServiceResolverImpl.java | Deprecated implementation now delegating to ServicePeerResolverImpl |
| PeerServiceAttributesExtractor.java | Deprecated extractor now delegating to ServicePeerAttributesExtractor |
| HttpClientPeerServiceAttributesExtractor.java | Deprecated HTTP extractor now delegating to new class |
| CommonConfig.java | Updated to use ServicePeerResolver type |
| DefaultHttpClientInstrumenterBuilder.java | Updated parameter names and imports |
| Various instrumentation modules | Updated imports and method calls to use new classes |
| Test files | Added tests for new classes, updated deprecated class tests with @SuppressWarnings |
| AbstractOtelSpringStarterSmokeTest.java | Added clarifying comment about JMX metrics scope |
2dd6281 to
14cf79f
Compare
3675ea9 to
7523619
Compare
7523619 to
1ac5f18
Compare
| implements DeclarativeConfigProperties { | ||
|
|
||
| private static final String GENERAL_PEER_SERVICE_MAPPING = "general.peer.service_mapping"; | ||
| private static final String JAVA_COMMON_SERVICE_PEER_MAPPING = "java.common.service_peer_mapping"; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Some of this copied from PeerServiceResolverImpl.java
git diff upstream/main:instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/net/PeerServiceResolverImpl.java instrumentation-api-incubator/src/main/java/io/opentelemetry/
instrumentation/api/incubator/semconv/service/peer/internal/ServicePeerResolver.java
diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/net/PeerServiceResolverImpl.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/service/peer/internal/ServicePeerResolver.java
index cc741640fb..d7d7f5faed 100644
--- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/net/PeerServiceResolverImpl.java
+++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/service/peer/internal/ServicePeerResolver.java
@@ -3,51 +3,125 @@
* SPDX-License-Identifier: Apache-2.0
*/
-package io.opentelemetry.instrumentation.api.incubator.semconv.net;
+package io.opentelemetry.instrumentation.api.incubator.semconv.service.peer.internal;
+import static java.util.Collections.emptyList;
import static java.util.Comparator.comparing;
import static java.util.Comparator.naturalOrder;
import static java.util.Comparator.nullsFirst;
import com.google.auto.value.AutoValue;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.AttributeKey;
+import io.opentelemetry.instrumentation.api.incubator.config.internal.DeclarativeConfigUtil;
import io.opentelemetry.instrumentation.api.incubator.semconv.net.internal.UrlParser;
+import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
+import java.util.function.BiConsumer;
import java.util.function.Supplier;
import javax.annotation.Nullable;
-class PeerServiceResolverImpl implements PeerServiceResolver {
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public class ServicePeerResolver {
+
+ // copied from PeerIncubatingAttributes
+ private static final AttributeKey<String> PEER_SERVICE = AttributeKey.stringKey("peer.service");
+ // copied from ServiceIncubatingAttributes
+ private static final AttributeKey<String> SERVICE_PEER_NAME =
+ AttributeKey.stringKey("service.peer.name");
+ private static final AttributeKey<String> SERVICE_PEER_NAMESPACE =
+ AttributeKey.stringKey("service.peer.namespace");
private static final Comparator<ServiceMatcher> matcherComparator =
nullsFirst(
comparing(ServiceMatcher::getPort, nullsFirst(naturalOrder()))
.thenComparing(comparing(ServiceMatcher::getPath, nullsFirst(naturalOrder()))));
- private final Map<String, Map<ServiceMatcher, String>> mapping = new HashMap<>();
-
- PeerServiceResolverImpl(Map<String, String> peerServiceMapping) {
- peerServiceMapping.forEach(
- (key, serviceName) -> {
- String url = "https://" + key;
- String host = UrlParser.getHost(url);
- Integer port = UrlParser.getPort(url);
- String path = UrlParser.getPath(url);
- Map<ServiceMatcher, String> matchers =
- mapping.computeIfAbsent(host, x -> new HashMap<>());
- matchers.putIfAbsent(ServiceMatcher.create(port, path), serviceName);
- });
+ private final Map<String, Map<ServiceMatcher, ServicePeer>> servicePeerMapping = new HashMap<>();
+
+ public ServicePeerResolver(OpenTelemetry openTelemetry) {
+ DeclarativeConfigUtil.getInstrumentationConfig(openTelemetry, "common")
+ .getStructuredList("service_peer_mapping", emptyList())
+ .forEach(
+ entry -> {
+ String peer = entry.getString("peer");
+ String serviceName = entry.getString("service_name");
+ String serviceNamespace = entry.getString("service_namespace");
+ if (peer != null && (serviceName != null || serviceNamespace != null)) {
+ addMapping(peer, serviceName, serviceNamespace);
+ }
+ });
+ }
+
+ @SuppressWarnings("deprecation") // used by deprecated PeerServiceResolver
+ public ServicePeerResolver(Map<String, String> peerServiceNameMapping) {
+ peerServiceNameMapping.forEach((peer, serviceName) -> addMapping(peer, serviceName, null));
+ }
+
+ private void addMapping(
+ String peer, @Nullable String serviceName, @Nullable String serviceNamespace) {
+ if (serviceName == null && serviceNamespace == null) {
+ return;
+ }
+ ServicePeer info = new ServicePeer(serviceName, serviceNamespace);
+ String url = "https://" + peer;
+ String host = UrlParser.getHost(url);
+ Integer port = UrlParser.getPort(url);
+ String path = UrlParser.getPath(url);
+ Map<ServiceMatcher, ServicePeer> matchers =
+ servicePeerMapping.computeIfAbsent(host, x -> new HashMap<>());
+ matchers.putIfAbsent(ServiceMatcher.create(port, path), info);
}
- @Override
public boolean isEmpty() {
- return mapping.isEmpty();
+ return servicePeerMapping.isEmpty();
+ }
+
+ @SuppressWarnings("deprecation") // old semconv
+ public void resolve(
+ String host,
+ @Nullable Integer port,
+ Supplier<String> pathSupplier,
+ BiConsumer<AttributeKey<String>, String> attributeSetter) {
+ ServicePeer servicePeer = resolveServicePeer(host, port, pathSupplier);
+ if (servicePeer == null) {
+ return;
+ }
+
+ String name = servicePeer.name;
+ if (name != null) {
+ if (SemconvStability.emitOldServicePeerSemconv()) {
+ attributeSetter.accept(PEER_SERVICE, name);
+ }
+ if (SemconvStability.emitStableServicePeerSemconv()) {
+ attributeSetter.accept(SERVICE_PEER_NAME, name);
+ }
+ }
+ if (SemconvStability.emitStableServicePeerSemconv()) {
+ String namespace = servicePeer.namespace;
+ if (namespace != null) {
+ attributeSetter.accept(SERVICE_PEER_NAMESPACE, namespace);
+ }
+ }
+ }
+
+ // TODO: remove this method when deprecated PeerServiceResolver is removed
+ @Nullable
+ public String resolveServiceName(
+ String host, @Nullable Integer port, Supplier<String> pathSupplier) {
+ ServicePeer peer = resolveServicePeer(host, port, pathSupplier);
+ return peer != null ? peer.name : null;
}
- @Override
@Nullable
- public String resolveService(String host, @Nullable Integer port, Supplier<String> pathSupplier) {
- Map<ServiceMatcher, String> matchers = mapping.get(host);
+ ServicePeer resolveServicePeer(
+ String host, @Nullable Integer port, Supplier<String> pathSupplier) {
+ Map<ServiceMatcher, ServicePeer> matchers = servicePeerMapping.get(host);
if (matchers == null) {
return null;
}
@@ -58,11 +132,31 @@ class PeerServiceResolverImpl implements PeerServiceResolver {
.orElse(null);
}
+ // TODO: remove this method when deprecated PeerServiceResolver is removed
+ @SuppressWarnings("deprecation") // bridges deprecated PeerServiceResolver
+ public static ServicePeerResolver fromPeerServiceResolver(
+ io.opentelemetry.instrumentation.api.incubator.semconv.net.PeerServiceResolver resolver) {
+ return new ServicePeerResolver(new HashMap<>()) {
+ @Override
+ public boolean isEmpty() {
+ return resolver.isEmpty();
+ }
+
+ @Override
+ @Nullable
+ ServicePeer resolveServicePeer(
+ String host, @Nullable Integer port, Supplier<String> pathSupplier) {
+ String serviceName = resolver.resolveService(host, port, pathSupplier);
+ return serviceName != null ? new ServicePeer(serviceName, null) : null;
+ }
+ };
+ }
+
@AutoValue
abstract static class ServiceMatcher {
static ServiceMatcher create(@Nullable Integer port, @Nullable String path) {
- return new AutoValue_PeerServiceResolverImpl_ServiceMatcher(port, path);
+ return new AutoValue_ServicePeerResolver_ServiceMatcher(port, path);
}
@Nullable
@@ -92,4 +186,15 @@ class PeerServiceResolverImpl implements PeerServiceResolver {
return true;
}
}
+
+ private static final class ServicePeer {
+
+ @Nullable private final String name;
+ @Nullable private final String namespace;
+
+ ServicePeer(@Nullable String name, @Nullable String namespace) {
+ this.name = name;
+ this.namespace = namespace;
+ }
+ }
}| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. | ||
| */ | ||
| public class ServicePeerResolver { |
There was a problem hiding this comment.
it's currently being subclassed as part of the backcompat story
| String peer = entry.getString("peer"); | ||
| String serviceName = entry.getString("service_name"); | ||
| String serviceNamespace = entry.getString("service_namespace"); | ||
| if (peer != null && (serviceName != null || serviceNamespace != null)) { |
There was a problem hiding this comment.
Wondering whether we should emit a warning when user has specified invalid configuration. Idk whether there are other places where this could also apply.
| return; | ||
| } | ||
| ServicePeer info = new ServicePeer(serviceName, serviceNamespace); | ||
| String url = "https://" + peer; |
There was a problem hiding this comment.
https:// feels random, is it here only to satisfy the url parser?
There was a problem hiding this comment.
yeah, added comment (this is carried over from PeerServiceResolverImpl.java)
3d5ea91 to
5dfabd7
Compare
Related to #16047
This PR renames and refactors the peer service classes to align with the new
service.peer.nameandservice.peer.namespacesemantic convention attributes (renamed frompeer.service).Goals
PeerService*toServicePeer*to match the new attribute namesOpenTelemetrydirectly, hiding the resolver implementation as an internal detailservice.peer.namespacesupport - the new resolver supports bothservice_nameandservice_namespaceconfigurationWhat changed
New classes created
service.peer.ServicePeerAttributesExtractorhttp.HttpClientServicePeerAttributesExtractorOld classes deprecated
The old classes are deprecated and delegate to the new implementations:
net.PeerServiceResolver- deprecated, delegates toServicePeerResolvernet.PeerServiceResolverImpl- deleted (logic moved toServicePeerResolver)net.PeerServiceAttributesExtractor- deprecated, delegates toServicePeerAttributesExtractorhttp.HttpClientPeerServiceAttributesExtractor- deprecated, delegates toHttpClientServicePeerAttributesExtractorConfiguration changes
The declarative config path changed:
general.peer.service_mappingjava.common.service_peer_mapping(see open-telemetry/opentelemetry-configuration#526 for why)
The config format now supports both
service_nameandservice_namespace:Testing
Didn't introduce new tests, since the old tests cover the new classes via the delegation pattern.
When deprecated classes are removed, tests will be updated to point to the new classes.