googleapis / googleapis/google-cloud-go

profiler: add Start function with a context to abort profiling

Open
#8,632 4 comments 0 reactions 1 assignee Claimed by @aabmass View on GitHub
api: cloudprofiler priority: p3 type: feature request
Dominant language
Go
Stars
4.5k
Forks
1.6k
Avg merge
1d 13h
Merged PRs (30d)
109

Description

**Is your feature request related to a problem? Please describe.**

I want to be able to stop the profiling and restart it - possibly with another config even.
However, the current API with `profiler.Start()` doesn't support that.

**Describe the solution you'd like**

Possibly a `ctx` argument for the `Start()` function, however, this is likely not possible due to backwards compatibility, therefore a new function could be added:

```go
// Start starts a goroutine to collect and upload profiles. The
// caller must provide the service string in the config. See
// Config for details. Start should only be called once. Any
// additional calls will be ignored.
func Start(cfg Config, options ...option.ClientOption) error {
startError := startOnce.do(func() error {
return start(context.Background(), cfg, options...)
})
return startError
}

// StartWithContext starts a goroutine to collect and upload profiles. The
// caller must provide the service string in the config. See
// Config for details. StartWithContext should only be called once. Any
// additional calls will be ignored.
// When the context is cancalled the profiling is aborted
func StartWithContext(ctx context.Context, cfg Config, options ...option.ClientOption) error {
startError := startOnce.do(func() error {
return start(ctx, cfg, options...)
})
return startError
}

func start(ctx context.Context, cfg Config, options ...option.ClientOption) error {
...
}
```

I think that [`pollProfilerService`](https://github.com/timofurrer/google-cloud-go/blob/852a230c929a4a2614ecb1c485bcedd1b3f8bcae/profiler/profiler.go#L624-L624) service also needs to change to respect exiting the `for`-loop on context cancellation.

When that is implemented I think the code and test code can be refactored to remove `i < config.numProfiles` and `profilerDone` channel and use a context during tests.

**Describe alternatives you've considered**

- use a `profiler.Config` field with a context - that would avoid adding `StartWithContext`, but is also kinda weird from an API perspective

**Additional context**

I haven't looked at the profiler internals and I'm not sure if it's even possible to simply stop and restart the profiling without leaving any traces of it.

I'm also open to contribute this myself if you are willing to accept the change :)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.