Skip to content

support GKE resource detection #24

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jul 17, 2020
Merged

support GKE resource detection #24

merged 11 commits into from
Jul 17, 2020

Conversation

AndrewAXue
Copy link
Contributor

Add the ability to detect GKE resources in the Google Cloud Resource Detector and to export this info to Cloud Trace and Cloud Monitoring

@AndrewAXue AndrewAXue requested review from aabmass and removed request for aabmass July 15, 2020 15:31
@AndrewAXue AndrewAXue requested a review from aabmass July 15, 2020 18:01
@AndrewAXue AndrewAXue linked an issue Jul 15, 2020 that may be closed by this pull request
Increase number of GCP tools we can detect #14
Closed
aabmass
aabmass reviewed Jul 17, 2020
View reviewed changes
Copy link
Collaborator

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Awesome! I added some general comments, but I can't comment on the k8s stuff.

opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py Outdated
],
"k8s.namespace.name": pod_namespace,
"host.id": all_metadata["instance"]["id"],
"k8s.pod.name": os.getenv("HOSTNAME", ""),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently GKE supports windows, idk if this will work there, unless k8s injects the envar. You might want platform.node()

Copy link
Contributor

Choose a reason for hiding this comment

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

It's being set by Kubernetes I believe, but it doesn't always 100% match the pod name due to trimming: kubernetes/kubernetes#4825 (comment)

So I think it would be safer to read a separate environment variable by default, e.g. POD_NAME, and possibly fallback to HOSTNAME as a best guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though this behavior does match the OpenCensus implementation: https://github.com/census-ecosystem/opencensus-go-resource/blob/87ca538ec76b3961839940406845ef624d794fa8/gke/gke.go#L48

/cc @rghetia see the issue above about trimming the pod name to 63 characters. Shall we introduce a separate environment variable to be passed via https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-pod-fields-as-values-for-environment-variables ?

@aabmass aabmass requested a review from nilebox July 17, 2020 02:12
@aabmass
Copy link
Collaborator

aabmass commented Jul 17, 2020

@nilebox could you take a look at the kubernetes specific stuff here?

nilebox
nilebox reviewed Jul 17, 2020
View reviewed changes
opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py Outdated
],
"k8s.namespace.name": pod_namespace,
"host.id": all_metadata["instance"]["id"],
"k8s.pod.name": os.getenv("HOSTNAME", ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

It's being set by Kubernetes I believe, but it doesn't always 100% match the pod name due to trimming: kubernetes/kubernetes#4825 (comment)

So I think it would be safer to read a separate environment variable by default, e.g. POD_NAME, and possibly fallback to HOSTNAME as a best guess.

opentelemetry-exporter-cloud-trace/src/opentelemetry/tools/resource_detector.py Outdated
],
"k8s.namespace.name": pod_namespace,
"host.id": all_metadata["instance"]["id"],
"k8s.pod.name": os.getenv("HOSTNAME", ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

Though this behavior does match the OpenCensus implementation: https://github.com/census-ecosystem/opencensus-go-resource/blob/87ca538ec76b3961839940406845ef624d794fa8/gke/gke.go#L48

/cc @rghetia see the issue above about trimming the pod name to 63 characters. Shall we introduce a separate environment variable to be passed via https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-pod-fields-as-values-for-environment-variables ?

aabmass
aabmass approved these changes Jul 17, 2020
View reviewed changes
Copy link
Collaborator

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Looks good! If you addressed all of Nail's comments, please merge 😃

@AndrewAXue AndrewAXue merged commit 756f003 into GoogleCloudPlatform:master Jul 17, 2020
@AndrewAXue AndrewAXue deleted the gke_detector branch July 17, 2020 18:08
@AndrewAXue AndrewAXue mentioned this pull request Jul 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilebox nilebox nilebox left review comments

@aabmass aabmass aabmass approved these changes

@sjbrunst sjbrunst sjbrunst approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase number of GCP tools we can detect
4 participants
@AndrewAXue @aabmass @nilebox @sjbrunst