Update net semantic convention changes

[trask]

Pros:

• minimum changes

Cons:

• the mapping between NetClientAttributesGetter and the semantic attributes feels a little magic because of the post-processing done in NetClientAttributesExtractor

Option B: don't do any post-processing in NetClientAttributesExtractor

Pros:

• clear/direct mapping between NetClientAttributesGetter and semantic attributes

Cons:

• end up duplicating logic in the InetSocketAddressNetClientAttributesGetter methods, which starts to feel inefficient since they are all called:

  @Override
  @Nullable
  public final String peerName(REQUEST request, @Nullable RESPONSE response) {
    InetSocketAddress address = getAddress(request, response);
    if (address == null) {
      return null;
    }
    String hostString = address.getHostString();
    if (hostString == null) {
      return null;
    }
    InetAddress remoteAddress = address.getAddress();
    if (remoteAddress != null && hostString.equals(remoteAddress.getHostAddress())) {
      // using net.sock.peer.addr in this case
      return null;
    }
    return hostString;
  }

  @Override
  @Nullable
  public final Integer peerPort(REQUEST request, @Nullable RESPONSE response) {
    InetSocketAddress address = getAddress(request, response);
    if (address == null) {
      return null;
    }
    String hostString = address.getHostString();
    if (hostString == null) {
      // using net.sock.peer.port in this case
      return null;
    }
    InetAddress remoteAddress = address.getAddress();
    if (remoteAddress != null && hostString.equals(remoteAddress.getHostAddress())) {
      // using net.sock.peer.port in this case
      return null;
    }
    return address.getPort();
  }

  @Override
  @Nullable
  public final String sockPeerAddr(REQUEST request, @Nullable RESPONSE response) {
    InetSocketAddress address = getAddress(request, response);
    if (address == null) {
      return null;
    }
    InetAddress remoteAddress = address.getAddress();
    if (remoteAddress != null) {
      return remoteAddress.getHostAddress();
    }
    return null;
  }

  @Override
  @Nullable
  public final Integer sockPeerPort(REQUEST request, @Nullable RESPONSE response) {
    InetSocketAddress address = getAddress(request, response);
    if (address == null) {
      return null;
    }
    String hostString = address.getHostString();
    if (hostString == null) {
      return address.getPort();
    }
    InetAddress remoteAddress = address.getAddress();
    if (remoteAddress != null && hostString.equals(remoteAddress.getHostAddress())) {
      return address.getPort();
    }
    // using net.peer.port in this case
    return null;
  }

Option C: introduce InetSocketAddressNetClientAttributesExtractor

...and change InetSocketAddressNetClientAttributesGetter to only expose address and transport

Pros:

• efficient
• no magic contract

Cons:

• no longer have a dedicated NetClientAttributesGetter that is always used for net client attributes, which is currently useful in PeerServiceAttributesExtractor and could be useful in other places in the future

[mateuszrzeszutek]

Just my 2c

• the mapping between NetClientAttributesGetter and the semantic attributes feels a little magic because of the post-processing done in NetClientAttributesExtractor

I think it's more important to have the API be easy to use (to make instrumenting things easier), rather than make it as close to semantic conventions. It's kind of the point to hide the complexities, different cases and requirements of the semantic conventions behind a very simple getter interface. The HTTP attributes extractors (the server one in particular) are a good example of that, as they add a significant chunk of very much needed logic over the raw values provided by a getter.

• no longer have a dedicated NetClientAttributesGetter that is always used for net client attributes, which is currently useful in PeerServiceAttributesExtractor and could be useful in other places in the future

We could always have 2 different getter interfaces for different scenarios, and have separate factory methods for raw data/InetSocketAddress -- similar to what we're already doping in DbClientSpanNameExtractor (I guess that we could simplify the attributes extractors in the db package so that there's exactly one public extractor for both SQL and general DB conventions)

[lmolkova]

the mapping between NetClientAttributesGetter and the semantic attributes feels a little magic because of the post-processing done in NetClientAttributesExtractor

it seems the post-processing (beyond setting attributes) is checking if sock attributes are not the same as net.peer. attributes, which doesn't seem too bad or magical, does it?

BTW, since we now have conditionally required attributes defined, it's a common pattern to guard attribute setting by something: default value, different from another attribute, another attribute set/not set.
I.e. this is a general question where to apply those conditions. I agree with @mateuszrzeszutek that it should be done centrally to make instrumentation easier.

[trask]

I agree with @mateuszrzeszutek that it should be done centrally to make instrumentation easier

thx both, I'm sold on this approach 👍

[trask]

alternatively we could check both net.sock.peer.addr and net.sock.peer.name, but now that we are putting IP address into net.peer.name when there's no hostname I don't think there is a need(?)

[mateuszrzeszutek]

@trask do you think this is ready for merge? Should we add it to 1.17?

[trask]

@trask do you think this is ready for merge? Should we add it to 1.17?

oops, I thought I had merged this! just fixed merge conflicts, yes I think we should merge it for 1.17