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

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

Open
aabmass opened this issue Feb 2, 2023 · 7 comments
Assignees

Comments

@aabmass
Copy link
Contributor

aabmass commented Feb 2, 2023

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 aabmass self-assigned this Feb 2, 2023
@aabmass aabmass changed the title Expose new resource detector and point @opentelemetry/resource-detector-gcp at it Point upstream @opentelemetry/resource-detector-gcp at the resource detector in this repo Feb 2, 2023
@aabmass
Copy link
Contributor Author

aabmass commented Feb 6, 2023

triaged and assigned to myself

@anuraaga
Copy link
Contributor

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

  • 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)
  • Upstream does not populate CLOUD_PLATFORM

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
Copy link
Contributor Author

aabmass commented Jul 20, 2023

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 open-telemetry/oteps#195 fell through the cracks?

@dashpole
Copy link
Contributor

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
Copy link
Contributor Author

aabmass commented Jul 20, 2023

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

@anuraaga
Copy link
Contributor

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
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants