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.

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

  • 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 mentioned this pull request Jul 21, 2021
Open
12 tasks
dashpole
dashpole reviewed Jul 28, 2021
View reviewed changes
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
@sallyom sallyom force-pushed the tracing-kubelet-kep branch 3 times, most recently from 6013bf0 to ed06666 Compare August 5, 2021 03:00
@dashpole
Copy link
Contributor

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

@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.

keps/sig-instrumentation/2831-kubelet-tracing/kep.yaml Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/kep.yaml Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
@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

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

keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
@sallyom
Copy link
Contributor Author

sallyom commented Oct 10, 2021

@dashpole @ehashman ptal, thanks

@ehashman ehashman added this to Needs Reviewer in SIG Node PR Triage Dec 6, 2021
@sallyom
Copy link
Contributor Author

sallyom commented Jan 20, 2022

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

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 :)

keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
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

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.

keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
@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.

keps/sig-instrumentation/2831-kubelet-tracing/README.md Outdated Show resolved Hide resolved
@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 @husky-parul
kubelet tracing
34e9f59 
Signed-off-by: Sally O'Malley <somalley@redhat.com>

Co-authored-by: husky-parul <parsingh@redhat.com>
@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
SIG Node PR Triage automation moved this from Needs Reviewer to Done 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 PR Triage
Done
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@sallyom @dashpole @ehashman @k8s-ci-robot @logicalhan @derekwaynecarr @lengrongfu @ddebroy