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

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

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Oct 24, 2024

Adds the needed resource/hostname processor that will update the host.name field based on k8s.node.name

Before:
379182423-1dfa46cd-a26d-45c4-b70d-6199e64ffa48

After:
Screenshot 2024-10-24 at 5 12 51 PM

Relates:

Closes: https://github.com/elastic/opentelemetry-dev/issues/516
Found in: https://github.com/elastic/opentelemetry-dev/issues/559

@rogercoll
Copy link
Contributor

@lahsivjar @AlexanderWert We first thought that this would be fixed with the elastictrace processor + the corresponding enrichment: elastic/opentelemetry-lib#108

The issue is that the processor only enriches traces, meaning that we cannot use the processor in metrics/logs OTLP pipeline. The configuration changes used in this PR uses the resource processor to do the mapping for any signal type.

Comment on lines +98 to +102
resource/hostname:
attributes:
- key: host.name
from_attribute: k8s.node.name
action: upsert
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.

@gizas gizas merged commit 038b337 into main Oct 25, 2024
1 check passed
@gizas gizas deleted the otel_hostname_fix branch October 25, 2024 08:07
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.

4 participants