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

Add semantic conventions for GCP resources #3384

Closed
wants to merge 2 commits into from

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Apr 12, 2023

Fixes #

Changes

This adds GCP-specific semantic conventions for GCE instances. Specifically, we would like to provide 2 ways of identifying host names: by their GCE instance name and their fully-qualified hostname (see GoogleCloudPlatform/opentelemetry-operations-go#511 for more downstream discussion)

(Following the pattern for AWS in #1099)

Related issues # GoogleCloudPlatform/opentelemetry-operations-go#511

Related OTEP(s) #

@damemi damemi requested review from a team April 12, 2023 18:00
@damemi
Copy link
Contributor Author

damemi commented Apr 12, 2023

cc @punya @jsuereth @aabmass @psx95

@punya
Copy link
Member

punya commented Apr 12, 2023

Do all GCE instances have a well-defined and unique fully qualified hostname?

@damemi
Copy link
Contributor Author

damemi commented Apr 12, 2023

Do all GCE instances have a well-defined and unique fully qualified hostname?

I think so, based on https://cloud.google.com/compute/docs/instances/custom-hostname-vm:

When you create a virtual machine (VM) instance, Google Cloud creates an internal DNS name from the VM name. Unless you specify a custom hostname, Google Cloud uses the automatically created internal DNS name as the hostname it provides to the VM.

Though I suppose this doesn't guarantee uniqueness, because you could set a custom hostname. We would want to provide guidance on handling that to avoid collisions.

@punya
Copy link
Member

punya commented Apr 13, 2023

Thanks for the documentation pointer. It looks like the internal DNS name is always defined, and (for projects created since 2018), has the form VM_NAME.ZONE.c.PROJECT_ID.internal, which is redundant with information we put into other resource attributes already. Can we learn more about the benefit/goal of adding this redundant information? If it's only useful to a small subset of users, might it be better for them to construct it (e.g. using a processor) rather than taking up another attribute from the (often limited) limit of attributes for everyone?

@damemi
Copy link
Contributor Author

damemi commented Apr 13, 2023

@punya is that just the default DNS name? There could still be a benefit when setting a custom hostname that differs from that default. In that case instanceName != hostName != DNS name

Documenting how to get the default would be useful too. Do you have a link to where you found that?

@punya
Copy link
Member

punya commented Apr 13, 2023

Documenting how to get the default would be useful too. Do you have a link to where you found that?

It's a link from your previous comment, https://cloud.google.com/compute/docs/internal-dns

@damemi
Copy link
Contributor Author

damemi commented Apr 13, 2023

@punya ah I missed that definition (I just copied the higher level explanation from the custom hostnames doc)

Do you think custom hostnames are a reasonable justification for this?

@punya
Copy link
Member

punya commented Apr 13, 2023

If it's a hostname in the generic sense(hostname -f), then it's not really GCE-specific. That should be handled by a different detector IMO.

@damemi
Copy link
Contributor Author

damemi commented Apr 13, 2023

There is already host.name, which is the generic Otel solution for host names. Where this becomes a provider-specific question is how host.name is populated, because something like the Instance name comes from GCP.

In that respect, we probably should have set host.name using the actual Hostname (since hostname is a generic concept), and provided Instance name as a GCP resource attribute. But from a practical standpoint, the Instance name is probably what most users want (especially given that the default Hostname is comprised of other available attributes, like you pointed out). So, today we use Instance name as the value for host.name, and providing both values as GCP attributes gives users the choice of which to use.

An alternative could be to make this a config option in the resource detector. I don't know what level of config we can provide in that processor, but something like hostnameValue: <instance/hostname> could also give users the choice.

@mx-psi
Copy link
Member

mx-psi commented Apr 13, 2023

An alternative could be to make this a config option in the resource detector. I don't know what level of config we can provide in that processor, but something like hostnameValue: <instance/hostname> could also give users the choice.

I think that defeats the purpose of semantic conventions; they should have a single well-defined meaning so that backends can reliably interpret them as one thing or the other. If host.name can have 5 different meanings, we either have broken setups or have to rely on other values (something like hostnameValue.config: instance, which I think is not a good idea)

@punya
Copy link
Member

punya commented Apr 13, 2023

Should we consider breaking backwards compatibility in the GCP resource detector, and stop setting host.name?

@damemi
Copy link
Contributor Author

damemi commented Apr 13, 2023

@punya I don't think there is a need to break the existing convention right away. Could we recommend users switch to one of the new attributes and plan to stop setting host.name at some point in the future?

It is a bit weird to set the Instance name in 2 places (host.name and gcp.gce.instance.name), but the new attribute is more rigidly defined ie "This is always exactly the GCP GCE instance name" vs the current definition for the generic attribute:

Name of the host. On Unix systems, it may contain what the hostname command returns, or the fully qualified hostname, or another name specified by the user.

@punya
Copy link
Member

punya commented Apr 14, 2023

My apologies, I'll be out of the office for the next 2 weeks. @jsuereth has graciously agreed to handle the remaining review work.

brief: >
The instance name of a GCE instance.
examples: ['instance-1']
- id: instance.hostname
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just ref: host.name instead of define a new field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds good, but gcp.gce.instance.name would actually be the new field that should ref: host.name (currently host.name is set to the instance name)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming what we discussed, we use the instance name to set host.name (also for GKE). We don't currently set the actual hostname anywhere.

@damemi damemi requested a review from a team April 25, 2023 14:18
@github-actions
Copy link

github-actions bot commented May 3, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 3, 2023
@damemi damemi force-pushed the gcp-host-attrs branch from 7a58cd3 to 485f071 Compare May 8, 2023 11:55
@damemi
Copy link
Contributor Author

damemi commented May 8, 2023

@jsuereth I added much more description to these fields. I think the original decision to use Instance name as the host.name (ie the name of the host) was reasonable in hindsight, as most users probably just want the VM name rather than the full my-vm.us-east.project.c.internal default DNS name. It is confusingly overloaded with the actual term hostname (ie the hostname of the host), so I'm fine switching that if you think it's better.

@Oberon00
Copy link
Member

Oberon00 commented May 8, 2023

The host.name field in OTel seems to be very vaguely defined so that any of those choices would be fine. I wonder if we should tighten the definition or introduce more clearly defined separate fields like host.dns_name?

@github-actions github-actions bot removed the Stale label May 9, 2023
@reyang reyang added the area:semantic-conventions Related to semantic conventions label May 9, 2023
@reyang
Copy link
Member

reyang commented May 9, 2023

@damemi heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment).

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damemi
Copy link
Contributor Author

damemi commented May 12, 2023

@damemi damemi closed this May 12, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants