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

KEP-2831: Instrumenting Kubelet for OpenTelemetry Tracing #2832

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented Jul 21, 2021

  • 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
12 tasks
@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
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/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

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

@sallyom
Copy link
Contributor Author

sallyom commented Jan 20, 2022

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

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

Copy link
Member

@ddebroy ddebroy left a comment

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

Signed-off-by: Sally O'Malley <somalley@redhat.com>

Co-authored-by: husky-parul <parsingh@redhat.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants