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

feat: interruptible CPU collector #91

Merged
merged 8 commits into from
Jan 19, 2024
Merged

Conversation

kolesnikovae
Copy link
Contributor

@kolesnikovae kolesnikovae commented Jan 15, 2024

Pyroscope CPU Profiler HTTP Handler

This package facilitates the collection of CPU profiles via HTTP without disrupting the background operation of the Pyroscope profiler. It enables you to seamlessly gather CPU profiles through HTTP while continuously sending them to Pyroscope.

The standard Go pprof HTTP endpoint /debug/pprof/profile returns an error if profiling is already started:

Could not enable CPU profiling: CPU profiling already in use

The Pyroscope CPU Profiler HTTP handler handles this gracefully by communicating with the Pyroscope profiler, which collects profiles in the background.

Usage

The package does not register the handler automatically. It is highly recommended to avoid using the standard path /debug/pprof/profile and the default mux because attempting to register the handler on the same path will cause a panic. In many cases, the net/http/pprof package is imported by dependencies, and therefore there is no reliable way to avoid the conflict.

import (
    "net/http"

    "github.com/grafana/pyroscope-go/http/pprof"
)

func main() {
	http.Handle("/debug/pprof/cpu", pprof.Profile())
}

@korniltsev
Copy link
Collaborator

I got

panic: http: multiple registrations for /debug/pprof/profile

goroutine 1 [running]:
net/http.(*ServeMux).Handle(0x927540, {0x6eccca, 0x14}, {0x764a00?, 0x718420})
        /home/korniltsev/.asdf/installs/golang/1.21.5/go/src/net/http/server.go:2530 +0x219

Maybe we should not register automatically the new endpoint in init function? or maybe the path should be different.

I am thinking that there may be cases when users import net/http/pprof from a transitive dependency, for example dskit. Also users may want to use grafana/http/pprof for cpu and net/http/pprof for heap.

@kolesnikovae
Copy link
Contributor Author

kolesnikovae commented Jan 16, 2024

panic: http: multiple registrations for /debug/pprof/profile

Yeah, this is expected, experimented with this while developing. I'm still testing/fixing it, and I will likely finish today – want to add some unit tests.

Maybe we should not register automatically the new endpoint in init function? or maybe the path should be different.

Agreed, I don't think there is a nice way to register it automatically. I'm not sure about a custom path: use of this handler together with the standard one has little to no value, IMO. Although, that would make sense if we wanted to embed our own UI

@kolesnikovae kolesnikovae force-pushed the feat/interruptible-cpu-collector branch from 191d026 to b6f3ca9 Compare January 16, 2024 13:42
@kolesnikovae kolesnikovae marked this pull request as ready for review January 16, 2024 13:47
Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

LGTM, nice job.

Took me a while to understand, but now that I understand, everything makes sense to me.

As we discussed: another approach could be forking parts of cpu profiler, linking to runtime_pprof_readProfile and we could share cpu data, for example here https://github.com/golang/go/blob/master/src/runtime/pprof/pprof.go?name=release#L851 we could write to multiple pprof builders. We can also never stop cpu profiler in theory. This way it could not interruptible but sharing.

@petethepig petethepig merged commit dc74771 into main Jan 19, 2024
8 checks passed
@petethepig petethepig deleted the feat/interruptible-cpu-collector branch January 19, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants