GCE to also get hostname

[nhamlh]

For our usecase, we're running on GKE and would love to also have hostname of GCE requested and populated to otel-collector. Currently, the HostName returns InstanceName instead of hostname:

opentelemetry-operations-go/detectors/gcp/gce.go

Lines 41 to 44 in ca90414

	// GCEHostName returns the instance name of the instance on which this program is running
	func (d *Detector) GCEHostName() (string, error) {
	return d.metadata.InstanceName()
	}

I just wonder if there's any reason blocking adding it? If not, I can open a PR for this.

[dashpole]

That seems reasonable to me. Feel free to add it as GCEHostHostname or GCEHostname. As a note, GCE attributes are not accessible on GKE if you are using workload identity (or autopilot, which uses workload identity).

[dashpole]

In the collector, we need to make sure failure to obtain this information does not cause resource detection to fail entirely.

[nhamlh]

@dashpole as a side note, should we deprecate GCEHostName and change the name to GCEInstanceName? IMO, it'll avoid ambiguous; however this change will break downsteam project.

[dashpole]

I just realized they changed the host name attributes recently: open-telemetry/opentelemetry-specification#838. It appears we can only have either the name of the host, or the hostname of the instance as a semantic convention.

I assume hostname and instance name are generally significantly different?

[nhamlh]

In this case I think they are not that much of the difference. Actually, exposing GCE hostname would leave users with a more flexible of choices: while GCE on GKE expose hostname instead of instance name, users can use gcp detector to get the hostname, or system detector to get instance name.

In resourcedetection processor we can add a fallback from instance name to hostname, wdyt?

[dashpole]

WDYM about the system detector getting the instance name? I wouldn't expect the system detector to use the GCE metadata server.

The instance name is critical when running on GCE, though... I think we need to find another place to put it before replacing it with hostname.

[nhamlh]

The system detector doesn't use GCE metadata server, it picks the hostname locally if used with hostname_sources: ["os"] which is actually the instance.name metadata. So users can choose to use system detector or gcp detector's fqdn hostname for host.name, that's the flexible I mentioned before.

[mx-psi]

As a note, GCE attributes are not accessible on GKE if you are using workload identity (or autopilot, which uses workload identity).

In this case, wouldn't we be able to get the cluster node name through the API server? AFAIK, instance/name returns the Kubernetes node name on GKE (when not using workload identity).

So users can choose to use system detector or gcp detector's fqdn hostname for host.name, that's the flexible I mentioned before.

Changing the host.name value would break some of our (Datadog) supported setups. I would suggest having GCP-specific attributes such as gcp.instance.name or gcp.instance.hostname to avoid breakage. I think this could be even eventually added at the OpenTelemetry specification.

[aabmass]

Thanks for the PR @nhamlh. We'll try to get this reviewed and merged soon

[dashpole]

I agree with mx-psi above. I would prefer to see this added to the specification before adding it to the resource detector.

[damemi]

To summarize, we can't change the value of host.name in the collector because that would be breaking. But, we can add 2 new gcp specific labels gcp.instance.name and gcp.instance.hostname in the resource detection processor.

The change in #515 alone won't break anything, because that's just adding the new method to the detector. For that reason, I think we could go ahead and merge #515 without any spec changes, but adding the new labels by default in the collector processor should have the spec change first. Does that sound right to everyone?

[damemi]

Opened open-telemetry/opentelemetry-specification#3384 upstream to add gcp.gce.instance.name and gcp.gce.instance.hostname

[damemi]

With open-telemetry/semantic-conventions#15 merged, we will be using the 2 new attributes mentioned above (once a new semconv release is published)

@nhamlh, are you still interested in working on #515 ?

[damemi]

Opened #669