PeerService Resolver

[mmorel-35]

Hi!

Here is a first draft of a Resolver for Peer Services.
For now protocol and path are not wired in instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/net/PeerServiceAttributesExtractor.java but PeerServiceResolver is already able to handle them.

What do you think ?

Fixes #9047

[breedx-splk]

Thanks for contributing! Sorry that I had so many comments. I don't think any of them should be particularly challenging to implement.

[mmorel-35]

Also, as otel.instrumentation.common.peer-service-mapping is defined once for all, the PeerServiceResolver could be Singleton but I don't see how to achieve this.

The config is created in javaagent-extension-api and the extractor in ‎instrumentation-api-semconv. Would it be possible to place the PeerServiceResolver class in a way they can both use it ?

[mateuszrzeszutek]

Why is a port condition here? Isn't this part about the path?

[mmorel-35]

Yes it is. When the matcher has a path, the tested port need to match the matcher's port. Otherwise with the mapping :
localhost/mypath=myservice

localhost:4859/mypath and localhost:8081/mypath would both be resolved as myservice.

[mateuszrzeszutek]

Yeah, but doesn't the

        if (port != null) {
          return port.equals(this.port);
        }

condition above already handle that?

[mmorel-35]

The checker can have a null port.
The first port checking requires the two ports to match if the checker has a defined port.

When it comes to path checking.
Once the path are matching, then the ports must also be equals.

condition above already handle that?

With the tests I did, it was not enough.

I'm interested if there is a shorter and easier way to ensure that without making port a required attribute.

[mateuszrzeszutek]

I'm not sure I get the scenario you're trying to cover.

So, when the mapping is 1.2.3.4/abc -> def and you pass (1.2.3.4, 8080, /abc) shouldn't you get def in return? That extra condition makes it return null, which does not seem correct to me.

[mmorel-35]

If you have the mapping (1.2.3.4, null, /abc) -> def but in reality you have (1.2.3.4, 8080, /abc) -> def and (1.2.3.4, 8081, /abc) -> ghi, I believe that it's better to return null so the mapping has to be fixed to have the port provided instead of having a misleading mapping.

[mateuszrzeszutek]

Also, as otel.instrumentation.common.peer-service-mapping is defined once for all, the PeerServiceResolver could be Singleton but I don't see how to achieve this.

The config is created in javaagent-extension-api and the extractor in ‎instrumentation-api-semconv. Would it be possible to place the PeerServiceResolver class in a way they can both use it ?

You could make the PeerServiceResolver a functional interface, with a static create(Map<String, String>) factory method that would built the resolver from the configuration. Then, the PeerServiceAttributesExtractor factory methods could accept a PeerServiceResolver instead of the string map; you could build and store the singleton resolver instance in the javaagent CommonConfig. This way you could remove the MAPPINGS global from the resolver implementation.

[mateuszrzeszutek]

what do you think about passing in the http.url (and only the http.url) to the PeerServiceResolver? and the PeerServiceResolver could then just check matches directly against the http.url without needing to do any parsing I think

Alternatively, the resolver could accept a Supplier<String> just for the path parameter, and only lazily evaluate that when host & port match.

[mmorel-35]

Hi @trask , @mateuszrzeszutek ,
Any news on this ?
Can it be in the next release?
Can it be merged as is and then you optimize it with your new ideas?

[mateuszrzeszutek]

Approved 👍

@trask there is one more open discussion about the port behavior: #9061 (comment)
Perhaps we should merge this PR and tackle that later?

[trask]

@trask there is one more open discussion about the port behavior: #9061 (comment)

I just resolved that discussion (sorry, I was having boolean logic failures in my head).

I'd still prefer not to add url parsing on the hot path for a feature that most people won't be using, so would prefer if we can implement some kind of lazy evaluation in this PR. I think the Supplier<String> proposal sounds reasonable.

@mmorel-35 can you try that out?

[mmorel-35]

Hi @trask ,
It’s done.

[trask]

thanks @mmorel-35 ❤️