Clarify http(s) default port handling for service.target_name

[z1c0]

When working on elastic/apm-agent-dotnet#1836 (span service inference tests) I encountered this:

While our JSON test spec is very explicit about the presence of ports, 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).

Assuming that the test spec is the authoritative source, the expected_resource is clearly without a (default) port number, so I think we should also state this in the pseudo-code.

{
  "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}`"
}

Update (28.10.2022)

After discussing this some more on #703 and in the agent weekly, I changed the direction of this PR to making the ports mandatory.

• The pseude code algorithm reflects this now.
• The test cases also expect ports now.

For that, I also changed the format of the http input data, as the existing format had a few shortcomings:

• It included no URL scheme
• Some test cases (e.g. "port" : -1) were hypothetical.

Before:

"http": {
  "url": {
    "host": "my-cluster.com",
    "port": 9200
}

Now:

"http": {
  "url": "https://my-cluster.com:9200"
}

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-11-28T05:29:00.100+0000

• Duration: 7 min 44 sec

[trentm]

I don't see any discussion in #627 (the PR introducing service.target.*) specifically about default ports for HTTP targets.

To a degree, the service.target.* fields were a refinement of the destination.service.target field, and its pseudo-code was:

else if (context.http?.url)
  if (context.http.url.port > 0)
    "${context.http.url.host}:${context.http.url.port}"
  else if (context.http.url.host)
    context.http.url.host

So I agree with your clarification.

aside

A possible wrinkle is the OTel Bridge spec. This pseudo-code in the OTel Bridge spec:

} 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']);
    }
}

suggests that a default port (for 'http' and 'https') should be used.

@SylvainJuge How much do we care to make these match?

1. I think this finer discussion could move to a separate issue so @z1c0's PR can move forward.
2. I think there is value in including the default port. Then a user's code making requests to https://example.com/... and https://example.com:443/... will have the same service.target.name.
3. However, I think this is a minor issue. I don't think user code would frequently use some requests with a port and some without.

:)

[z1c0]

Thanks for taking such a detailed look @trentm.

I was not aware of that OTel Bridge spec, thanks for bringing it up. My gut feeling would be to aim for consistency in our specs.

ad 2) Did you mean to say "... excluding the default port."? Otherwise, we would have the exact opposite effect and produce different values for service.target.name, right?

[trentm]

I agree that aiming for consistency would be good.

ad 2) Did you mean to say "... excluding the default port."? Otherwise, we would have the exact opposite effect and produce different values for service.target.name, right?

Actually I meant to bias to including the default port, so that we would tend to have service.target.name values that have the port. IIUC, service.target.* values are used in the APM UI Dependencies and Service Map views without the full URL or scheme, so the port can be useful to disambiguate. If a user sees a Service Map node for "example.com" they don't know if that was HTTP or HTTPS traffic. It would probably be useful to have separate Service Map nodes for "example.com:443" and "example.com:80" ... even when in the user code the usage is https://example.com/... and http://example.com/... (i.e. without the port specified in the URL).

I might be missing something about how we would have the exact opposite effect.

[z1c0]

ad 2 again) Sorry for this confusion, for some reason, I always read HTTP://example.com (instead of HTTPS). Of course, it makes sense what you are saying.

Thinking about this some more, I can see the value of including the ports in service.target.name.
My main intention here is to make our spec less ambiguous. If we decide to go with including ports, we should however

• make this explicit in the pseudo-code algorithm
• update the test cases in https://github.com/elastic/apm/blob/main/tests/agents/json-specs/service_resource_inference.json

[trentm]

If we decide to go with including ports, we should however: ...

Definitely agree.

I'd like to hear input from others to see if there are downsides/objections to changing the specs to prefer adding a default port to service.target.name and destination.service.target.

[z1c0]

Since we need to reach a consensus first, in which direction this PR should actually go (include ports: yes/no), I moved it into draft state for now and opened a dedicated discussion ticket: #703

[z1c0]

I updated the PR and its description.

[z1c0]

In the related discussion (#703) the feedback I had so far was "go with this approach" so I'll mark this "Ready for review" again.
FYI @gregkalapos