How to handle http(s) default port when setting service.target_name

[z1c0]

Note: there is some initial discussion of this topic on #700 but since the direction of this PR became unclear, I decided to move the discussion to this dedicated issue.

The subject of this discussion is how to handle HTTP(S) default ports when setting service.target.

• Our JSON test spec does not expect the presence of default ports. See e.g. this test case.

{
  "span": {
    "exit": "true",
    "type": "external",
    "subtype": "http",
    "context": {
      "http": {
        "url": {
          "host": "my-cluster.com"
        }
      }
    }
  },
  "expected_resource": "my-cluster.com",
  "expected_service_target": {
    "type": "http",
    "name": "my-cluster.com"
  },
  "failure_message": "If `context.http.url.port` does not exist, output should be `${context.http.url.host}`"
}

• The pseudo-code in https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans-service-target.md#implementation-details only refers to a helper function getPortFromUrl that does not specify how to handle default HTTP ports (80/443).

...
    } else if (context.http?.url) { // http spans
      service_target.name = getHostFromUrl(context.http.url);
      port = getPortFromUrl(context.http.url);
      if (port > 0) {
        service_target.name += ":" + port;
      }
...

• There is also pseudo-code in the OTel Bridge spec (thanks for bringing this to my attention @trentm) which suggests that default ports should be used.

...
} else if (a['http.url'] || a['http.scheme']) {
    type = 'external';
    subtype = 'http';
    serviceTargetType = subtype;

    httpHost = a['http.host'] || netPeer;
    if (httpHost) {
        if (netPort < 0) {
            netPort = httpPortFromScheme(a['http.scheme']);
        }
        serviceTargetName = netPort < 0 ? httpHost : httpHost + ':' + netPort;
    } else if (a['http.url']) {
        serviceTargetName = parseNetName(a['http.url']);
    }
}
...

I think we should aim for consistency here between the general agent spec and the OTel Bridge spec.
Also, convincing arguments for including the default ports have been made by @trentm (see e.g. #700 (comment)).

[SylvainJuge]

For call to services through HTTP/HTTPS I would prefer to not have it, but it's definitely not that easy to choose what is best:

• network port is often a rather low-level detail, and I am not sure it provides a lot of value on the Map, but it depends on what we do expect to see on it: is the map an abstract view of the app and its ecosystem or an accurate technical view that includes low-level details like network ports ?
• for external services in public clouds, 99% of them will use HTTPS, so it is very unlikely to have both github.com vs github.com:443
• for internal services though, that's quite different, you could have a single hostname hosting multiple instances of the same service (for example a single host with 3 ES nodes, each with their own port).

In an ideal situation, we would be able to "show the port only when it's relevant", but we probably can't, so we might have to include it always for consistency. Maybe in the future when we'll have a kind of "service entity" we could solve that in a more elegant way and make the map (and dependencies) automatically display entities (and aggregate metrics) with an optimal granularity and/or fold automatically. For now the service.target and the destination.service.resource fields are directly tied to the Map and Service Dependencies UI, so we have to make compromises here.

[z1c0]

In a recent "APM Agents Weekly" we discussed this topic but did not reach a definite conclusion.
However, if I interpreted this conversation correctly, there was a tendency towards including those default ports for consistency reasons even though it "might not look that nicely on the service map".

I will go forward and update #700 (including the related json-spec file) accordingly.

@SylvainJuge @felixbarny please give me a 👍 / 👎 depending on whether you are OK with this approach or not.
Please also mention anyone else who still might have an opinion or feedback on this.

[SylvainJuge]

👍 to favor consistency on this one, we might find better solutions to the "display name" when we have a more entity-base approach available in the map.