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

Include k8s.pod.ip attribute in k8sattributes preset #1286

Closed
wants to merge 1 commit into from

Conversation

ChrsMark
Copy link
Member

With the addition at open-telemetry/opentelemetry-collector-contrib#33440 to provide the option to explicitly export the k8s.pod.ip (in case it's not already added) I think it makes sense to enable this in the preset as well.

@ChrsMark ChrsMark requested a review from a team July 31, 2024 12:35
@@ -1,6 +1,6 @@
apiVersion: v2
name: opentelemetry-collector
version: 0.100.0
version: 0.101.0
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be bumping the patch version i.e. 0.100.1 here :)

@@ -218,6 +218,7 @@ processors:
- "k8s.node.name"
- "k8s.pod.name"
- "k8s.pod.uid"
- "k8s.pod.ip"
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to hear more about why this should be enabled by default. I'm not sure everyone would want the IP recorded on their telemetry.

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea here is mostly to achieve consistency. The preset defines the following pod_associations:

pod_association:
- sources:
  - from: resource_attribute
    name: k8s.pod.ip
- sources:
  - from: resource_attribute
    name: k8s.pod.uid
- sources:
  - from: connection

So this implies that some incoming data already have the k8s.pod.ip attribute based on which the metadata association is performed. The output of this processor still has the k8s.pod.ip obviously.
However, since we have 2 other pod_assocation rules, incoming data without a pre-existing k8s.pod.ip attribute will be enriched based on the k8s.pod.uid, for instance, and come out of the processor but this time without the k8s.pod.ip because this is not added explicitly by the processor.

This is in principle correct, because this is what we instruct the processor to do through the defined configuration, however users that just enable the preset without really diving into the details of its underlying (advanced :)) configuration might find this tricky to be explained and even complain about this being buggy. We have some relevant reports in Collector's contrib repository about the processor not being so clear what metadata it actually adds and I have spotted this k8s.pod.ip question specifically in some private conversations.

Consequently, my only intention was to avoid this kind of confusion and make the processor to always add the k8s.pod.ip if that's not already there so as all output data of the processor to be consistent.
I don't have any strong preference here though, so if there are reasons for not adding this I'm just fine :).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think k8s.pod.ip should be opt-in, one can use k8s attributes processor in many ways, for example enriching metrics coming from k8scluster receiver, then this would add new high cardinality attribute to the metric backends, which might be not even needed 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

That make sense @povilasv. I agree this could potentially bring more trouble than convenience. I'm gonna close this PR then. Thank's for the feedback @TylerHelmuth @povilasv!

@ChrsMark ChrsMark force-pushed the add_pod_ip_attribute branch from 72b679d to b3d3a19 Compare August 1, 2024 07:30
@ChrsMark ChrsMark force-pushed the add_pod_ip_attribute branch from b3d3a19 to 36aa355 Compare August 1, 2024 07:55
@ChrsMark ChrsMark closed this Aug 1, 2024
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