search

GoogleCloudPlatform / opentelemetry-operations-js

This repository is home to Google Cloud Exporters (Trace and Monitoring) for OpenTelemetry Node.js Project (https://github.com/open-telemetry/opentelemetry-js)
Apache License 2.0
70 stars 62 forks source link

Point upstream @opentelemetry/resource-detector-gcp at the resource detector in this repo #518

Open aabmass opened 1 year ago

aabmass commented 1 year ago

There is an existing upstream resource detector for GCP: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/detectors/node/opentelemetry-resource-detector-gcp @opentelemetry/resource-detector-gcp

@opentelemetry/resource-detector-gcp should either

  1. be deprecated
  2. compose/re-export the one in this repo
aabmass commented 1 year ago

triaged and assigned to myself

anuraaga commented 1 year ago

@aabmass Was looking at resource for a NodeJS app and found these two big differences between upstream and this package

The latter is particularly important because the resource transformation logic in this repo relies heavily on CLOUD_PLATFORM being set correctly. So for now, I am using both packages but presumably the CLOUD_PLATFORM logic needs to be upstreamed and then this repo's package could be deprecated

aabmass commented 1 year ago

Yes, the plan is to replace the implementation of @opentelemetry/resource-detector-gcp with the library provided here. I just haven't had time to do it.

  • Upstream populates more k8s information such as K8S_POD_NAME (I couldn't find it in this repo but let me know if I missed it)

@dashpole should we be detecting this from hostname like the upstream detector does, since https://github.com/open-telemetry/oteps/pull/195 fell through the cracks?

dashpole commented 1 year ago

Personally, I don't think k8s_pod_name detection belongs in a GCP detector, as it isn't GCP-specific. That being said, its fine if it stays in the GCP detector if there isn't an alternative k8s detector that detects it.

aabmass commented 1 year ago

I agree it would be nice to push these into generic k8s resource detectors

anuraaga commented 1 year ago

BTW, I noticed this in Go

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/detectors/gcp/gke.go#L31

If the generally accepted approach is to populate k8s information in the manifest through OTEL_RESOURCE_ATTRIBUTES, then the population of k8s information that upstream gcp detector is currently doing isn't actually needed in the long term. Either way, for my app I went with removing use of the upstream one and only use the one from this repo now as I'm setting that env variable anyways for Go apps now.

dashpole commented 1 year ago

xref: https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4136