-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[chore] use metadata.yaml in k8sattributes processor #21399
[chore] use metadata.yaml in k8sattributes processor #21399
Conversation
cad91d4
to
1fa9aa8
Compare
| Stability | [beta] | | ||
| Supported pipeline types | logs, metrics, traces | | ||
| Distributions | [contrib] | | ||
| Warnings | [Statefulness](#warnings) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this link going anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but at least now the statefulness warning is showing. I am not sure what I'd enter in the warnings section.
Do you want an issue to follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the right place to send users would be https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/standard-warnings.md#statefulness, does that make sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, how do you want to go about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in, are we making mdatagen output the right URL, or should we add a link in a warnings section that we create specifically for this component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this processor, is it actually stateful?
Depends on what we call stateful. This processor keeps k8s API state, not any pdata state. If we take definition from https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/cumulativetodeltaprocessor#warnings, it's not stateful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it is not stateful; the deployment architecture of the collector is not affected by the k8sattributes processor (passthrough doesn't count because you don't have to use it). It may keep extra information in memory, but the term Stateful implies you need to send all the related telemetry to the same collector instance, and that is not true for k8sattributes.
I think it could be fair to add a warning about memory consumption. Since the processor has to remember the metadata of the node it is on, it consumes more memory than other processors. That consumption is compounded if you don't filter down to only the metadata for the node it is on. I think those are fair warnings to call out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great call out! I agree with the additional memory warning instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this needs to be kicked off to its own issue and separate PR. I can remove the warnings for now and get this in and we can hammer the wording separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to remove the stateful warning and I have filed #21789 to follow up.
1fa9aa8
to
3df32e9
Compare
3df32e9
to
b1176ed
Compare
See #21213