Move PeerServiceAttributesExtractor from javaagent api to instrumentation api

[iNikem]

As it does not have any dependency on javaagent

[mateuszrzeszutek]

The PeerServiceAttributesExtractor is a javaagent-only thing that's not really useable by the library instrumentations. Should we really move it to instrumentation-api?
And also most (if not all) of the classes from javaagent-instrumentation-api don't have any dependency on the javaagent.

[iNikem]

The PeerServiceAttributesExtractor is a javaagent-only thing that's not really useable by the library instrumentations. Should we really move it to instrumentation-api? And also most (if not all) of the classes from javaagent-instrumentation-api don't have any dependency on the javaagent.

why is this javaagent-only? I don't understand why library instrumentations cannot use it?

[anuraaga]

Mapping hostnames to service name is pretty fragile in practice (just run a container in docker-compose and it doesn't work anymore, having to update the mapping every time) but is the only mechanism we've come up with for setting peer.service in the javaagent. When initializing library instrumentation, it's better in every way to pass in AttributeExtractor.constant(SemanticAttributes.PEER_SERVICE, "auth-service) (we don't provide it yet but want to) when initializing the library instrumentation). So it's intentional to keep the mapping approach to javaagent-only.

[iNikem]

Mapping hostnames to service name is pretty fragile in practice (just run a container in docker-compose and it doesn't work anymore, having to update the mapping every time) but is the only mechanism we've come up with for setting peer.service in the javaagent. When initializing library instrumentation, it's better in every way to pass in AttributeExtractor.constant(SemanticAttributes.PEER_SERVICE, "auth-service) (we don't provide it yet but want to) when initializing the library instrumentation). So it's intentional to keep the mapping approach to javaagent-only.

You cannot assume that given library communicates only with one service. Do you mean we should provide per-call extractors?

[anuraaga]

You're right bad example, it needs to be per stub. For example, Brave has this pattern

https://github.com/openzipkin/brave/blob/5be287b91c3d18da9fc7a597d71b12b27be6043c/instrumentation/http/src/main/java/brave/http/HttpTracing.java#L114

and I think something like that, for updating service name per stub in code tends to be easiest to manage in practice compared to host mapping.

[iNikem]

Ok, next question about PeerServiceAttributesExtractor: we currently require every user of Instrumenters to manually add this extractor. Does it make sense to incorporate its functionality right inside NetAttributesExtractor and thus simplify its usage and remove this class for public API?

[mateuszrzeszutek]

Does it make sense to incorporate its functionality right inside NetAttributesExtractor and thus simplify its usage and remove this class for public API?

Hmm, what would be the source of peer service mappings for NetAttributesExtractor? Since Config is a javaagent-only concept.

I guess we could do something like https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/4237/files#diff-e9a4a5cb8f2720dbde375382b17292bec6875d9c892f2a0f3ae51975bde40a95 for the net extractor, if the general idea in my PR gets accepted -- and the javaagent could construct those peer service mappings from the Config (sort of similar to what I do in my PR for headers)

[iNikem]

@mateuszrzeszutek Can you please explain again, why Config is purely agent concept not relevant for library instrumentations? Or is it written somewhere already?

[mateuszrzeszutek]

I don't think we have that documented anywhere (we should've done that though...).
I vaguely remember discussions in some PRs (or maybe even in one of the SIGs?) that all configuration knobs for the library instrumentations should be kept in the *TracingBuilder classes - we shouldn't use Config and/or system properties, everything should be configurable via code/builder setting.
Config should be left for the javaagent and its instrumentations/features, because we need to be able to set these knobs without changing the application code.

[iNikem]

we shouldn't use Config and/or system properties, everything should be configurable via code/builder setting.
Config should be left for the javaagent and its instrumentations/features, because we need to be able to set these knobs without changing the application code.

I argue that a lot of deployments will want the possibility to change instrumentation behaviour without touching application code. Platform teams and SRE will be quite happy about that. This very same peer mapping may very well be different in different environments, thus it cannot be configure programmatically.

[mateuszrzeszutek]

That's a good point - if we only provide builder settings we'll force users of library instrumentations to handle properties/env vars themselves. Which may or may not be okay

[mateuszrzeszutek]

Ok, next question about PeerServiceAttributesExtractor: we currently require every user of Instrumenters to manually add this extractor. Does it make sense to incorporate its functionality right inside NetAttributesExtractor and thus simplify its usage and remove this class for public API?

So, now that we've decided to keep Config in instrumentation-api, should we merge the PeerServiceAttributesExtractor into NetAttributesExtractor and make the latter configurable (with the default value extracted from Config)?

[anuraaga]

We need to be able to allow instrumentation to allow a programmatic knob for peer.service specifically since it's so important - I'm not sure we could do that cleanly if it was merged into NetAttributes since it would mean overriding just one part of NetAttributes not the whole thing. peer.service probably needs to be an independent concept.

[iNikem]

@open-telemetry/java-instrumentation-approvers please review this PR as-is and express your approval if you do. If this PR gets 2 approvals, I will merge it. Otherwise I will close it in 2 days.

I personally still want to move this class to better demonstrate that javaagent-instrumentation-api module is so small that it probably does not require a separate module.

[trask]

Let's give it a try. I will make an attempt to integrate this as fallback for library instrumentation as we discussed in Tue SIG.