-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[receiver/kubeletstats] Collect network metrics from all interfaces #38737
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
[receiver/kubeletstats] Collect network metrics from all interfaces #38737
Conversation
cf6d079
to
334c409
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@dmitryax Could you please take a look while other code owners on vacation? |
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.
Thank's for reviving this @Fiery-Fenix and sorry for the delay here. From my perspective I like the idea of having this configurable and not by default enabled so as to ensure that only users that actually need this will enable it.
@TylerHelmuth @jinja2 @povilasv what do you think about this?
Config option sounds good to me. What do we think about renaming the config to |
Renamed, new name sounds really better |
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.
lgtm
dbf2dbf
to
b8c94f5
Compare
Co-authored-by: ChrsMark <[email protected]> Co-authored-by: jinja2 <[email protected]>
Co-authored-by: Christos Markou <[email protected]>
Co-authored-by: Jina Jain <[email protected]>
b8c94f5
to
4f1f56b
Compare
Sorry for massive ping, accidentally rebased on outdated main branch in fork :( |
@jinja2 @TylerHelmuth @dmitryax @povilasv that should be ready for a review now. |
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.
Changes to receiver lgtm, but there are tests failing
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.
LGTM
Description
Revives #34287 by picking it's changes.
Also changed changed approach from feature gate to configuration parameters (disabled by default).
In context of network metrics - default behavior of component hasn't changed, so I consider it as a non-breaking change.
Link to tracking issue
Fixes #30196
Testing
Unit test were updated to cover new functionality
Documentation
Documentation updated to cover new opt-in functionality
Original authors are included in the commit:
Co-authored-by: ChrsMark [email protected]
Co-authored-by: jinja2 [email protected]