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.

Sign up for GitHub

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

Jump to bottom

KEP-2831: Instrumenting Kubelet for OpenTelemetry Tracing #2832

Merged
merged 2 commits into from
Feb 3, 2022
Merged

KEP-2831: Instrumenting Kubelet for OpenTelemetry Tracing #2832

merged 2 commits into from
Feb 3, 2022

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented Jul 21, 2021 •
edited
Loading

  • One-line PR description: Instrument kubelet to generate and export OpenTelemetry trace data.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 21, 2021
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from ed71587 to 7142e9a Compare July 21, 2021 03:44
@sallyom sallyom mentioned this pull request Jul 21, 2021
Open
12 tasks
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 7142e9a to 1bf3d00 Compare July 21, 2021 15:38
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 1bf3d00 to ccfca15 Compare July 21, 2021 19:23
dashpole
dashpole reviewed Jul 28, 2021
View reviewed changes
@sallyom sallyom force-pushed the tracing-kubelet-kep branch 3 times, most recently from 6013bf0 to ed06666 Compare August 5, 2021 03:00
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from ed06666 to c6f127e Compare August 5, 2021 19:10
@dashpole
Copy link
Contributor

@sallyom let me know when comments are addressed and you are ready for another round of review

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from c6f127e to 6d03584 Compare August 16, 2021 17:09
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 6d03584 to 44e50f9 Compare September 8, 2021 16:38
@ehashman
Copy link
Member

ehashman commented Sep 8, 2021

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 8, 2021
ehashman
ehashman reviewed Sep 8, 2021
View reviewed changes
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick pass, I don't think this will make it in time for 1.23.

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 44e50f9 to c165872 Compare September 9, 2021 15:44
@sallyom
Copy link
Contributor Author

sallyom commented Sep 9, 2021

@ehashman @dashpole thanks for your reviews. bummer, this won't make 1.23 but i'll keep on this to push for merge and kubelet changes with 1.24

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from c165872 to 5885e52 Compare September 9, 2021 15:48
dashpole
dashpole reviewed Sep 9, 2021
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 5885e52 to bca704f Compare September 10, 2021 16:54
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from bca704f to f71b8d4 Compare October 10, 2021 01:03
@sallyom
Copy link
Contributor Author

sallyom commented Oct 10, 2021

@dashpole @ehashman ptal, thanks

dashpole
dashpole approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
This looks great. We should make sure we get feedback from sig-node members as well.

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 8eee33b to 868a441 Compare January 20, 2022 15:29
@sallyom
Copy link
Contributor Author

sallyom commented Jan 20, 2022

@ehashman ptal at the prod-readiness review for 1.24 freeze, thanks again :)

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 868a441 to f61a344 Compare January 20, 2022 16:32
ehashman
ehashman reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRR is nearly done, just a couple of small questions. Need node and @dashpole review as well :)

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from f61a344 to 5cca408 Compare January 25, 2022 18:03
ehashman
ehashman reviewed Jan 26, 2022
View reviewed changes
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

for PRR. Leaving final LGTM to @dashpole

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2022
ehashman
ehashman reviewed Jan 26, 2022
View reviewed changes
keps/sig-instrumentation/2831-kubelet-tracing/kep.yaml
owning-sig: sig-instrumentation
participating-sigs:
- sig-architecture
- sig-node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIG Node acked the KEP at the 2022-01-25 meeting.

@dashpole
Copy link
Contributor

Once the comment about passthrough for trace context above is resolved, i'll lgtm

ddebroy
ddebroy reviewed Feb 1, 2022
View reviewed changes
Copy link
Member

@ddebroy ddebroy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be great if we can call out support for tracing volume attach+mount related operations between Kubelet and the in-tree volume plugins + CSI node plugins.

Would also love to know if tracing from the perspective of individual PVs/Volumes across Kubelet/Attach-Detach controller and PV controller exists or is called out somewhere.

@sallyom sallyom force-pushed the tracing-kubelet-kep branch 2 times, most recently from 51938bc to 63f8691 Compare February 1, 2022 21:47
derekwaynecarr
derekwaynecarr approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kep generally lgtm for sig-node.

defer to @dashpole for final review who has good understanding of kubelet as well.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, derekwaynecarr, ehashman, sallyom

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sallyom sallyom force-pushed the tracing-kubelet-kep branch from 63f8691 to b465d40 Compare February 3, 2022 00:33
@sallyom @husky-parul
kubelet tracing
34e9f59 
Signed-off-by: Sally O'Malley <somalley@redhat.com>

Co-authored-by: husky-parul <parsingh@redhat.com>
@sallyom sallyom force-pushed the tracing-kubelet-kep branch from b465d40 to 34e9f59 Compare February 3, 2022 00:41
@sallyom @dashpole
Update keps/sig-instrumentation/2831-kubelet-tracing/README.md
2ef3e9a 
Co-authored-by: David Ashpole <dashpole@google.com>
@dashpole
Copy link
Contributor

dashpole commented Feb 3, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7e55759 into kubernetes:master Feb 3, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Feb 3, 2022
@sallyom sallyom mentioned this pull request Jul 18, 2022
Merged
@swagatbora90 swagatbora90 mentioned this pull request Sep 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logicalhan logicalhan logicalhan left review comments

@dashpole dashpole dashpole approved these changes

@ehashman ehashman ehashman left review comments

@lengrongfu lengrongfu lengrongfu left review comments

@ddebroy ddebroy ddebroy left review comments

@derekwaynecarr derekwaynecarr derekwaynecarr approved these changes

Assignees

@dashpole dashpole

@ehashman ehashman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.
8 participants
@sallyom @dashpole @ehashman @k8s-ci-robot @logicalhan @derekwaynecarr @lengrongfu @ddebroy