Skip to content

Expand scope of kubernetes monitored resource mapping to include other platforms#683

Merged
damemi merged 3 commits intoGoogleCloudPlatform:mainfrom
damemi:issue-627
Jul 26, 2023
Merged

Expand scope of kubernetes monitored resource mapping to include other platforms#683
damemi merged 3 commits intoGoogleCloudPlatform:mainfrom
damemi:issue-627

Conversation

@damemi
Copy link
Copy Markdown
Contributor

@damemi damemi commented Jul 21, 2023

Fixes #627
This broadens the exporter's monitored resource mapping to match anything with k8s.cluster.name resource attribute to a k8s monitored resource. Using k8s.cluster.name means that any cloud provider k8s platforms like GKE, EKS, or AKS should be identified, as well as non-cloud clusters like local k8s or minikube.

This is a change from the previous behavior, in which any non-GKE kubernetes resources were mapped to generic_node.

// Otherwise, it returns the cloud.platform value.
func getK8sClusterOrCloudPlatform(attrs ReadOnlyAttributes) string {
if _, cluster := attrs.GetString(string(semconv.K8SClusterNameKey)); cluster {
return k8sCluster
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: kind of strange to mix platform semantic conventions and other strings. Maybe return semconv.CloudPlatformGCPKubernetesEngine.Value.AsString() here instead?

Copy link
Copy Markdown
Contributor Author

@damemi damemi Jul 24, 2023

Choose a reason for hiding this comment

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

Yeah, I thought about that too. I don't think it's perfect either way (felt weird to return the platform as GCP when it could be any k8s).

It's odd to me that there isn't an attribute to indicate generic K8S platform. The best you can do, as far as I can tell, is just infer if k8s.cluster.name is set. And it's not practical to enumerate every k8s-based cloud platform

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 24, 2023

Codecov Report

Merging #683 (c129d54) into main (c3147fc) will decrease coverage by 0.17%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
- Coverage   69.31%   69.14%   -0.17%     
==========================================
  Files          36       36              
  Lines        4728     4728              
==========================================
- Hits         3277     3269       -8     
- Misses       1297     1305       +8     
  Partials      154      154              

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

k8s_container monitored resource type requires cloud.platform: gcp_kubernetes_engine even when running outside of Google Cloud

2 participants