elastic / elastic/apm

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

Open
#703 3 comments 0 reactions 1 assignee Claimed by @z1c0 View on GitHub
discussion
Dominant language
Gherkin
Stars
427
Forks
125
PR merge metrics
No merged PRs in 30d

Description

*Note*: there is some initial discussion of this topic on https://github.com/elastic/apm/pull/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](https://github.com/elastic/apm/blob/main/tests/agents/json-specs/service_resource_inference.json#L229).

```json
{
"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).

```js
...
} 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](https://github.com/elastic/apm/blob/main/specs/agents/tracing-api-otel.md?rgh-link-date=2022-10-06T16%3A26%3A59Z#span-type-sub-type-and-service-target) (thanks for bringing this to my attention @trentm) which suggests that default ports **should** be used.

```js
...
} 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. https://github.com/elastic/apm/pull/700#issuecomment-1270501343).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.