-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP-2831: update kubelet tracing KEP to target GA in 1.33 #5134
KEP-2831: update kubelet tracing KEP to target GA in 1.33 #5134
Conversation
329a3f0
to
f2dbbce
Compare
/assign @wojtek-t |
Sorry for the late review request @wojtek-t |
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.
/cc
please @dashpole make sure to use the latest KEP template. The template is periodically updated and if the lastest update of the KEP was more than a couple cycles old there can be some changes to be made
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.
Some very minor comments but don't want them to slip through the cracks:
Some sections in the PRR Questionaire are still marked as TBD (e.g, Are there any missing metrics that would be useful to have to improve observability ).
Also, I see there are plans to add metrics but metric section is TBD in kep.yaml.
@swatisehgal @ffromani I updated the KEP to use the new template. We are blocked adding metrics by work in upstream OpenTelemetry. But traces are themselves an observability signal, and can be used to see if the feature is working properly. I don't think metrics should block the GA of this feature |
80404ee
to
810ca4b
Compare
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.
Two minor comments - other than that LGTM.
But please work on having SIG approval first.
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.
pretty much LGTM from sig-node, few thoughts inline
810ca4b
to
51ee9a7
Compare
/lgtm |
LGTM also from sig-node. I think pending open points are on PRR side. |
51ee9a7
to
446aa3e
Compare
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
One-line PR description: updating stable target for 1.33
Issue link: Kubelet OpenTelemetry Tracing #2831