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

📖 Add new documentation explaining the usage of Pprof #4160

Merged
merged 1 commit into from
Sep 15, 2024

Conversation

TAM360
Copy link
Contributor

@TAM360 TAM360 commented Sep 12, 2024

Description

This PR introduces a new documentation file explaining the purpose, and usage of pprof library within the Kubebuilder project.

Motivation

Pprof is a profiling library designed for retrieving and visualization profiling statistics such as CPU, and Memory usage of Go application. With Pprof, you can identify, and fix the bottlenecks that are hampering the performance of your Controller.
Pprof comes as part of controller-runtime package since v0.15.x which is used by Kubebuilder under the hood.

Having an introductory tutorial will help any existing or future user of Kubebuilder in about analyzing their own codebase's behavior, without relying on any external resources.

Issues fixed

3338

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 12, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @TAM360. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@TAM360 TAM360 closed this Sep 12, 2024
@TAM360 TAM360 reopened this Sep 13, 2024
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Great work 🥇 Well done 🚀

I added some comments /suggestions in my review !!!

PS.: See that in this case we do not use in the title go/v4 because the change is not in the source code of the plugin. It is only for docs. The title is what we use to generate the release notes, in case of doc changes it will not end up in the summary, just in all changes done in the release, see for example: https://github.com/kubernetes-sigs/kubebuilder/releases/tag/v4.2.0

@TAM360 TAM360 changed the title 📖 (go/v4): Add new documentation explaining the usage of Pprof 📖 Add new documentation explaining the usage of Pprof Sep 13, 2024
@TAM360 TAM360 force-pushed the feat/pprof-docs branch 4 times, most recently from 9d134c1 to c4eed09 Compare September 13, 2024 17:11
@TAM360 TAM360 force-pushed the feat/pprof-docs branch 4 times, most recently from 1bc9803 to 7a2e873 Compare September 15, 2024 02:45
@camilamacedo86
Copy link
Member

/ok-to-test
/approved

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 15, 2024
@camilamacedo86
Copy link
Member

Hi @zqzten

You asked for this one and is the author of the implementation in C+R.
You can check the final result in : https://deploy-preview-4160--kubebuilder.netlify.app/reference/pprof-tutorial
WDYT?

@camilamacedo86
Copy link
Member

/approved

Copy link
Member

@zqzten zqzten left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

I have some opinions on the Pprof Not Recommended for Production section, as pprof would only introduce overhead on actual CPU sampling (by calling the API) and can be useful for debugging performance issues in production (sometimes performance issue cannot be easily reproduced in test env or after restarting process to enable pprof in production), so the tip might be Use Pprof in Production with Caution to instruct that:

  • Pprof endpoint should be protected in production for security reasons.
  • Pprof would introduce overhead on CPU sampling (when calling the API).

@TAM360
Copy link
Contributor Author

TAM360 commented Sep 15, 2024

Thanks for the work!

I have some opinions on the Pprof Not Recommended for Production section, as pprof would only introduce overhead on actual CPU sampling (by calling the API) and can be useful for debugging performance issues in production (sometimes performance issue cannot be easily reproduced in test env or after restarting process to enable pprof in production), so the tip might be Use Pprof in Production with Caution to instruct that:

  • Pprof endpoint should be protected in production for security reasons.
  • Pprof would introduce overhead on CPU sampling (when calling the API).

@camilamacedo86 WDYT?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Sep 15, 2024

Hi @zqzten,

Regarding: PR Review 4160

be useful for debugging performance issues in production (sometimes performance issue cannot be easily reproduced in test env or after restarting process to enable pprof in production),

I believe we should be clear in our messaging here. If we say "not recommended" (as we use it in other places like for metrics) and provide clear reasoning, users will understand that this feature should not be enabled by default. However, if we use language like "use with caution," it could lead to confusion and encourage users to deliver solutions with it enabled by default, which could lead to unintended consequences.

It’s analogous to troubleshooting in an environment. If a scenario cannot be reproduced in one environment, you naturally move to an environment where it can be reproduced, ensuring that you’re working under the right conditions. In other words, not matter we say that is NOT recommended for Prod, if the person need to test things out in Prod for any reason this person will do that.

as pprof would only introduce overhead on actual CPU sampling (by calling the API)

I think we can rewording it to clarify that is mainly CPU . I will add a suggestion and ping you.

Is that make sense ?
However, if you have other suggestions to rewording please feel free.

Copy link
Member

@zqzten zqzten left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2024
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

All conveyed suggestions with the author of the issue are addressed
So, it seems that all is done and we can get this one merged !!!

/lgtm
/approved
/ok-to-test

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, TAM360, zqzten

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 Sep 15, 2024
@k8s-ci-robot k8s-ci-robot merged commit 662f78f into kubernetes-sigs:master Sep 15, 2024
18 checks passed
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants