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

pprof: take cpu and memory profiles by setting environment variables #2746

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

jsternberg
Copy link
Collaborator

When run in standalone mode, the environment variables DOCKER_BUILDX_CPU_PROFILE and DOCKER_BUILDX_MEM_PROFILE will cause profiles to be written by the CLI.

}

func setupCPUProfile(ctx context.Context) (stop func()) {
if cpuProfile := os.Getenv("DOCKER_BUILDX_CPU_PROFILE"); cpuProfile != "" {
Copy link
Member

Choose a reason for hiding this comment

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

I think this could just be BUILDX_CPU_PROFILE to be consistent with other envs: https://docs.docker.com/build/building/variables/#build-tool-configuration-variables

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I also added a snippet to capture the log when this is run as a plugin too. It was previously just for standalone runs.

@dvdksn
Copy link
Contributor

dvdksn commented Oct 22, 2024

is this for 0.18.0?

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

There are couple of places in commands package that call os.Exit() directly and would skip these defers. Can be follow-up but needs tracking issue then.

@@ -40,6 +40,9 @@ func init() {
}

func runStandalone(cmd *command.DockerCli) error {
stopProfiles := setupDebugProfiles(context.TODO())
Copy link
Member

Choose a reason for hiding this comment

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

Why not call this from main() directly to avoid duplicate calls.

Copy link
Member

Choose a reason for hiding this comment

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

This can also be a single defer line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just a style thing. I find this easier to read.

@thompson-shaun thompson-shaun modified the milestones: v0.18.0, v0.19.0 Oct 23, 2024
When run in standalone mode, the environment variables
`DOCKER_BUILDX_CPU_PROFILE` and `DOCKER_BUILDX_MEM_PROFILE` will cause
profiles to be written by the CLI.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
@jsternberg
Copy link
Collaborator Author

Refactored the run into a single function that calls runStandalone or runPlugin. The main reason for duplicating that code was because of the calls to os.Exit in the main function. We can tackle that with a future clean up since there are also some other places that I think call os.Exit when they shouldn't.

@crazy-max crazy-max merged commit 658ed58 into docker:master Oct 25, 2024
106 checks passed
@thompson-shaun thompson-shaun modified the milestones: v0.19.0, v0.18.0 Oct 25, 2024
@crazy-max
Copy link
Member

@dvdksn For docs follow-up we could add docker/docs#21214 to contributing docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants