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

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

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

damemi
Copy link
Member

@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
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
Member 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

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
2 participants