Split Instrumentation.Run to Load and Run #1245
Conversation
| @@ -306,6 +308,26 @@ func (m *Manager) Run(ctx context.Context, target *process.TargetDetails) error | |||
| return <-done | |||
| } | |||
|
|
|||
| // Stop stops all probes and cleans up all the resources associated with them. | |||
| func (m *Manager) Stop() error { | |||
There was a problem hiding this comment.
Calling this will not stop Run. Run is waiting for the context to be done. This will clear probes without having that process end.
There was a problem hiding this comment.
Yeah, this is intentional. However I wasn't sure about this approach.
The idea was to be able to clean up when Stop is called between Load and Run (so just to cleanup all the load stuff). After Rum is called the context is used to clean up everything.
How do you think we can improve this? I think it's quite confusing this way.
There was a problem hiding this comment.
I would expect most users would want the Manager to stop if they call this, meaning the Run call should return. Given you have state checking added here, I think we could wrap the passed context to Run and sync the stop methods.
For example, we can add a stop field to the Manager.
--- a/internal/pkg/instrumentation/manager.go
+++ b/internal/pkg/instrumentation/manager.go
@@ -58,6 +58,7 @@ type Manager struct {
cp ConfigProvider
exe *link.Executable
td *process.TargetDetails
+ stop context.CancelCauseFunc
runningProbesWG sync.WaitGroup
telemetryCh chan ptrace.ScopeSpans
currentConfig ConfigAnd then in Run:
@@ -283,6 +284,8 @@ func (m *Manager) Run(ctx context.Context) error {
return errors.New("manager is not loaded, call Load before Run")
}
+ ctx, m.stop = context.WithCancelCause(ctx)
+
for id, p := range m.probes {
if isProbeEnabled(id, m.currentConfig) {
m.runProbe(p)
@@ -299,7 +302,10 @@ func (m *Manager) Run(ctx context.Context) error {
<-ctx.Done()
err := m.Stop()
- done <- errors.Join(err, ctx.Err())
+ if e := context.Cause(ctx); !errors.Is(e, errStop) {
+ err = errors.Join(err, e)
+ }
+ done <- err
}()
for e := range m.telemetryCh {Then we can close this child context when Stop is called:
@@ -308,12 +314,16 @@ func (m *Manager) Run(ctx context.Context) error {
return <-done
}
+var errStop = errors.New("stopped called")
+
// Stop stops all probes and cleans up all the resources associated with them.
func (m *Manager) Stop() error {
if m.state == managerStateUninitialized || m.state == managerStateStopped {
return nil
}
+ m.stop(errStop)There was a problem hiding this comment.
updated, PTAL.
Do you think that after #1248, we can simplify this closing mechanism?
Resolves #1240
Manager.Stopwhich should be called ifInstrumentation.Closeis called in between Load and Run.