-
Notifications
You must be signed in to change notification settings - Fork 867
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
PeerService Resolver #9061
Merged
Merged
PeerService Resolver #9061
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
3ee0800
Peer Service Mapping
mmorel-35 cd14177
Merge branch 'main' into issue-9047
mmorel-35 960c902
up
mmorel-35 4c45bc5
Merge remote-tracking branch 'upstream/main' into issue-9047
mmorel-35 1f18c1d
up
mmorel-35 ce1f072
Merge branch 'main' into issue-9047
mmorel-35 1dd3978
up
mmorel-35 d8dc4b6
Apply suggestions from code review
mmorel-35 0997225
Merge branch 'main' into issue-9047
mmorel-35 f80caf7
Merge branch 'main' into issue-9047
laurit e7be2c1
fix merge
laurit 6e8824a
import new semconv class
laurit 07bc886
Update PeerServiceResolverImpl.java
mmorel-35 4f3a714
Update PeerServiceResolver.java
mmorel-35 9f79f92
Update PeerServiceResolver.java
mmorel-35 90448e6
Update HttpClientPeerServiceAttributesExtractor.java
mmorel-35 4e90b37
Update PeerServiceResolverTest.java
mmorel-35 c0cfe8b
Update PeerServiceResolverTest.java
mmorel-35 8424036
Update HttpClientPeerServiceAttributesExtractor.java
mmorel-35 b73017f
Merge branch 'main' into issue-9047
mmorel-35 5b8f103
ge
mmorel-35 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
94 changes: 94 additions & 0 deletions
94
...metry/instrumentation/api/instrumenter/http/HttpClientPeerServiceAttributesExtractor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.http; | ||
|
||
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.PeerServiceResolver; | ||
import io.opentelemetry.instrumentation.api.instrumenter.url.UrlParser; | ||
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; | ||
import javax.annotation.Nullable; | ||
|
||
/** | ||
* Extractor of the {@code peer.service} span attribute, described in <a | ||
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/span-general.md#general-remote-service-attributes">the | ||
* specification</a>. | ||
*/ | ||
public final class HttpClientPeerServiceAttributesExtractor<REQUEST, RESPONSE> | ||
implements AttributesExtractor<REQUEST, RESPONSE> { | ||
|
||
private final HttpClientAttributesGetter<REQUEST, RESPONSE> attributesGetter; | ||
private final PeerServiceResolver peerServiceResolver; | ||
|
||
// visible for tests | ||
HttpClientPeerServiceAttributesExtractor( | ||
HttpClientAttributesGetter<REQUEST, RESPONSE> attributesGetter, | ||
PeerServiceResolver peerServiceResolver) { | ||
this.attributesGetter = attributesGetter; | ||
this.peerServiceResolver = peerServiceResolver; | ||
} | ||
|
||
/** | ||
* Returns a new {@link HttpClientPeerServiceAttributesExtractor} that will use the passed {@code | ||
* attributesGetter} instance to determine the value of the {@code peer.service} attribute. | ||
*/ | ||
public static <REQUEST, RESPONSE> | ||
HttpClientPeerServiceAttributesExtractor<REQUEST, RESPONSE> create( | ||
HttpClientAttributesGetter<REQUEST, RESPONSE> attributesGetter, | ||
PeerServiceResolver peerServiceResolver) { | ||
return new HttpClientPeerServiceAttributesExtractor<>(attributesGetter, peerServiceResolver); | ||
} | ||
|
||
@Override | ||
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {} | ||
|
||
@Override | ||
public void onEnd( | ||
AttributesBuilder attributes, | ||
Context context, | ||
REQUEST request, | ||
@Nullable RESPONSE response, | ||
@Nullable Throwable error) { | ||
|
||
if (peerServiceResolver.isEmpty()) { | ||
// optimization for common case | ||
return; | ||
} | ||
|
||
String serverAddress = attributesGetter.getServerAddress(request); | ||
Integer serverPort = attributesGetter.getServerPort(request); | ||
String path = getUrlPath(attributesGetter, request); | ||
String peerService = mapToPeerService(serverAddress, serverPort, path); | ||
if (peerService == null) { | ||
String serverSocketDomain = attributesGetter.getServerSocketDomain(request, response); | ||
Integer serverSocketPort = attributesGetter.getServerSocketPort(request, response); | ||
peerService = mapToPeerService(serverSocketDomain, serverSocketPort, null); | ||
} | ||
if (peerService != null) { | ||
attributes.put(SemanticAttributes.PEER_SERVICE, peerService); | ||
} | ||
} | ||
|
||
@Nullable | ||
private String mapToPeerService( | ||
@Nullable String host, @Nullable Integer port, @Nullable String path) { | ||
if (host == null) { | ||
return null; | ||
} | ||
return peerServiceResolver.resolveService(host, port, path); | ||
} | ||
|
||
@Nullable | ||
private String getUrlPath( | ||
HttpClientAttributesGetter<REQUEST, RESPONSE> attributesGetter, REQUEST request) { | ||
String urlFull = attributesGetter.getUrlFull(request); | ||
if (urlFull == null) { | ||
return null; | ||
} | ||
return UrlParser.getPath(urlFull); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 21 additions & 0 deletions
21
.../main/java/io/opentelemetry/instrumentation/api/instrumenter/net/PeerServiceResolver.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.net; | ||
|
||
import java.util.Map; | ||
import javax.annotation.Nullable; | ||
|
||
public interface PeerServiceResolver { | ||
|
||
public boolean isEmpty(); | ||
|
||
@Nullable | ||
public String resolveService(String host, @Nullable Integer port, @Nullable String path); | ||
|
||
static PeerServiceResolver create(Map<String, String> mapping) { | ||
return new PeerServiceResolverImpl(mapping); | ||
} | ||
} |
93 changes: 93 additions & 0 deletions
93
...n/java/io/opentelemetry/instrumentation/api/instrumenter/net/PeerServiceResolverImpl.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.net; | ||
|
||
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.instrumentation.api.instrumenter.url.UrlParser; | ||
import java.util.Comparator; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
import javax.annotation.Nullable; | ||
|
||
class PeerServiceResolverImpl implements PeerServiceResolver { | ||
|
||
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); | ||
}); | ||
} | ||
|
||
@Override | ||
public boolean isEmpty() { | ||
return mapping.isEmpty(); | ||
} | ||
|
||
@Override | ||
@Nullable | ||
public String resolveService(String host, @Nullable Integer port, @Nullable String path) { | ||
Map<ServiceMatcher, String> matchers = mapping.get(host); | ||
if (matchers == null) { | ||
return null; | ||
} | ||
return matchers.entrySet().stream() | ||
.filter(entry -> entry.getKey().matches(port, path)) | ||
.max((o1, o2) -> matcherComparator.compare(o1.getKey(), o2.getKey())) | ||
.map(Map.Entry::getValue) | ||
.orElse(null); | ||
} | ||
|
||
@AutoValue | ||
abstract static class ServiceMatcher { | ||
|
||
static ServiceMatcher create(Integer port, String path) { | ||
return new AutoValue_PeerServiceResolverImpl_ServiceMatcher(port, path); | ||
} | ||
|
||
@Nullable | ||
abstract Integer getPort(); | ||
|
||
@Nullable | ||
abstract String getPath(); | ||
|
||
public boolean matches(Integer port, String path) { | ||
if (this.getPort() != null) { | ||
if (!this.getPort().equals(port)) { | ||
return false; | ||
} | ||
} | ||
trask marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (this.getPath() != null && this.getPath().length() > 0) { | ||
if (path == null) { | ||
return false; | ||
} | ||
if (!path.startsWith(this.getPath())) { | ||
return false; | ||
} | ||
if (port != null) { | ||
return port.equals(this.getPort()); | ||
} | ||
trask marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return true; | ||
} | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
it would be nice if we could avoid the url parsing in the common case where there's no peer service mapping (and even better also in the case where there's no peer service mapping that depends on path)
what do you think about passing in the
http.url
(and only thehttp.url
) to the PeerServiceResolver? and the PeerServiceResolver could then just check matches directly against thehttp.url
without needing to do any parsing I thinkI guess this would only work for http client instrumentation.
Maybe there's not a one-size-fits-all PeerServiceResolver interface.
E.g. for database instrumentation, you may still want a PeerServiceResolver, but
path
doesn't make sense in that case, sopath
already is specific to http PeerServiceResolvers.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.
It would be nice indeed !
I don't mind seing the PeerServiceResolver having different interfaces depending on the client instrumentation type. Does it has an impact on the agent configuration like another property or having to precise the port explicitly ?
That seems quite clean !
I see your point on performance here but I don't see yet what will be the impact on the configuration of the agent and how it would be implemented.
If you want to proceed on that trajectory, feel free to take it from here.
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.
Alternatively, the resolver could accept a
Supplier<String>
just for the path parameter, and only lazily evaluate that when host & port match.