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-647: Update apiserver tracing to GA for 1.30 #4333

Merged
merged 5 commits into from
Nov 21, 2023
Merged

KEP-647: Update apiserver tracing to GA for 1.30 #4333

merged 5 commits into from
Nov 21, 2023
+26 −15

Conversation

dashpole
Copy link
Contributor

  • The OpenTelemetry library does not need to be wrapped, as we already are using an interface. If we ever need to wrap it, we can simply reimplement the interface.
  • Beta graduation criteria were addressed: Graduate API Server tracing to beta kubernetes#116144.
  • Added GA graduation criteria. Feel free to suggest more.

/sig instrumentation

/assign @logicalhan @dgrisonnet

@k8s-ci-robot k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Nov 10, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 10, 2023
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 10, 2023
logicalhan
logicalhan approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2023
@dashpole
Copy link
Contributor Author

/assign @wojtek-t
Is there a new template this needs to follow?

wojtek-t
wojtek-t reviewed Nov 17, 2023
View reviewed changes
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Just a few smaller comments from PRR perspective.

keps/sig-instrumentation/647-apiserver-tracing/README.md
@@ -207,15 +202,19 @@ Alpha

Beta

- [] Tracing 100% of requests does not break scalability tests (this does not necessarily mean trace backends can handle all the data).
- [X] Tracing 100% of requests does not break scalability tests (this does not necessarily mean trace backends can handle all the data).
Copy link
Member

Choose a reason for hiding this comment

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

Couple smaller comments:

  1. For testing section:
  • can you please add the coverage numbers (as in the template)
  • for integration tests - can you please link to k8s-triage (as described in the template)
  1. for beta criteria:

Tracing 100% of requests does not break scalability tests (this does not necessarily mean trace backends can handle all the data).

For my own education - how did you do that? I think we didn't modify the periodic tests - should we?

  1. SLIs question - are there any plans to have it addressed on OpenTelemetry?

  2. Scalability section - there is a new question to answer:
    https://github.com/kubernetes/enhancements/blob/master/keps/NNNN-kep-template/README.md#can-enabling--using-this-feature-result-in-resource-exhaustion-of-some-node-resources-pids-sockets-inodes-etc

Copy link
Contributor Author

@dashpole dashpole Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It was a one-time test with tracing set at 100%: kubernetes/kubernetes#113695 (comment). It isn't continuous. I don't think we should modify the periodic tests. If we did, it should be at a reasonably low sampling rate to make traces available for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For SLIs, some in OTel are working on proposals: open-telemetry/oteps#238. I don't know when it will land, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed points 1 and 4 in 4500ad8

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Then just one more follow-up on the scalability tests:

  1. Can you maybe link the comment about the passing thread to the KEP itself too
  2. Co you add a sentence that we're consciously not modifying the periodic tests and we will consider sampling at low rate in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 47c37e4

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 17, 2023
@wojtek-t
Copy link
Member

/label tide/merge-method-squash

/lgtm
/approve PRR

@k8s-ci-robot k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 21, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, logicalhan, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9771318 into kubernetes:master Nov 21, 2023
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Nov 21, 2023
@dashpole dashpole deleted the apiserver_tracing_ga branch January 8, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

@logicalhan logicalhan logicalhan approved these changes

@dgrisonnet dgrisonnet Awaiting requested review from dgrisonnet

Assignees

@logicalhan logicalhan

@wojtek-t wojtek-t

@dgrisonnet dgrisonnet

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.
5 participants
@dashpole @wojtek-t @k8s-ci-robot @logicalhan @dgrisonnet