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.

Sign up for GitHub

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

Jump to bottom

Shut down all processors even on error #3091

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Shut down all processors even on error #3091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e327663
Shut down all processors even on error
ash2k Aug 13, 2022
7dedf49
Merge branch 'main' into always-shut-down-all-processors
MadVikingGod Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions sdk/trace/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,10 @@ var _ trace.TracerProvider = &TracerProvider{}
// NewTracerProvider returns a new and configured TracerProvider.
//
// By default the returned TracerProvider is configured with:
// - a ParentBased(AlwaysSample) Sampler
// - a random number IDGenerator
// - the resource.Default() Resource
// - the default SpanLimits.
// - a ParentBased(AlwaysSample) Sampler
// - a random number IDGenerator
// - the resource.Default() Resource
// - the default SpanLimits.
//
// The passed opts are used to override these default values and configure the
// returned TracerProvider appropriately.
Expand Down Expand Up @@ -241,10 +241,7 @@ func (p *TracerProvider) Shutdown(ctx context.Context) error {
if !ok {
return fmt.Errorf("failed to load span processors")
}
if len(spss) == 0 {
ash2k marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

var retErr error
for _, sps := range spss {
select {
case <-ctx.Done():
Expand All @@ -257,10 +254,15 @@ func (p *TracerProvider) Shutdown(ctx context.Context) error {
err = sps.sp.Shutdown(ctx)
})
if err != nil {
return err
if retErr == nil {
retErr = err
} else {
// Poor man's list of errors
retErr = fmt.Errorf("%v; %v", retErr, err)
}
Comment on lines +264 to +269
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if retErr == nil {
retErr = err
} else {
// Poor man's list of errors
retErr = fmt.Errorf("%v; %v", retErr, err)
}
if retErr != nil {
// Poor man's list of errors
retErr = fmt.Errorf("%v; %v", retErr, err)
}
retErr = err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work the way it should, does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ash2k's version looks correct to me.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if retErr == nil {
retErr = err
} else {
// Poor man's list of errors
retErr = fmt.Errorf("%v; %v", retErr, err)
}
if retErr != nil {
// Poor man's list of errors
err = fmt.Errorf("%w; %w", retErr, err)
}
retErr = err

This might work, but the original also looks correct. Any reason to not use multierr here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Aneurysm9 If you are talking about https://pkg.go.dev/go.uber.org/multierr, it's not a dependency at the moment and it doesn't feel worth adding it just for this case. In 99% of cases there will be no errors here and it's probably very unlikely that there will be more than one. In this PR I just wanted to ensure all processors are shut down even if there is an error. If there is a better way to handle multiple errors that is discovered later, this code can always be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Aneurysm9 are you satisfied with this answer, or are there further changes you want to request?

}
}
return nil
return retErr
}

// TracerProviderOption configures a TracerProvider.
Expand Down
22 changes: 22 additions & 0 deletions sdk/trace/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,28 @@ func TestFailedProcessorShutdown(t *testing.T) {
assert.Equal(t, err, spErr)
}

func TestFailedProcessorsShutdown(t *testing.T) {
stp := NewTracerProvider()
spErr1 := errors.New("basic span processor shutdown failure1")
spErr2 := errors.New("basic span processor shutdown failure2")
sp1 := &basicSpanProcesor{
running: true,
injectShutdownError: spErr1,
}
sp2 := &basicSpanProcesor{
running: true,
injectShutdownError: spErr2,
}
stp.RegisterSpanProcessor(sp1)
stp.RegisterSpanProcessor(sp2)

err := stp.Shutdown(context.Background())
assert.Error(t, err)
assert.EqualError(t, err, "basic span processor shutdown failure1; basic span processor shutdown failure2")
assert.False(t, sp1.running)
assert.False(t, sp2.running)
}

func TestFailedProcessorShutdownInUnregister(t *testing.T) {
handler.Reset()
stp := NewTracerProvider()
Expand Down