Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource host.hostname and host.name confusing #787

Closed
anuraaga opened this issue Aug 12, 2020 · 9 comments · Fixed by #838
Closed

Resource host.hostname and host.name confusing #787

anuraaga opened this issue Aug 12, 2020 · 9 comments · Fixed by #838
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:resource Related to the specification/resource directory

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Aug 12, 2020

I find the fact that Resource has two very similar attributes to be confusing, especially in terms of how to actually implement them in instrumentation.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/host.md#host

host.hostname - Hostname of the host. It contains what the hostname command returns on the host machine.
host.name - Name of the host. It may contain what hostname returns on Unix systems, the fully qualified, or a name specified by the user.

First inclination is the hostname command seems OS-specific (though I think all even windows do have it in practice) and it's strange to have a whole attribute dedicated to it.

I also wonder where other ways of getting hostname, such as polling a cloud service, or runtime hooks like InetAddress.getLocalHost().getHostName() fit in.

And I've seen this sort of complex logic to get a "good hostname". It seems to work great in practice. If this is a good way of getting hostname, maybe we can document this as the spec of host.name and delete host.hostname?
https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java#L205

@anuraaga anuraaga added the spec:trace Related to the specification/trace directory label Aug 12, 2020
@arminru arminru added area:semantic-conventions Related to semantic conventions release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:resource Related to the specification/resource directory and removed spec:trace Related to the specification/trace directory labels Aug 13, 2020
@arminru
Copy link
Member

arminru commented Aug 13, 2020

Good catch, it looks to me like if these slipped through when resource conventions were consolidated rather than having both separately on purpose. @tigrannajaryan maybe you can shed some light on that?
If there is some intended distinction, this needs to be pointed out more clearly. Otherwise I'd argue for going with just host.name and dropping host.hostname as @anuraaga suggested. Listing the different options queried by the code linked above and probably defining a precedence seems reasonable.

@tigrannajaryan
Copy link
Member

@arminru this is coming from OpenCensus conventions: https://github.com/census-instrumentation/opencensus-specs/blob/master/resource/StandardResources.md that were originally written by @bogdandrutu so maybe he can help clarify what the intent was.

@Oberon00
Copy link
Member

The examples at the linked spec are all the same string. I think the spec should either have a definition (ideally with an example of a typical situation) of how these can be different, or we remove host.hostname at least for now.

@Oberon00 Oberon00 changed the title Resource hostname and host.name confusing Resource host.hostname and host.name confusing Aug 18, 2020
@andrewhsu andrewhsu added the priority:p2 Medium priority level label Aug 18, 2020
@bogdandrutu
Copy link
Member

Ok o remove the hostname. Will do a PR.

bogdandrutu added a commit to bogdandrutu/opentelemetry-specification that referenced this issue Aug 19, 2020
Fixes open-telemetry#787

Idea from the issue is that we should use `host.name` with either of the proposed values.

Signed-off-by: Bogdan Drutu <[email protected]>
@jrcamp
Copy link
Contributor

jrcamp commented Aug 20, 2020

While looking at existing usages I came across hostname defined in spans:

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/span-general.md

Is there a reason the semantic conventions need to differ between metrics and traces?

@arminru
Copy link
Member

arminru commented Aug 20, 2020

@jrcamp There should not be any semantic difference between span attributes and metric labels of the same name (see #832 which will explicitly specify this in the spec). Resource attributes, which this PR is about, however, are different. They describe the entity from which the traces and metrics originate.
In this particular case, the host.name resource attribute describes the hostname of the host running the instrumented process as observed by itself (e.g., by invoking the hostname command). The net.host.name span attribute you mentioned above however specifies the hostname a peer used to connect to this host on incoming connections as specified in the note below the table. This hostname might be different from the one the host determined for itself since multiple hostnames might point to the same machine.

@markdascher
Copy link

#838 removed host.hostname from one page, but it's still mentioned in other parts of OTEL. For example, logs/data-model.md suggests mapping syslog hostname to host.hostname.

Should every host.hostname reference be replaced with host.name in the entire OTEL spec? (Is that even possible, since "Logs Data Model" is listed as stable?)

@tigrannajaryan
Copy link
Member

@markdascher thanks for catching.

Should every host.hostname reference be replaced with host.name in the entire OTEL spec?

Yes, I think so.

(Is that even possible, since "Logs Data Model" is listed as stable?)

Yes, that is fine, since it is clearly a bug in the spec, we just forgot to update it. Bug fixes are allowed in stable documents.

Would you be able to submit a PR to fix this or file an issue to get this fixed?

@markdascher
Copy link

Sure, I filed #3102 with the details. Thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants