From b6cbfe3e50e4400bd65dfee1204d289d1fdce848 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Mon, 5 Dec 2022 11:48:03 -0800 Subject: [PATCH 1/5] OTLP traces export errors use a consistent error message prefix --- exporters/otlp/otlptrace/exporter.go | 7 +- exporters/otlp/otlptrace/exporter_test.go | 66 +++++++++++++++++++ .../otlptrace/otlptracegrpc/client_test.go | 5 +- .../otlp/otlptrace/otlptracehttp/client.go | 2 +- .../otlptrace/otlptracehttp/client_test.go | 6 +- 5 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 exporters/otlp/otlptrace/exporter_test.go diff --git a/exporters/otlp/otlptrace/exporter.go b/exporters/otlp/otlptrace/exporter.go index c5ee6c098cc..0b4ea1cde96 100644 --- a/exporters/otlp/otlptrace/exporter.go +++ b/exporters/otlp/otlptrace/exporter.go @@ -17,6 +17,7 @@ package otlptrace // import "go.opentelemetry.io/otel/exporters/otlp/otlptrace" import ( "context" "errors" + "fmt" "sync" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/tracetransform" @@ -45,7 +46,11 @@ func (e *Exporter) ExportSpans(ctx context.Context, ss []tracesdk.ReadOnlySpan) return nil } - return e.client.UploadTraces(ctx, protoSpans) + err := e.client.UploadTraces(ctx, protoSpans) + if err != nil { + return fmt.Errorf("trace export: %w", err) + } + return nil } // Start establishes a connection to the receiving endpoint. diff --git a/exporters/otlp/otlptrace/exporter_test.go b/exporters/otlp/otlptrace/exporter_test.go new file mode 100644 index 00000000000..27eec2637c3 --- /dev/null +++ b/exporters/otlp/otlptrace/exporter_test.go @@ -0,0 +1,66 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package otlptrace_test + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/exporters/otlp/otlptrace" + tracesdk "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + tracepb "go.opentelemetry.io/proto/otlp/trace/v1" +) + +type client struct { + uploadErr error +} + +var _ otlptrace.Client = &client{} + +func (c *client) Start(ctx context.Context) error { + return nil +} + +func (c *client) Stop(ctx context.Context) error { + return nil +} + +func (c *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.ResourceSpans) error { + return c.uploadErr +} + +type span struct { + tracesdk.ReadOnlySpan +} + +func TestExporterClientError(t *testing.T) { + ctx := context.Background() + exp, err := otlptrace.New(ctx, &client{ + uploadErr: context.Canceled, + }) + assert.NoError(t, err) + + spans := tracetest.SpanStubs{{Name: "Span 0"}}.Snapshots() + err = exp.ExportSpans(ctx, spans) + + assert.Error(t, err) + assert.True(t, errors.Is(err, context.Canceled)) + assert.Contains(t, err.Error(), "trace export") + + assert.NoError(t, exp.Shutdown(ctx)) +} diff --git a/exporters/otlp/otlptrace/otlptracegrpc/client_test.go b/exporters/otlp/otlptrace/otlptracegrpc/client_test.go index 395ccc28bf4..aa9cbe122a6 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/client_test.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/client_test.go @@ -26,9 +26,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/goleak" "google.golang.org/grpc" - "google.golang.org/grpc/codes" "google.golang.org/grpc/encoding/gzip" - "google.golang.org/grpc/status" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -239,7 +237,8 @@ func TestExportSpansTimeoutHonored(t *testing.T) { // Release the export so everything is cleaned up on shutdown. close(exportBlock) - require.Equal(t, codes.DeadlineExceeded, status.Convert(err).Code()) + require.Error(t, err) + require.Contains(t, err.Error(), "deadline exceeded") } func TestNewWithMultipleAttributeTypes(t *testing.T) { diff --git a/exporters/otlp/otlptrace/otlptracehttp/client.go b/exporters/otlp/otlptrace/otlptracehttp/client.go index 79b8af73286..9fbe861717d 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/client.go +++ b/exporters/otlp/otlptrace/otlptracehttp/client.go @@ -197,7 +197,7 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc } return newResponseError(resp.Header) default: - return fmt.Errorf("failed to send %s to %s: %s", d.name, request.URL, resp.Status) + return fmt.Errorf("failed to send to %s: %s", request.URL, resp.Status) } }) } diff --git a/exporters/otlp/otlptrace/otlptracehttp/client_test.go b/exporters/otlp/otlptrace/otlptracehttp/client_test.go index 135b6517622..9d1a6694887 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/client_test.go +++ b/exporters/otlp/otlptrace/otlptracehttp/client_test.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "net/http" - "os" "testing" "time" @@ -219,7 +218,8 @@ func TestTimeout(t *testing.T) { assert.NoError(t, exporter.Shutdown(ctx)) }() err = exporter.ExportSpans(ctx, otlptracetest.SingleReadOnlySpan()) - assert.Equalf(t, true, os.IsTimeout(err), "expected timeout error, got: %v", err) + + assert.Contains(t, err.Error(), "deadline exceeded") } func TestNoRetry(t *testing.T) { @@ -246,7 +246,7 @@ func TestNoRetry(t *testing.T) { }() err = exporter.ExportSpans(ctx, otlptracetest.SingleReadOnlySpan()) assert.Error(t, err) - assert.Equal(t, fmt.Sprintf("failed to send traces to http://%s/v1/traces: 400 Bad Request", mc.endpoint), err.Error()) + assert.Equal(t, fmt.Sprintf("trace export: failed to send to http://%s/v1/traces: 400 Bad Request", mc.endpoint), err.Error()) assert.Empty(t, mc.GetSpans()) } From f224003fc4315955d6e3a4d9846ac2dc4709569f Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 16 Dec 2022 11:02:39 -0800 Subject: [PATCH 2/5] use a wrapped error --- CHANGELOG.md | 6 ++ exporters/otlp/internal/wrappederror.go | 61 +++++++++++++++++++ exporters/otlp/internal/wrappederror_test.go | 32 ++++++++++ exporters/otlp/otlptrace/exporter.go | 4 +- exporters/otlp/otlptrace/exporter_test.go | 9 +-- .../otlptrace/otlptracegrpc/client_test.go | 18 +++--- .../otlptrace/otlptracehttp/client_test.go | 22 ++++--- 7 files changed, 129 insertions(+), 23 deletions(-) create mode 100644 exporters/otlp/internal/wrappederror.go create mode 100644 exporters/otlp/internal/wrappederror_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f0e0943382..df7a80bdf7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add `Producer` interface and `Reader.RegisterProducer(Producer)` to `go.opentelemetry.io/otel/sdk/metric` to enable external metric Producers. (#3524) +### Changed + +- OTLP trace exporter errors are wrapped in a type that prefixes the + signal name in front of error strings (e.g., "traces export: context + canceled" insterafd of just "context canceled"). (#3016) + ## [1.11.2/0.34.0] 2022-12-05 ### Added diff --git a/exporters/otlp/internal/wrappederror.go b/exporters/otlp/internal/wrappederror.go new file mode 100644 index 00000000000..217751da552 --- /dev/null +++ b/exporters/otlp/internal/wrappederror.go @@ -0,0 +1,61 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package internal // import "go.opentelemetry.io/otel/exporters/otlp/internal" + +// ErrorKind is used to identify the kind of export error +// being wrapped. +type ErrorKind int + +const ( + // TracesExport indicates the error comes from the OTLP trace exporter. + TracesExport ErrorKind = iota +) + +// prefix returns a prefix for the Error() string. +func (k ErrorKind) prefix() string { + switch k { + case TracesExport: + return "traces export: " + default: + return "unknown: " + } +} + +// wrappedExportError wraps an OTLP exporter error with the kind of +// signal that produced it. +type wrappedExportError struct { + wrap error + kind ErrorKind +} + +// WrapTracesError wraps an error from the OTLP exporter for traces. +func WrapTracesError(err error) error { + return wrappedExportError{ + wrap: err, + kind: TracesExport, + } +} + +var _ error = wrappedExportError{} + +// Error attaches a prefix corresponding to the kind of exporter. +func (t wrappedExportError) Error() string { + return t.kind.prefix() + t.wrap.Error() +} + +// Unwrap returns the wrapped error. +func (t wrappedExportError) Unwrap() error { + return t.wrap +} diff --git a/exporters/otlp/internal/wrappederror_test.go b/exporters/otlp/internal/wrappederror_test.go new file mode 100644 index 00000000000..995358a9f8e --- /dev/null +++ b/exporters/otlp/internal/wrappederror_test.go @@ -0,0 +1,32 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package internal // import "go.opentelemetry.io/otel/exporters/otlp/internal" + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestWrappedError(t *testing.T) { + e := WrapTracesError(context.Canceled) + + require.Equal(t, context.Canceled, errors.Unwrap(e)) + require.Equal(t, TracesExport, e.(wrappedExportError).kind) + require.Equal(t, "traces export: context canceled", e.Error()) + require.True(t, errors.Is(e, context.Canceled)) +} diff --git a/exporters/otlp/otlptrace/exporter.go b/exporters/otlp/otlptrace/exporter.go index 0b4ea1cde96..b65802edbbd 100644 --- a/exporters/otlp/otlptrace/exporter.go +++ b/exporters/otlp/otlptrace/exporter.go @@ -17,9 +17,9 @@ package otlptrace // import "go.opentelemetry.io/otel/exporters/otlp/otlptrace" import ( "context" "errors" - "fmt" "sync" + "go.opentelemetry.io/otel/exporters/otlp/internal" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/tracetransform" tracesdk "go.opentelemetry.io/otel/sdk/trace" ) @@ -48,7 +48,7 @@ func (e *Exporter) ExportSpans(ctx context.Context, ss []tracesdk.ReadOnlySpan) err := e.client.UploadTraces(ctx, protoSpans) if err != nil { - return fmt.Errorf("trace export: %w", err) + return internal.WrapTracesError(err) } return nil } diff --git a/exporters/otlp/otlptrace/exporter_test.go b/exporters/otlp/otlptrace/exporter_test.go index 27eec2637c3..1007d23a7e2 100644 --- a/exporters/otlp/otlptrace/exporter_test.go +++ b/exporters/otlp/otlptrace/exporter_test.go @@ -17,11 +17,12 @@ package otlptrace_test import ( "context" "errors" + "strings" "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/exporters/otlp/otlptrace" - tracesdk "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" tracepb "go.opentelemetry.io/proto/otlp/trace/v1" ) @@ -44,10 +45,6 @@ func (c *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc return c.uploadErr } -type span struct { - tracesdk.ReadOnlySpan -} - func TestExporterClientError(t *testing.T) { ctx := context.Background() exp, err := otlptrace.New(ctx, &client{ @@ -60,7 +57,7 @@ func TestExporterClientError(t *testing.T) { assert.Error(t, err) assert.True(t, errors.Is(err, context.Canceled)) - assert.Contains(t, err.Error(), "trace export") + assert.True(t, strings.HasPrefix(err.Error(), "traces export: "), err) assert.NoError(t, exp.Shutdown(ctx)) } diff --git a/exporters/otlp/otlptrace/otlptracegrpc/client_test.go b/exporters/otlp/otlptrace/otlptracegrpc/client_test.go index aa9cbe122a6..ded71a5a3b4 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/client_test.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/client_test.go @@ -16,6 +16,7 @@ package otlptracegrpc_test import ( "context" + "errors" "fmt" "net" "strings" @@ -26,7 +27,9 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/goleak" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/encoding/gzip" + "google.golang.org/grpc/status" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -237,8 +240,9 @@ func TestExportSpansTimeoutHonored(t *testing.T) { // Release the export so everything is cleaned up on shutdown. close(exportBlock) - require.Error(t, err) - require.Contains(t, err.Error(), "deadline exceeded") + unwrapped := errors.Unwrap(err) + require.Equal(t, codes.DeadlineExceeded, status.Convert(unwrapped).Code()) + require.True(t, strings.HasPrefix(err.Error(), "traces export: "), err) } func TestNewWithMultipleAttributeTypes(t *testing.T) { @@ -398,18 +402,18 @@ func TestPartialSuccess(t *testing.T) { }) t.Cleanup(func() { require.NoError(t, mc.stop()) }) - errors := []error{} + errs := []error{} otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) { - errors = append(errors, err) + errs = append(errs, err) })) ctx := context.Background() exp := newGRPCExporter(t, ctx, mc.endpoint) t.Cleanup(func() { require.NoError(t, exp.Shutdown(ctx)) }) require.NoError(t, exp.ExportSpans(ctx, roSpans)) - require.Equal(t, 1, len(errors)) - require.Contains(t, errors[0].Error(), "partially successful") - require.Contains(t, errors[0].Error(), "2 spans rejected") + require.Equal(t, 1, len(errs)) + require.Contains(t, errs[0].Error(), "partially successful") + require.Contains(t, errs[0].Error(), "2 spans rejected") } func TestCustomUserAgent(t *testing.T) { diff --git a/exporters/otlp/otlptrace/otlptracehttp/client_test.go b/exporters/otlp/otlptrace/otlptracehttp/client_test.go index 9d1a6694887..859c48dd38c 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/client_test.go +++ b/exporters/otlp/otlptrace/otlptracehttp/client_test.go @@ -16,8 +16,11 @@ package otlptracehttp_test import ( "context" + "errors" "fmt" "net/http" + "os" + "strings" "testing" "time" @@ -218,8 +221,9 @@ func TestTimeout(t *testing.T) { assert.NoError(t, exporter.Shutdown(ctx)) }() err = exporter.ExportSpans(ctx, otlptracetest.SingleReadOnlySpan()) - - assert.Contains(t, err.Error(), "deadline exceeded") + unwrapped := errors.Unwrap(err) + assert.Equalf(t, true, os.IsTimeout(unwrapped), "expected timeout error, got: %v", unwrapped) + assert.True(t, strings.HasPrefix(err.Error(), "traces export: "), err) } func TestNoRetry(t *testing.T) { @@ -246,7 +250,9 @@ func TestNoRetry(t *testing.T) { }() err = exporter.ExportSpans(ctx, otlptracetest.SingleReadOnlySpan()) assert.Error(t, err) - assert.Equal(t, fmt.Sprintf("trace export: failed to send to http://%s/v1/traces: 400 Bad Request", mc.endpoint), err.Error()) + unwrapped := errors.Unwrap(err) + assert.Equal(t, fmt.Sprintf("failed to send to http://%s/v1/traces: 400 Bad Request", mc.endpoint), unwrapped.Error()) + assert.True(t, strings.HasPrefix(err.Error(), "traces export: ")) assert.Empty(t, mc.GetSpans()) } @@ -384,14 +390,14 @@ func TestPartialSuccess(t *testing.T) { assert.NoError(t, exporter.Shutdown(context.Background())) }() - errors := []error{} + errs := []error{} otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) { - errors = append(errors, err) + errs = append(errs, err) })) err = exporter.ExportSpans(ctx, otlptracetest.SingleReadOnlySpan()) assert.NoError(t, err) - require.Equal(t, 1, len(errors)) - require.Contains(t, errors[0].Error(), "partially successful") - require.Contains(t, errors[0].Error(), "2 spans rejected") + require.Equal(t, 1, len(errs)) + require.Contains(t, errs[0].Error(), "partially successful") + require.Contains(t, errs[0].Error(), "2 spans rejected") } From 78f75d45201b32d58a360e481c29ac8a8fedebb9 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 16 Dec 2022 11:07:40 -0800 Subject: [PATCH 3/5] update changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df7a80bdf7c..f9d48aca61e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - OTLP trace exporter errors are wrapped in a type that prefixes the signal name in front of error strings (e.g., "traces export: context - canceled" insterafd of just "context canceled"). (#3016) + canceled" insterafd of just "context canceled"). Existing users of + the exporters attempting to identify specific errors will need to use + `errors.Unwrap()` to get the underlying error. (#3016) ## [1.11.2/0.34.0] 2022-12-05 From 127ef48398a8be644f04e8c371620f29aca4eeb2 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 4 Jan 2023 10:47:14 -0800 Subject: [PATCH 4/5] merge changelog --- CHANGELOG.md | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8993ad73e7f..8492678074e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `traceIDRatioSampler` (given by `TraceIDRatioBased(float64)`) now uses the rightmost bits for sampling decisions, fixing random sampling when using ID generators like `xray.IDGenerator` and increasing parity with other language implementations. (#3557) +- OTLP trace exporter errors are wrapped in a type that prefixes the signal name in front of error strings (e.g., "traces export: context canceled" insterafd of just "context canceled"). Existing users of the exporters attempting to identify specific errors will need to use `errors.Unwrap()` to get the underlying error. (#3016) ### Deprecated @@ -53,14 +54,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The deprecated `go.opentelemetry.io/otel/sdk/metric/view` package is removed. (#3520) -### Changed - -- OTLP trace exporter errors are wrapped in a type that prefixes the - signal name in front of error strings (e.g., "traces export: context - canceled" insterafd of just "context canceled"). Existing users of - the exporters attempting to identify specific errors will need to use - `errors.Unwrap()` to get the underlying error. (#3016) - ## [1.11.2/0.34.0] 2022-12-05 ### Added From 8be26f19e614fcb5c036334f227bf4d09c3c86e2 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 27 Jan 2023 10:14:02 -0800 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8311171324e..3a8c434ad91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `traceIDRatioSampler` (given by `TraceIDRatioBased(float64)`) now uses the rightmost bits for sampling decisions, fixing random sampling when using ID generators like `xray.IDGenerator` and increasing parity with other language implementations. (#3557) -- OTLP trace exporter errors are wrapped in a type that prefixes the signal name in front of error strings (e.g., "traces export: context canceled" insterafd of just "context canceled"). Existing users of the exporters attempting to identify specific errors will need to use `errors.Unwrap()` to get the underlying error. (#3016) +- OTLP trace exporter errors are wrapped in a type that prefixes the signal name in front of error strings (e.g., "traces export: context canceled" instead of just "context canceled"). + Existing users of the exporters attempting to identify specific errors will need to use `errors.Unwrap()` to get the underlying error. (#3516) - The OTLP exporter for traces and metrics will print the final retryable error message when attempts to retry time out. (#3514) - The instrument kind names in `go.opentelemetry.io/otel/sdk/metric` are updated to match the API. (#3562) - `InstrumentKindSyncCounter` is renamed to `InstrumentKindCounter`