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.

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

Adding fix for host.name field update based on k8s.node.name #37

Merged
merged 1 commit into from
Oct 25, 2024
+18 −0
Merged

Adding fix for host.name field update based on k8s.node.name #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions resources/kubernetes/operator/helm/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ collectors:
action: insert
- key: app.label.version
action: delete
resource/hostname:
attributes:
- key: host.name
from_attribute: k8s.node.name
action: upsert
Comment on lines +98 to +102
Copy link
Member

Choose a reason for hiding this comment

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

Is this something the k8sattributes should do by default? If so, can we propose these changes upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

That could be an option, similar to the override functionality in the resource detector processor (e.g. overrides the host.name value with the cloud instance ID). But I have two concerns:

  • Upstream agreement with host.name == k8s.node.name: looks like a custom workaround to fix the Infrastructure UI relationships. From the container perspective, it has its own host.name value, which is actually used among the cluster for communication.
  • Otlp data that already contains k8s resource attributes that won't be processed by the k8sattributes processor.

Copy link
Member

Choose a reason for hiding this comment

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

Upstream agreement with host.name == k8s.node.name: looks like a custom workaround to fix the Infrastructure UI relationships. From the container perspective, it has its own host.name value, which is actually used among the cluster for communication.

IMHO, that is a symptom of host.name in SemConv being underspecified. In particular, there's no clear statement about the semantics of the host.name in containerized environments.
In particular it says ... On Unix systems, it may contain what the hostname command returns, or the fully qualified hostname .... For containerized system that means that SDKs will report that value which from the perspective from within a container is per default the pod name. I'm not sure if that behavior is really intended by SemConv or just not thought through well enough. The consequence is, that host.name is not a reliable attribute with that. So I don't think it's an Elastic-specific challenge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we need to understand all scenarios of the host.name population per different k8s setup. Created internal issue to summarise all the above

Copy link
Member

Choose a reason for hiding this comment

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

The consequence is, that host.name is not a reliable attribute with that. So I don't think it's an Elastic-specific challenge.

I agree. That's why I'm skeptical that we should fix it with Elastic-specific configuration. Maybe we should embrace that the data could be messy and adjust the UI accordingly. For example, exclude metrics in the hosts view that have a kubernetes.pod.uid.

I'm fine with this as a short-term workaround but I feel like we're adding a lot of these fixes to the configuration that aren't or shouldn't be Elastic-specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should embrace that the data could be messy and adjust the UI accordingly. For example, exclude metrics in the hosts view that have a kubernetes.pod.uid.

+1 Or as @lahsivjar was suggesting, change the UI logic to rely on k8s.node.name and then fallback to host.name.

k8sattributes:
passthrough: false
pod_association:
Expand Down Expand Up @@ -161,6 +166,7 @@ collectors:
- resourcedetection/gcp
- resourcedetection/aks
- resource/k8s
- resource/hostname
receivers:
- k8s_cluster
logs:
Expand All @@ -170,6 +176,7 @@ collectors:
- resourcedetection/eks
- resourcedetection/gcp
- resourcedetection/aks
- resource/hostname
exporters:
- debug
- elasticsearch/otel
Expand Down Expand Up @@ -504,6 +511,11 @@ collectors:
action: insert
- key: app.label.version
action: delete
resource/hostname:
attributes:
- key: host.name
from_attribute: k8s.node.name
action: upsert
attributes/dataset:
actions:
- key: event.dataset
Expand Down Expand Up @@ -727,6 +739,7 @@ collectors:
- resourcedetection/gcp
- resourcedetection/aks
- resource/k8s
- resource/hostname
- resource/cloud
exporters:
- debug
Expand All @@ -742,6 +755,7 @@ collectors:
- resourcedetection/gcp
- resourcedetection/aks
- resource/k8s
- resource/hostname
- resource/cloud
exporters:
- debug
Expand All @@ -759,6 +773,7 @@ collectors:
- resourcedetection/gcp
- resourcedetection/aks
- resource/k8s
- resource/hostname
- resource/cloud
- attributes/dataset
- resource/process
Expand All @@ -770,6 +785,7 @@ collectors:
- otlp
processors:
- batch
- resource/hostname
exporters:
- debug
- signaltometrics
Expand All @@ -779,6 +795,7 @@ collectors:
- otlp
processors:
- batch
- resource/hostname
exporters:
- debug
- signaltometrics
Expand All @@ -789,6 +806,7 @@ collectors:
processors:
- batch
- elastictrace
- resource/hostname
exporters:
- debug
- signaltometrics
Expand Down