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

K8S Processor: Include more tags #141

Closed
pmm-sumo opened this issue Mar 21, 2020 · 12 comments
Closed

K8S Processor: Include more tags #141

pmm-sumo opened this issue Mar 21, 2020 · 12 comments
Assignees
Labels
processor/k8sattributes k8s Attributes processor
Milestone

Comments

@pmm-sumo
Copy link
Contributor

Currently, the processor supports following tags:

  • namespace
  • podName
  • deployment
  • cluster
  • node
  • startTime

It would be great to include more tags, such as:

  • hostName
  • containerName
  • daemonSetName
  • serviceName
  • statefulSetName

Additionally, it would be good to have an ability to include all labels and annotations

@bogdandrutu
Copy link
Member

I like having them at least as options for the config, not always on :)

mxiamxia referenced this issue in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020
This package is going to allow controlled transition/rename of metrics.
The initial implementation just prepares the way to remove the calls to
the observability package.

In order to be able to rename and change the metrics generated by the Collector without breaking current usage the obsreport package is being introduced. After this is merged a command-line flag can be added to control the generation of the different sets of metrics - initially with the default as legacy only.

Link to tracking Issue: Needed for #141, see also the comments of PR #530

Testing: Besides the added tests I have a series of follow up PRs ready, I used these other PRs to manually validate the changes.

Documentation: Please refer to doc.go in the new package.
mxiamxia referenced this issue in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020
These metrics had an outdated prefix, while at it renamed the metrics attempting to make their meaning more obvious.

Link to tracking Issue: Related to #141

Testing: Manually validated generated metric names
@tigrannajaryan tigrannajaryan added this to the Backlog milestone Jul 30, 2020
@tigrannajaryan tigrannajaryan added the help wanted Extra attention is needed label Jul 30, 2020
@Oberon00
Copy link
Member

Oberon00 commented Oct 8, 2020

@hansh0801
Copy link

I am currently using this processor to map deployment data from istio's sidecar trace data. I would be very happy if it supports statefulset and daemonset other than deployment.

@pmm-sumo
Copy link
Contributor Author

I am currently using this processor to map deployment data from istio's sidecar trace data. I would be very happy if it supports statefulset and daemonset other than deployment.

What we ended up with (at least for now), was having a custom changeset that provides some of this capabilities (and few others) on top of existing processor. It's not the highest priority, but we would like to move some of those changes back to the main version (the scope of the changes was not accepted previously, so we would need to redo it in a more efficient way). But, if that helps, you could try it out: https://github.com/SumoLogic/opentelemetry-collector-contrib/tree/main/processor/k8sprocessor

@hansh0801
Copy link

I am currently using this processor to map deployment data from istio's sidecar trace data. I would be very happy if it supports statefulset and daemonset other than deployment.

What we ended up with (at least for now), was having a custom changeset that provides some of this capabilities (and few others) on top of existing processor. It's not the highest priority, but we would like to move some of those changes back to the main version (the scope of the changes was not accepted previously, so we would need to redo it in a more efficient way). But, if that helps, you could try it out: https://github.com/SumoLogic/opentelemetry-collector-contrib/tree/main/processor/k8sprocessor

Thank you very much ! When will the code (https://github.com/SumoLogic/opentelemetry-collector-contrib/tree/main/processor/k8sprocessor) be upstreamed to otel-contrib main code?

@pmm-sumo
Copy link
Contributor Author

Thank you very much ! When will the code (https://github.com/SumoLogic/opentelemetry-collector-contrib/tree/main/processor/k8sprocessor) be upstreamed to otel-contrib main code?

Thanks @hansh0801 This is a bit complex, you can refer to the original discussion on that for context.

We started working on a reimplementation using a more lightweight approach, but hit several issues, such lack of agreement how to treat tags when there are several containers running in a Pod (this information cannot be really extracted on that level). Or how to handle host.name vs k8s.pod.hostname (i.e. if it's the same thing or a different one).

The plan was to go back to this after GA

@dmitryax dmitryax added the processor/k8sattributes k8s Attributes processor label Aug 4, 2022
@dmitryax
Copy link
Member

dmitryax commented Aug 4, 2022

@povilasv thanks for starting working on the issue in #12957. Do you want me to assign it to you?

@dmitryax
Copy link
Member

dmitryax commented Aug 4, 2022

I would make a correction to the original request.

  • hostName - this seems like beyond k8s attribute processor responsibilities. If user wants that attribute taken from an underlying node, they should install collector as daemonset and use a regular attributes processor with downward k8s API.
  • containerName - this is an identifying attribute of a container within a pod. Using client connection association, we can only get a pod identifier not container. Logs coming from the filelog receiver should already have this attribute taken from a file path.
  • daemonSetName
  • serviceName - I don't think we should fetch this attribute as there is no direct ownership relationships between service and pod. This also increases load on k8s API. We can revisit this later if we really need this attribute.
  • statefulSetName

I suggest to add the following attributes as optional that can be clearly associated with a pod:

  • k8s.replicaset.name and k8s.replicaset.uid
  • k8s.deployment.uid
  • k8s.statefulset.name and k8s.statefulset.uid

We also should probably make k8s.deployment.name optional for consistency and make it more robust as part of #12936

@povilasv
Copy link
Contributor

povilasv commented Aug 4, 2022

@povilasv thanks for starting working on the issue in #12957. Do you want me to assign it to you?

Yes, please. I want to also add DaemonSet, Job and CronJob attributes. What do you think?

@dmitryax
Copy link
Member

dmitryax commented Aug 5, 2022

I want to also add DaemonSet, Job and CronJob attributes

Makes sense. Let's add Job and CronJob as optional attributes as well 👍

@povilasv
Copy link
Contributor

povilasv commented Sep 7, 2022

@dmitryax I think this is done, or should we also implement more reliable way of getting CronJob metadata similiar to #12936?

@dmitryax
Copy link
Member

dmitryax commented Sep 7, 2022

@povilasv I believe functionality-wise this issue can be marked as completed. #12936 can be handled separately. Thanks for your help!

@dmitryax dmitryax closed this as completed Sep 7, 2022
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this issue Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/k8sattributes k8s Attributes processor
Projects
None yet
Development

No branches or pull requests

7 participants