Skip to content

Commit

Permalink
Remove context check on stdout exporters (#5189)
Browse files Browse the repository at this point in the history
  • Loading branch information
prasad-shirodkar authored Apr 25, 2024
1 parent 37619ea commit fd6d4db
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 50 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Changed

- `go.opentelemetry.io/otel/exporters/stdout/stdoutlog` won't print timestamps when `WithoutTimestamps` option is set. (#5241)
- The `Shutdown` method of `Exporter` in `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` ignores the context cancellation and always returns `nil`. (#5189)
- The `ForceFlush` and `Shutdown` methods of the exporter returned by `New` in `go.opentelemetry.io/otel/exporters/stdout/stdoutmetric` ignore the context cancellation and always return `nil`. (#5189)

## [1.26.0/0.48.0/0.2.0-alpha] 2024-04-24

Expand Down
16 changes: 6 additions & 10 deletions exporters/stdout/stdoutmetric/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,8 @@ func (e *exporter) Aggregation(k metric.InstrumentKind) metric.Aggregation {
}

func (e *exporter) Export(ctx context.Context, data *metricdata.ResourceMetrics) error {
select {
case <-ctx.Done():
// Don't do anything if the context has already timed out.
return ctx.Err()
default:
// Context is still valid, continue.
if err := ctx.Err(); err != nil {
return err
}
if e.redactTimestamps {
redactTimestamps(data)
Expand All @@ -67,18 +63,18 @@ func (e *exporter) Export(ctx context.Context, data *metricdata.ResourceMetrics)
return e.encVal.Load().(encoderHolder).Encode(data)
}

func (e *exporter) ForceFlush(ctx context.Context) error {
func (e *exporter) ForceFlush(context.Context) error {
// exporter holds no state, nothing to flush.
return ctx.Err()
return nil
}

func (e *exporter) Shutdown(ctx context.Context) error {
func (e *exporter) Shutdown(context.Context) error {
e.shutdownOnce.Do(func() {
e.encVal.Store(encoderHolder{
encoder: shutdownEncoder{},
})
})
return ctx.Err()
return nil
}

func (e *exporter) MarshalLog() interface{} {
Expand Down
45 changes: 34 additions & 11 deletions exporters/stdout/stdoutmetric/exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ func testEncoderOption() stdoutmetric.Option {
func testCtxErrHonored(factory func(*testing.T) func(context.Context) error) func(t *testing.T) {
return func(t *testing.T) {
t.Helper()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
ctx := context.Background()

t.Run("DeadlineExceeded", func(t *testing.T) {
innerCtx, innerCancel := context.WithTimeout(ctx, time.Nanosecond)
Expand All @@ -55,26 +54,50 @@ func testCtxErrHonored(factory func(*testing.T) func(context.Context) error) fun
}
}

func TestExporterHonorsContextErrors(t *testing.T) {
t.Run("Shutdown", testCtxErrHonored(func(t *testing.T) func(context.Context) error {
func testCtxErrIgnored(factory func(*testing.T) func(context.Context) error) func(t *testing.T) {
return func(t *testing.T) {
t.Helper()
ctx := context.Background()

t.Run("Canceled Ignored", func(t *testing.T) {
innerCtx, innerCancel := context.WithCancel(ctx)
innerCancel()

f := factory(t)
assert.NoError(t, f(innerCtx))
})

t.Run("NoError", func(t *testing.T) {
f := factory(t)
assert.NoError(t, f(ctx))
})
}
}

func TestExporterExportHonorsContextErrors(t *testing.T) {
t.Run("Export", testCtxErrHonored(func(t *testing.T) func(context.Context) error {
exp, err := stdoutmetric.New(testEncoderOption())
require.NoError(t, err)
return exp.Shutdown
return func(ctx context.Context) error {
data := new(metricdata.ResourceMetrics)
return exp.Export(ctx, data)
}
}))
}

t.Run("ForceFlush", testCtxErrHonored(func(t *testing.T) func(context.Context) error {
func TestExporterForceFlushIgnoresContextErrors(t *testing.T) {
t.Run("ForceFlush", testCtxErrIgnored(func(t *testing.T) func(context.Context) error {
exp, err := stdoutmetric.New(testEncoderOption())
require.NoError(t, err)
return exp.ForceFlush
}))
}

t.Run("Export", testCtxErrHonored(func(t *testing.T) func(context.Context) error {
func TestExporterShutdownIgnoresContextErrors(t *testing.T) {
t.Run("Shutdown", testCtxErrIgnored(func(t *testing.T) func(context.Context) error {
exp, err := stdoutmetric.New(testEncoderOption())
require.NoError(t, err)
return func(ctx context.Context) error {
data := new(metricdata.ResourceMetrics)
return exp.Export(ctx, data)
}
return exp.Shutdown
}))
}

Expand Down
8 changes: 3 additions & 5 deletions exporters/stdout/stdouttrace/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ type Exporter struct {

// ExportSpans writes spans in json format to stdout.
func (e *Exporter) ExportSpans(ctx context.Context, spans []trace.ReadOnlySpan) error {
if err := ctx.Err(); err != nil {
return err
}
e.stoppedMu.RLock()
stopped := e.stopped
e.stoppedMu.RUnlock()
Expand Down Expand Up @@ -88,11 +91,6 @@ func (e *Exporter) Shutdown(ctx context.Context) error {
e.stopped = true
e.stoppedMu.Unlock()

select {
case <-ctx.Done():
return ctx.Err()
default:
}
return nil
}

Expand Down
46 changes: 22 additions & 24 deletions exporters/stdout/stdouttrace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,30 +60,44 @@ func TestExporterExportSpan(t *testing.T) {
tests := []struct {
opts []stdouttrace.Option
expectNow time.Time
ctx context.Context
wantErr error
}{
{
opts: []stdouttrace.Option{stdouttrace.WithPrettyPrint()},
expectNow: now,
ctx: context.Background(),
},
{
opts: []stdouttrace.Option{stdouttrace.WithPrettyPrint(), stdouttrace.WithoutTimestamps()},
// expectNow is an empty time.Time
ctx: context.Background(),
},
{
opts: []stdouttrace.Option{},
ctx: func() context.Context {
ctx, cancel := context.WithCancel(context.Background())
cancel()
return ctx
}(),
wantErr: context.Canceled,
},
}

ctx := context.Background()
for _, tt := range tests {
// write to buffer for testing
var b bytes.Buffer
ex, err := stdouttrace.New(append(tt.opts, stdouttrace.WithWriter(&b))...)
require.Nil(t, err)

err = ex.ExportSpans(ctx, tracetest.SpanStubs{ss, ss}.Snapshots())
require.Nil(t, err)
err = ex.ExportSpans(tt.ctx, tracetest.SpanStubs{ss, ss}.Snapshots())
assert.Equal(t, tt.wantErr, err)

got := b.String()
wantone := expectedJSON(tt.expectNow)
assert.Equal(t, wantone+wantone, got)
if tt.wantErr == nil {
got := b.String()
wantone := expectedJSON(tt.expectNow)
assert.Equal(t, wantone+wantone, got)
}
}
}

Expand Down Expand Up @@ -181,23 +195,7 @@ func expectedJSON(now time.Time) string {
`
}

func TestExporterShutdownHonorsTimeout(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
defer cancel()

e, err := stdouttrace.New()
if err != nil {
t.Fatalf("failed to create exporter: %v", err)
}

innerCtx, innerCancel := context.WithTimeout(ctx, time.Nanosecond)
defer innerCancel()
<-innerCtx.Done()
err = e.Shutdown(innerCtx)
assert.ErrorIs(t, err, context.DeadlineExceeded)
}

func TestExporterShutdownHonorsCancel(t *testing.T) {
func TestExporterShutdownIgnoresContext(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
defer cancel()

Expand All @@ -209,7 +207,7 @@ func TestExporterShutdownHonorsCancel(t *testing.T) {
innerCtx, innerCancel := context.WithCancel(ctx)
innerCancel()
err = e.Shutdown(innerCtx)
assert.ErrorIs(t, err, context.Canceled)
assert.NoError(t, err)
}

func TestExporterShutdownNoError(t *testing.T) {
Expand Down

0 comments on commit fd6d4db

Please sign in to comment.