From fd6d4db8c35ad638f9c3ceb55a35836b0ac7f5c5 Mon Sep 17 00:00:00 2001 From: Prasad Shirodkar Date: Thu, 25 Apr 2024 08:11:45 -0700 Subject: [PATCH] Remove context check on stdout exporters (#5189) --- CHANGELOG.md | 2 + exporters/stdout/stdoutmetric/exporter.go | 16 +++--- .../stdout/stdoutmetric/exporter_test.go | 49 ++++++++++++++----- exporters/stdout/stdouttrace/trace.go | 8 ++- exporters/stdout/stdouttrace/trace_test.go | 46 +++++++++-------- 5 files changed, 69 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8eba638ff..aec59d05f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/exporters/stdout/stdoutmetric/exporter.go b/exporters/stdout/stdoutmetric/exporter.go index 96006ab29..fc155d79f 100644 --- a/exporters/stdout/stdoutmetric/exporter.go +++ b/exporters/stdout/stdoutmetric/exporter.go @@ -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) @@ -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{} { diff --git a/exporters/stdout/stdoutmetric/exporter_test.go b/exporters/stdout/stdoutmetric/exporter_test.go index a8c96046d..d7f957e63 100644 --- a/exporters/stdout/stdoutmetric/exporter_test.go +++ b/exporters/stdout/stdoutmetric/exporter_test.go @@ -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) @@ -55,19 +54,27 @@ 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 { - exp, err := stdoutmetric.New(testEncoderOption()) - require.NoError(t, err) - return exp.Shutdown - })) +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("ForceFlush", testCtxErrHonored(func(t *testing.T) func(context.Context) error { - exp, err := stdoutmetric.New(testEncoderOption()) - require.NoError(t, err) - return exp.ForceFlush - })) + 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) @@ -78,6 +85,22 @@ func TestExporterHonorsContextErrors(t *testing.T) { })) } +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 + })) +} + +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 exp.Shutdown + })) +} + func TestShutdownExporterReturnsShutdownErrorOnExport(t *testing.T) { var ( data = new(metricdata.ResourceMetrics) diff --git a/exporters/stdout/stdouttrace/trace.go b/exporters/stdout/stdouttrace/trace.go index dcbd04dd6..b19eb0dfb 100644 --- a/exporters/stdout/stdouttrace/trace.go +++ b/exporters/stdout/stdouttrace/trace.go @@ -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() @@ -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 } diff --git a/exporters/stdout/stdouttrace/trace_test.go b/exporters/stdout/stdouttrace/trace_test.go index d308b13ed..a1fc44e55 100644 --- a/exporters/stdout/stdouttrace/trace_test.go +++ b/exporters/stdout/stdouttrace/trace_test.go @@ -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) + } } } @@ -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() @@ -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) {