[WIP] Add peer.service to Instrumenter API

[mateuszrzeszutek]

We should add peer.service extractor to Instrumenter API as soon as possible so that we don't lose existing functionality when migrating tracers.
This attribute is only used in the agent - the problem is, NetAttributesExtractor implementations are package-private in library instrumentations and agent code currently has no way to inject another extractor that depends on the private net one.
For example:

private static final ArmeriaTracing TRACING = ArmeriaTracing.create(GlobalOpenTelemetry.get());

I thought of several options on how to handle this:

1. Just add the attribute to NetAttributesExtractor;
2. Create a separate extractor just for peer.service and keep it in instrumentation-api so that it's accessible to library instrumentation code; the library instrumentations have to add it even though it's useless without javaagent (this PR implements this solution);
3. Add a getter to the builder: InstrumenterBuilder#attributeExtractors() (and library instrumentations tracing builders...) and in the agent code find net attributes extractor using instanceof and if it's there, add the peer service one. This is hacky, and you have to do it in each javaagent instrumentation...
4. Add a Function<InstrumenterBuilder, InstrumenterBuilder> hook to instrumentation-api that'll be implemented in the javaagent (similar to e.g. rxjava hooks that we use in our instrumentations) that'll do the instanceof check. This option sort of implements Support for subclassing instrumentation Tracers in custom distributions #778

WDYT? I'd like to get your opinion on this matter as I don't have any strong preference towards any of these options (and maybe there's another one that I didn't think of)

TODO: tests, javadocs

[anuraaga]

Create a separate extractor just for peer.service and keep it in instrumentation-api so that it's accessible to library instrumentation code; the library instrumentations have to add it even though it's useless without javaagent (this PR implements this solution);

This doesn't seem to be the approach in this PR since you add it in javaagent instrumentation not library instrumentation, right?

4. Seems like an interesting option if we know other real use cases wonder if anything comes to mind?

[anuraaga]

I guess this should be in javaagent-api since it's only for auto instrumentation?

[mateuszrzeszutek]

That's the problem: if I put it inside javaagent-api then I'd have to make library instrumentations (armeria, okhttp, ...) depend on javaagent-api to use that (because NetAttributeExtractor implementations are package-private and not visible from the javaagent code).

[anuraaga]

Oops package names are so long so got confused. Hmm - yeah we definitely shouldn't use this code from library instrumentation, libraries are just supposed to be called with something like .addAttributeExtractor(FixedAttributeExtractor.of(PEER_SERVICE, "my-service")), there's no point of the map approach outside of agent and it shouldn't be supported.

Should we just access the package private ArmeriaNetAttributesExtractor from the agent reflectively?

[mateuszrzeszutek]

Should we just access the package private ArmeriaNetAttributesExtractor from the agent reflectively?

Hmm, that would work too -- I'll implement that option in this PR and we'll see how it looks.

[anuraaga]

This is the approach I had in mind. I guess it's a bit tedious, but we could do it for now as a quick fix, luckily being agent code we're never stuck with it long term.

[mateuszrzeszutek]

4. Seems like an interesting option if we know other real use cases wonder if anything comes to mind?

Adding custom attributes/metrics to built-in javaagent instrumentations?

[anuraaga]

This seems to do a good job of keeping the agent-specific logic to the agent. It's too bad it makes the net attributes not follow the pattern of the others with protected methods. Maybe we should just go with public for all of them though, since they're user-implemented classes the visibility doesn't matter that much I guess.

[mateuszrzeszutek]

Maybe we should just go with public for all of them though, since they're user-implemented classes the visibility doesn't matter that much I guess.

💯
That'll even make testing a bit easier, since you'll be able to call AttributesExtractor methods directly:

AttributesExtractor<?> ae = new MyExtractor<>();
// onStart is now public, can be called by other packages
ae.onStart(...);

[mateuszrzeszutek]

I ended up making only NetAttributesExtractor public - I'll change all other extractors in a separate PR, to minimize merge conflicts in this one.

[trask]

I'm ok with this, thanks for getting the functionality back. Hopefully we'll see some patterns emerge in the future and come up with something nicer (or realize that this is just a weird one-off and we can live with it).