-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
feat!: Enable pprof server on demand (#16594) #17068
Closed
+210
−23
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
2f76bb7
feat: Add an integrated Pprof server enabled on demand
JorTurFer 57f916c
update docs
JorTurFer 1461aa6
Apply feedback
JorTurFer 6238d8c
revert args
JorTurFer 4b139ca
Add documentation about pprof
JorTurFer 85b3a3d
remove var
JorTurFer 87aa849
Apply suggestions from code review
JorTurFer 3c0ee25
rename file
JorTurFer 878f181
Update docs/operator-manual/profiling.md
JorTurFer 3ae08b4
apply feedback
JorTurFer b71bc4d
update comment
JorTurFer e109d52
Update docs/operator-manual/profiling.md
JorTurFer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# Profiling with `pprof` | ||
|
||
[`pprof`](https://go.dev/blog/pprof) is a powerful tool for profiling Go applications. It is part of the Go standard library and provides rich insights into the performance characteristics of Go programs, such as CPU usage, memory allocation, and contention. | ||
|
||
## Basic Usage | ||
|
||
Enable profiling endpoints by setting the environment variable `ARGO_PPROF` with your preferred port. For instance, `ARGO_PPROF=8888` will start profiling endpoints on port `8888`. | ||
|
||
`pprof` has two modes: interactive and non-interactive. Non-interactive mode generates profiling data for future analysis. Interactive mode launches a web server to visualize the profiling data in real-time. | ||
|
||
!!! Note "Port Forward" | ||
[Port forwarding](https://kubernetes.io/docs/tasks/access-application-cluster/port-forward-access-application-cluster/) is a more secure approach to access debug level information than exposing these endpoints via an Ingress. | ||
The below examples assume you have an Argo component forwarded to `http://localhost:6060`, but you can replace that with your preferred local port. | ||
|
||
### Generate CPU Profile | ||
|
||
Generate a CPU profile with the following command: | ||
|
||
```bash | ||
go tool pprof http://localhost:6060/debug/pprof/profile | ||
``` | ||
|
||
### Generate Heap Profiles | ||
|
||
Generate a heap profile with the following command: | ||
|
||
```bash | ||
go tool pprof http://localhost:6060/debug/pprof/heap | ||
``` | ||
|
||
### Interactive Mode | ||
|
||
Use interactive mode with the following command: | ||
|
||
```bash | ||
go tool pprof -http=:8080 http://localhost:6060/debug/pprof/profile | ||
``` | ||
|
||
This starts a web server and opens a browser window displaying the profiling data. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
package pprof | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
"net" | ||
"net/http" | ||
"net/http/pprof" | ||
"os" | ||
"time" | ||
|
||
log "github.com/sirupsen/logrus" | ||
) | ||
|
||
var ( | ||
listener net.Listener | ||
) | ||
|
||
func init() { | ||
addr, exist := os.LookupEnv("ARGO_PPROF") | ||
if !exist { | ||
return | ||
} | ||
listener, _ = net.Listen("tcp", fmt.Sprintf(":%s", addr)) | ||
} | ||
|
||
func IsEnabled() bool { | ||
return listener != nil | ||
} | ||
|
||
type pprofServer struct { | ||
server *http.Server | ||
} | ||
|
||
func NewPprofServer() (*pprofServer, error) { | ||
if listener == nil { | ||
return nil, fmt.Errorf("pprof server is disabled") | ||
} | ||
mux := http.NewServeMux() | ||
srv := &http.Server{ | ||
Handler: mux, | ||
MaxHeaderBytes: 1 << 20, | ||
IdleTimeout: 90 * time.Second, // matches http.DefaultTransport keep-alive timeout | ||
ReadHeaderTimeout: 32 * time.Second, | ||
} | ||
|
||
mux.HandleFunc("/debug/pprof/", pprof.Index) | ||
mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | ||
mux.HandleFunc("/debug/pprof/profile", pprof.Profile) | ||
mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | ||
mux.HandleFunc("/debug/pprof/trace", pprof.Trace) | ||
return &pprofServer{ | ||
server: srv, | ||
}, nil | ||
} | ||
|
||
func (s *pprofServer) Start(ctx context.Context) error { | ||
if listener == nil { | ||
return fmt.Errorf("pprof server is disabled") | ||
} | ||
serverShutdown := make(chan struct{}) | ||
go func() { | ||
<-ctx.Done() | ||
log.Info("shutting down server") | ||
if err := s.server.Shutdown(context.Background()); err != nil { | ||
log.Error(err, "error shutting down server") | ||
} | ||
close(serverShutdown) | ||
}() | ||
|
||
log.Info("Starting pprof server") | ||
if err := s.server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
return err | ||
} | ||
|
||
<-serverShutdown | ||
return nil | ||
} | ||
|
||
func (s *pprofServer) NeedLeaderElection() bool { | ||
return false | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to remove this other code that added handlers previously: #7533
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, I didn't realize that the code was there 🤦
but, does ArgoCD already have support for enabling the profiler using vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, maybe I've added a feature that was already supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I was just responding to someone on Slack about this and found the old PR and your PR and issue. Complete coincidence that I stumbled upon this, especially as I primarily contribute to Workflows.
it seems to be unconditionally enabled on the metrics port(?) if I'm reading correctly. that would be a security issue if so (albeit a minor one, as it's just debug info of an OSS project), and a perf issue too.
also that means the port isn't customizable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, we will replace that with my current approach, right? I mean, I have to remove the old code and just keep my PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the title in particular is used in the changelog and the prefix is typically used for cherry-picking (although CD has a bit different process for that). This might be a
fix!
since it's technically breaking?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the
!
required too in the title?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In conventional commits, the
!
is an indication of a breaking change. This does remove the previously default enabled access to thepprof
endpoints and so is technically breaking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the previous support for
pprof
actually only added it to the Application Controller and Server (some of the components may not have existed at the time; I think only support for the Repo Server was missing at the time of #7533), so this PR also adds it to the rest... which is technically a feature... 😖So maybe this makes sense as a
feat!:
that is still backported due to the security issue? I'll let the CD Maintainers decide how to handle that 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem ,
feat!:
! xD