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

Unify and improve GCP resource detection #2223

Closed
wants to merge 1 commit into from

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Apr 26, 2022

Instead of installing different resource detectors for different GCP platforms, this PR creates a single detector for all platforms. It works by looking for unique environment variables for the platform, and then detecting the rest based on which it finds. The result is a much more consistent detector; it adds the appropriate CloudPlatform attribute for all platforms, and now shares all discovery logic between cloud run and cloud functions. Cloud Functions, Cloud Run, and Google App Engine (new) now all set FaaS resource attributes, instead of service attributes, which more closely aligns with semantic conventions.

This removes support for some kubernetes resource attributes (e.g. k8s.container.name). These don't belong in the GCP resource detection code, and should ideally be set by shared resource detectors (e.g. open-telemetry/oteps#195). For now, users can achieve the same behavior as before with this in their pod spec:

env:
- name: POD_NAME
   valueFrom:
     fieldRef:
       fieldPath: metadata.name
- name: NAMESPACE_NAME
  valueFrom:
    fieldRef:
      fieldPath: metadata.namespace
- name: CONTAINER_NAME
   value: my-container-name
- name: OTEL_RESOURCE_ATTRIBUTES
  value: k8s.pod.name=$(POD_NAME),k8s.namespace.name=$(NAMESPACE_NAME),k8s.container.name=$(CONTAINER_NAME)

I still need to address some outstanding TODOs here, and manually test this in various environments.

@dashpole dashpole force-pushed the update_gcp_resource branch from 37a5022 to f54647e Compare April 26, 2022 03:22
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #2223 (3e51a6e) into main (5883a27) will increase coverage by 0.6%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2223     +/-   ##
=======================================
+ Coverage   70.7%   71.3%   +0.6%     
=======================================
  Files        139     134      -5     
  Lines       6386    6210    -176     
=======================================
- Hits        4517    4433     -84     
+ Misses      1741    1650     -91     
+ Partials     128     127      -1     
Impacted Files Coverage Δ
detectors/gcp/gce.go
detectors/gcp/gke.go
detectors/gcp/version.go

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

Successfully merging this pull request may close these issues.

1 participant