[chore] [processor/k8sattributes] improve documentation for the k8sattributes processor #33800
Conversation
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
github-actions
bot
requested review from
dmitryax,
fatsheep9146,
rmfitzpatrick and
TylerHelmuth
| In order to get an association applied, all the sources specified need to match. | ||
|
|
||
| Each sources rule is specified as a pair of `from` (representing the rule type) and `name` (representing the attribute name if `from` is set to `resource_attribute`). | ||
| The following rule types are available: | ||
|
|
||
| - `connection`: Takes the IP attribute from connection context (if available). In this case the processor must appear before any batching or tail sampling, which remove this information. | ||
| - `resource_attribute`: Allows specifying the attribute name to lookup in the list of attributes of the received Resource. Semantic convention should be used for naming. | ||
|
|
||
| Pod association configuration. | ||
| Available attribute names for association sources are the following: |
There was a problem hiding this comment.
please let me know if this list is incomplete or wrong, but I thought it might be worth having this explicit list to avoid confusion regarding which attributes can be used for these rules (e.g. the limitation of container level attributes not working here)
bacherfl
changed the title
bacherfl
changed the title
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
There was a problem hiding this comment.
Thank's for working on this!
I also think that the list at https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/33800/files#diff-57b0d6448d516c0c80b326a7f7f95c3256b2b774e496870dc1dd35dcf8bfc256R101 is a bit hard to interpret and leaves some parts as assumptions?
Could we also revisit this as well?
I'm particularly interested about if the container.id is populated when only the k8s.container.name is provided. If that's the case, then the documentation needs to be fixed. If not, then I think this should be fixed in the implementation since I guess it should be doable to get the container.id as well (at extractPodContainersAttributes) (filing a different issue to track this should be fine).
| - k8s.pod.ip | ||
| - k8s.pod.start_time | ||
| - k8s.pod.uid | ||
| - k8s.replicaset.uid |
There was a problem hiding this comment.
Shouldn't these be either enabled by default or explicitly set by the metadata rules in order to be available in the association? If so, I think it makes sense to mention that.
There was a problem hiding this comment.
good point, I have moved the list to the section about extending the metadata and added a remark that they need to be set there to be available in the association
Thanks for your feedback, @ChrsMark! You're right - I was not 100% sure if I should also revisit this section - After trying out some configurations and debugging I verified that those additional container attributes are indeed only added if the |
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
…tps://github.com/bacherfl/opentelemetry-collector-contrib into chore/32104/improve-k8sattributes-processor-docs
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@ChrsMark when you have some time, can you give this one another review please? |
There was a problem hiding this comment.
The added content looks good to me.
My only concern is if we actually covered the following:
After trying out some configurations and debugging I verified that those additional container attributes are indeed only added if the k8s.container.name is present in the resource attributes, so it is technically correct.
I still think it's a tricky part which either needs to be clearly documented and/or be fixed within the implementation so as to provide the same set of metadata in all cases (if possible).
Thank you for the feedback @ChrsMark! When working on this it seemed to me that this particular point was actually already covered in the original readme in this section: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/33800/files#diff-57b0d6448d516c0c80b326a7f7f95c3256b2b774e496870dc1dd35dcf8bfc256R97-R106 I hope the additional example added in this PR helps to make this a bit clearer, but I agree that this is a bit of a confusing requirement to begin with, so having a closer look at the implementation to see if this can be improved might be a good idea (at least for pods running a single container it should be possible to automatically attach container level attributes) |
|
FYI - I have created an issue to discuss the possibility of falling back to the only container in a single-container pod to relax the requirement of having to provide the |
|
@dmitryax @fatsheep9146 @rmfitzpatrick @TylerHelmuth could one of you please take a look? |
Co-authored-by: Tiffany Hrabusa <30397949+tiffany76@users.noreply.github.com>
Thanks for the suggestions, @tiffany76! I have included them into the PR |
Description: This PR attempts to improve the documentation of the k8sattributes processor. Being relatively new to this component I tried to make the documentation a bit easier to understand for new users, and highlight some limitations I found while going through the implementation.
Link to tracking Issue: #32104
Testing: Trying out different configurations and association rules to get a better understanding
Documentation: Extended the readme of this component