From 58704eca3f974a6a10f5095769775b6877306694 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 4 Dec 2023 21:43:33 -0500 Subject: [PATCH 01/37] Add new consumererror error types --- .chloggen/breaking-retryable-errors.yaml | 25 ++++ .chloggen/issue-7047.yaml | 25 ++++ consumer/consumererror/README.md | 117 ++++++++++++++++++ consumer/consumererror/component.go | 38 ++++++ consumer/consumererror/component_test.go | 37 ++++++ .../internal/statusconversion/conversion.go | 52 ++++++++ .../statusconversion/conversion_test.go | 113 +++++++++++++++++ consumer/consumererror/partial.go | 39 ++++++ consumer/consumererror/partial_test.go | 36 ++++++ consumer/consumererror/permanent.go | 11 +- consumer/consumererror/permanent_test.go | 60 ++++++--- consumer/consumererror/signalerrors.go | 75 +++++++++-- consumer/consumererror/signalerrors_test.go | 42 +++++++ consumer/consumererror/transporterrors.go | 91 ++++++++++++++ .../consumererror/transporterrors_test.go | 111 +++++++++++++++++ consumer/go.mod | 8 +- consumer/go.sum | 16 +++ 17 files changed, 860 insertions(+), 36 deletions(-) create mode 100644 .chloggen/breaking-retryable-errors.yaml create mode 100644 .chloggen/issue-7047.yaml create mode 100644 consumer/consumererror/README.md create mode 100644 consumer/consumererror/component.go create mode 100644 consumer/consumererror/component_test.go create mode 100644 consumer/consumererror/internal/statusconversion/conversion.go create mode 100644 consumer/consumererror/internal/statusconversion/conversion_test.go create mode 100644 consumer/consumererror/partial.go create mode 100644 consumer/consumererror/partial_test.go create mode 100644 consumer/consumererror/transporterrors.go create mode 100644 consumer/consumererror/transporterrors_test.go diff --git a/.chloggen/breaking-retryable-errors.yaml b/.chloggen/breaking-retryable-errors.yaml new file mode 100644 index 00000000000..a36f16000c2 --- /dev/null +++ b/.chloggen/breaking-retryable-errors.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: consumererror + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add optional arguments to `NewTraces`, `NewMetrics`, and `NewLogs` + +# One or more tracking issues or pull requests related to the change +issues: [7047] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: This change has no impact unless you depend on the type signature of these functions. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/.chloggen/issue-7047.yaml b/.chloggen/issue-7047.yaml new file mode 100644 index 00000000000..6dc9d1bcfce --- /dev/null +++ b/.chloggen/issue-7047.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: consumererror + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Provide additional error types to allow annotating errors + +# One or more tracking issues or pull requests related to the change +issues: [7047] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: Allows putting things like status codes, retry information, etc. on consumer errors. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md new file mode 100644 index 00000000000..96ccbefb73f --- /dev/null +++ b/consumer/consumererror/README.md @@ -0,0 +1,117 @@ +# Consumer errors + +This package contains error types that should be returned by a consumer when an +error occurs while processing telemetry. The error types included in this +package provide functionality for communicating details about +the error for use upstream in the pipeline. Ideally the error returned by a +component in its `consume` function should be from this package. + +## Use cases + +**Retry logic**: Errors should be allowed to embed rejected telemetry items to +be retried by the consumer. + +**Indicating partial success**: Errors can indicate that not all items were +accepted, for example as in an OTLP partial success message. + +**Communicating network error codes**: Errors should allow embedding information +necessary for the Collector to act as a proxy for a backend, i.e. relay a status +code returned from a backend in a response to a system upstream from the +Collector. + +## Creating Errors + +Errors can be created through one of the three constructors provided: + +- `consumererror.New[Signal]` for retryable errors. +- `consumererror.NewPartial` for errors where only some of the items failed. +- `consumererror.NewHTTP` for errors resulting from an HTTP call with an error status code. +- `consumererror.NewGRPC` for errors resulting from a gRPC call with an error status code. +- `consumererror.NewComponent` for metadata around the component that produced the error. + +Only retryable errors will be retried, all other errors are considered permanent +and will not be retried. + +Errors can be joined by passing them to a call to `errors.Join`. + + +### Per-signal retryable errors + +If an error is considered transient and data processing should be retried, the data +that should be retried should be created using `NewTraces`, `NewMetrics` or `NewLogs` +with the data that should be retried, e.g. `NewTraces(err, traces)`. The data that +is contained in the error will be the only data that is retried, the caller should +not use its copy of the data for the retry. + +## Other considerations + +To keep error analysis simple when looking at an error upstream in a pipeline, +the component closest to the source of an error or set of errors should make a +decision about the nature of the error. The following are a few places where +special considerations may need to be made. + +### Fanouts + +#### Permanent errors + +When a fanout component receives multiple permanent errors from downstream in +the pipeline, it should determine the error with the highest precedence and +forward that upward in the pipeline. If a precedence cannot be determined, +the component should forward all errors. + +#### Retryable errors + +When a fanout component receives a retryable error, it must only retry the data +the failed pipeline branch. + +#### Mixed errors + +When a mix of retryable and permanent errors are received from downstream +pipeline branches, the fanout component should continue all retries for +retryable data and return a permanent error upstream regardless of whether +any of the retries succeed. The component performing retries should capture +which pipelines succeeded or failed using the Collector's pipeline observability +mechanisms. + +### Signal conversion + +When converting between signals in a pipeline, it is expected that the component +performing the conversion should perform the translation necessary in the error +for any signal item counts. If the converted count cannot be determined, the full +count of pre-converted signals should be returned. + +### Asynchronous processing + +The use of any components that do asynchronous processing in a pipeline will cut +off error backpropagation at the asynchronous component. The asynchronous +component may communicate error information using the Collector's own signals. + +## Examples + +Creating an error: + +```golang +errors.Join( + consumererror.NewComponent(exp.componentID), + consumererror.NewTraces( + error, + traces, + consumererror.WithRetryDelay(2 * time.Duration) + ), + consumererror.NewHTTP(error, http.StatusTooManyRequests) +) +``` + +Using an error: + +```golang +err := nextConsumer(ctx, traces) +var status consumererror.TransportError +var retry consumererror.Traces + +if errors.As(err, &status) { + if status.HTTPStatus >= 500 && errors.As(err, &retry) { + doRetry(retry.Data()) + } +} +``` diff --git a/consumer/consumererror/component.go b/consumer/consumererror/component.go new file mode 100644 index 00000000000..c8fc91289fa --- /dev/null +++ b/consumer/consumererror/component.go @@ -0,0 +1,38 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +import "go.opentelemetry.io/collector/component" + +// Component contains information about the component that returned an +// error. This is helpful when reasoning about the context an error +// occurred in when handling errors in upstream components within a +// pipeline. +type Component struct { + err error + id component.ID +} + +var _ error = Component{} + +// NewComponent instantiates a new Component error. +// `id` should be the component's component.ID object. +func NewComponent(err error, id component.ID) error { + return Component{err: err, id: id} +} + +// Error returns a string representation of the underlying error. +func (c Component) Error() string { + return c.id.String() + " returned an error: " + c.err.Error() +} + +// Unwrap returns the wrapped error for functions Is and As in standard package errors. +func (c Component) Unwrap() error { + return c.err +} + +// Count returns the count of rejected records. +func (c Component) ID() component.ID { + return c.id +} diff --git a/consumer/consumererror/component_test.go b/consumer/consumererror/component_test.go new file mode 100644 index 00000000000..d56defaf31c --- /dev/null +++ b/consumer/consumererror/component_test.go @@ -0,0 +1,37 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/component" +) + +func TestComponent_Error(t *testing.T) { + e := errors.New("couldn't export") + pe := NewComponent(e, component.MustNewID("nop")) + require.Contains(t, pe.Error(), e.Error()) +} + +func TestComponent_Unwrap(t *testing.T) { + var err error = testErrorType{"some error"} + ce := NewComponent(err, component.MustNewIDWithName("nop", "test")) + joinedErr := errors.Join(errors.New("other error"), ce) + target := testErrorType{} + require.NotEqual(t, err, target) + require.True(t, errors.As(joinedErr, &target)) + require.Equal(t, err, target) +} + +func TestComponent_ID(t *testing.T) { + id := component.MustNewIDWithName("nop", "test") + e := NewComponent(errors.New("couldn't export"), id) + var ce Component + ok := errors.As(e, &ce) + require.True(t, ok) + require.Equal(t, id, ce.ID()) +} diff --git a/consumer/consumererror/internal/statusconversion/conversion.go b/consumer/consumererror/internal/statusconversion/conversion.go new file mode 100644 index 00000000000..a193206dfb6 --- /dev/null +++ b/consumer/consumererror/internal/statusconversion/conversion.go @@ -0,0 +1,52 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package statusconversion // import "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" + +import ( + "net/http" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func GetHTTPStatusCodeFromStatus(s *status.Status) int { + // See https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#failures + // to see if a code is retryable. + // See https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#failures-1 + // to see a list of retryable http status codes. + switch s.Code() { + // Retryable + case codes.Canceled, codes.DeadlineExceeded, codes.Aborted, codes.OutOfRange, codes.Unavailable, codes.DataLoss: + return http.StatusServiceUnavailable + // Retryable + case codes.ResourceExhausted: + return http.StatusTooManyRequests + // Not Retryable + default: + return http.StatusInternalServerError + } +} + +func NewStatusFromMsgAndHTTPCode(errMsg string, statusCode int) *status.Status { + var c codes.Code + // Mapping based on https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md + // 429 mapping to ResourceExhausted and 400 mapping to StatusBadRequest are exceptions. + switch statusCode { + case http.StatusBadRequest: + c = codes.InvalidArgument + case http.StatusUnauthorized: + c = codes.Unauthenticated + case http.StatusForbidden: + c = codes.PermissionDenied + case http.StatusNotFound: + c = codes.Unimplemented + case http.StatusTooManyRequests: + c = codes.ResourceExhausted + case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: + c = codes.Unavailable + default: + c = codes.Unknown + } + return status.New(c, errMsg) +} diff --git a/consumer/consumererror/internal/statusconversion/conversion_test.go b/consumer/consumererror/internal/statusconversion/conversion_test.go new file mode 100644 index 00000000000..e87294e680d --- /dev/null +++ b/consumer/consumererror/internal/statusconversion/conversion_test.go @@ -0,0 +1,113 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package statusconversion // import "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func Test_GetHTTPStatusCodeFromStatus(t *testing.T) { + tests := []struct { + name string + input *status.Status + expected int + }{ + { + name: "Retryable Status", + input: status.New(codes.Unavailable, "test"), + expected: http.StatusServiceUnavailable, + }, + { + name: "Non-retryable Status", + input: status.New(codes.InvalidArgument, "test"), + expected: http.StatusInternalServerError, + }, + { + name: "Specifically 429", + input: status.New(codes.ResourceExhausted, "test"), + expected: http.StatusTooManyRequests, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := GetHTTPStatusCodeFromStatus(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} + +func Test_ErrorMsgAndHTTPCodeToStatus(t *testing.T) { + tests := []struct { + name string + errMsg string + statusCode int + expected *status.Status + }{ + { + name: "Bad Request", + errMsg: "test", + statusCode: http.StatusBadRequest, + expected: status.New(codes.InvalidArgument, "test"), + }, + { + name: "Unauthorized", + errMsg: "test", + statusCode: http.StatusUnauthorized, + expected: status.New(codes.Unauthenticated, "test"), + }, + { + name: "Forbidden", + errMsg: "test", + statusCode: http.StatusForbidden, + expected: status.New(codes.PermissionDenied, "test"), + }, + { + name: "Not Found", + errMsg: "test", + statusCode: http.StatusNotFound, + expected: status.New(codes.Unimplemented, "test"), + }, + { + name: "Too Many Requests", + errMsg: "test", + statusCode: http.StatusTooManyRequests, + expected: status.New(codes.ResourceExhausted, "test"), + }, + { + name: "Bad Gateway", + errMsg: "test", + statusCode: http.StatusBadGateway, + expected: status.New(codes.Unavailable, "test"), + }, + { + name: "Service Unavailable", + errMsg: "test", + statusCode: http.StatusServiceUnavailable, + expected: status.New(codes.Unavailable, "test"), + }, + { + name: "Gateway Timeout", + errMsg: "test", + statusCode: http.StatusGatewayTimeout, + expected: status.New(codes.Unavailable, "test"), + }, + { + name: "Unsupported Media Type", + errMsg: "test", + statusCode: http.StatusUnsupportedMediaType, + expected: status.New(codes.Unknown, "test"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := NewStatusFromMsgAndHTTPCode(tt.errMsg, tt.statusCode) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/consumer/consumererror/partial.go b/consumer/consumererror/partial.go new file mode 100644 index 00000000000..b4e6de859b5 --- /dev/null +++ b/consumer/consumererror/partial.go @@ -0,0 +1,39 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +// Partial describes situations where only some telemetry data was +// accepted. +// The semantics for this error are based on OTLP partial success +// messages. +// See: https://github.com/open-telemetry/opentelemetry-proto/blob/9d139c87b52669a3e2825b835dd828b57a455a55/docs/specification.md#partial-success +type Partial struct { + err error + count int +} + +var _ error = Partial{} + +// NewPartial instantiates a new Partial error. +// `count` should be a positive number representing +// the number of failed records, i.e. spans, data points, +// or log records. +func NewPartial(err error, count int) error { + return Partial{err: err, count: count} +} + +// Error returns a string representation of the underlying error. +func (p Partial) Error() string { + return "Partial success: " + p.err.Error() +} + +// Unwrap returns the wrapped error for functions Is and As in standard package errors. +func (p Partial) Unwrap() error { + return p.err +} + +// Count returns the count of rejected records. +func (p Partial) Count() int { + return p.count +} diff --git a/consumer/consumererror/partial_test.go b/consumer/consumererror/partial_test.go new file mode 100644 index 00000000000..596bf4547e2 --- /dev/null +++ b/consumer/consumererror/partial_test.go @@ -0,0 +1,36 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPartial_Error(t *testing.T) { + e := errors.New("couldn't export") + pe := NewPartial(e, 3) + require.Contains(t, pe.Error(), e.Error()) +} + +func TestPartial_Unwrap(t *testing.T) { + var err error = testErrorType{"some error"} + se := NewPartial(err, 4) + joinedErr := errors.Join(errors.New("other error"), se) + target := testErrorType{} + require.NotEqual(t, err, target) + require.True(t, errors.As(joinedErr, &target)) + require.Equal(t, err, target) +} + +func TestPartial_Count(t *testing.T) { + count := 4 + e := NewPartial(errors.New("couldn't export"), count) + var pe Partial + ok := errors.As(e, &pe) + require.True(t, ok) + require.Equal(t, count, pe.Count()) +} diff --git a/consumer/consumererror/permanent.go b/consumer/consumererror/permanent.go index 71e26df214f..87b03967524 100644 --- a/consumer/consumererror/permanent.go +++ b/consumer/consumererror/permanent.go @@ -11,6 +11,8 @@ type permanent struct { err error } +var _ error = permanent{} + // NewPermanent wraps an error to indicate that it is a permanent error, i.e. an // error that will be always returned if its source receives the same inputs. func NewPermanent(err error) error { @@ -26,12 +28,13 @@ func (p permanent) Unwrap() error { return p.err } -// IsPermanent checks if an error was wrapped with the NewPermanent function, which -// is used to indicate that a given error will always be returned in the case -// that its sources receives the same input. +// IsPermanent checks if an error was wrapped with the NewPermanent function or +// otherwise is not a retryable error. A `true` return value indicates that a +// given error will always be returned in the case that its sources receives the +// same input. func IsPermanent(err error) bool { if err == nil { return false } - return errors.As(err, &permanent{}) + return errors.As(err, &permanent{}) || errors.As(err, &Partial{}) || errors.As(err, &TransportError{}) } diff --git a/consumer/consumererror/permanent_test.go b/consumer/consumererror/permanent_test.go index a8291db0496..e3761213639 100644 --- a/consumer/consumererror/permanent_test.go +++ b/consumer/consumererror/permanent_test.go @@ -10,28 +10,52 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" -) - -type testErrorType struct { - s string -} -func (t testErrorType) Error() string { - return "" -} + "go.opentelemetry.io/collector/pdata/ptrace" +) func TestIsPermanent(t *testing.T) { - var err error - assert.False(t, IsPermanent(err)) - - err = errors.New("testError") - assert.False(t, IsPermanent(err)) - - err = NewPermanent(err) - assert.True(t, IsPermanent(err)) + tests := []struct { + err error + permanent bool + }{ + { + err: NewPermanent(errors.New("testError")), + permanent: true, + }, + { + err: NewHTTP(errors.New("testError"), 500), + permanent: true, + }, + { + err: NewGRPC(errors.New("testError"), nil), + permanent: true, + }, + { + err: NewPartial(errors.New("testError"), 123), + permanent: true, + }, + { + err: fmt.Errorf("%w", NewPermanent(errors.New("testError"))), + permanent: true, + }, + { + err: errors.New("testError"), + permanent: false, + }, + { + err: NewTraces(errors.New("testError"), ptrace.Traces{}), + permanent: false, + }, + { + err: NewTraces(NewPermanent(errors.New("testError")), ptrace.Traces{}), + permanent: true, + }, + } - err = fmt.Errorf("%w", err) - assert.True(t, IsPermanent(err)) + for _, tt := range tests { + assert.Equal(t, tt.permanent, IsPermanent(tt.err)) + } } func TestPermanent_Unwrap(t *testing.T) { diff --git a/consumer/consumererror/signalerrors.go b/consumer/consumererror/signalerrors.go index 1d7558ce1ca..1d55d665616 100644 --- a/consumer/consumererror/signalerrors.go +++ b/consumer/consumererror/signalerrors.go @@ -4,16 +4,25 @@ package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" import ( + "time" + "go.opentelemetry.io/collector/pdata/plog" "go.opentelemetry.io/collector/pdata/pmetric" "go.opentelemetry.io/collector/pdata/ptrace" ) -type retryable[V ptrace.Traces | pmetric.Metrics | plog.Logs] struct { +type retryableCommon struct { error + delay time.Duration +} + +type retryable[V ptrace.Traces | pmetric.Metrics | plog.Logs] struct { + retryableCommon data V } +var _ error = retryable[ptrace.Traces]{} + // Unwrap returns the wrapped error for functions Is and As in standard package errors. func (err retryable[V]) Unwrap() error { return err.error @@ -24,6 +33,22 @@ func (err retryable[V]) Data() V { return err.data } +// Delay returns the time duration before the telemetry data should +// be resent to the destination. +func (err retryable[V]) Delay() time.Duration { + return err.delay +} + +// RetryableOption allows adding data to a retryable error, +// e.g. the duration before sending should be retried. +type RetryableOption func(err *retryableCommon) + +func WithRetryDelay(delay time.Duration) RetryableOption { + return func(err *retryableCommon) { + err.delay = delay + } +} + // Traces is an error that may carry associated Trace data for a subset of received data // that failed to be processed or sent. type Traces struct { @@ -31,13 +56,21 @@ type Traces struct { } // NewTraces creates a Traces that can encapsulate received data that failed to be processed or sent. -func NewTraces(err error, data ptrace.Traces) error { - return Traces{ +func NewTraces(err error, data ptrace.Traces, options ...RetryableOption) error { + t := Traces{ retryable: retryable[ptrace.Traces]{ - error: err, - data: data, + retryableCommon: retryableCommon{ + error: err, + }, + data: data, }, } + + for _, opt := range options { + opt(&t.retryableCommon) + } + + return t } // Logs is an error that may carry associated Log data for a subset of received data @@ -47,13 +80,21 @@ type Logs struct { } // NewLogs creates a Logs that can encapsulate received data that failed to be processed or sent. -func NewLogs(err error, data plog.Logs) error { - return Logs{ +func NewLogs(err error, data plog.Logs, options ...RetryableOption) error { + l := Logs{ retryable: retryable[plog.Logs]{ - error: err, - data: data, + retryableCommon: retryableCommon{ + error: err, + }, + data: data, }, } + + for _, opt := range options { + opt(&l.retryableCommon) + } + + return l } // Metrics is an error that may carry associated Metrics data for a subset of received data @@ -63,11 +104,19 @@ type Metrics struct { } // NewMetrics creates a Metrics that can encapsulate received data that failed to be processed or sent. -func NewMetrics(err error, data pmetric.Metrics) error { - return Metrics{ +func NewMetrics(err error, data pmetric.Metrics, options ...RetryableOption) error { + m := Metrics{ retryable: retryable[pmetric.Metrics]{ - error: err, - data: data, + retryableCommon: retryableCommon{ + error: err, + }, + data: data, }, } + + for _, opt := range options { + opt(&m.retryableCommon) + } + + return m } diff --git a/consumer/consumererror/signalerrors_test.go b/consumer/consumererror/signalerrors_test.go index 475d3173bb9..fbc013a424e 100644 --- a/consumer/consumererror/signalerrors_test.go +++ b/consumer/consumererror/signalerrors_test.go @@ -6,6 +6,7 @@ package consumererror import ( "errors" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -13,6 +14,14 @@ import ( "go.opentelemetry.io/collector/pdata/testdata" ) +type testErrorType struct { + s string +} + +func (t testErrorType) Error() string { + return "" +} + func TestTraces(t *testing.T) { td := testdata.GenerateTraces(1) err := errors.New("some error") @@ -37,6 +46,17 @@ func TestTraces_Unwrap(t *testing.T) { require.Equal(t, err, target) } +func TestTraces_Delay(t *testing.T) { + delay := 4 * time.Second + td := testdata.GenerateTraces(1) + // Wrapping err with error Traces. + err := NewTraces(errors.New("some error"), td, WithRetryDelay(delay)) + var traceErr Traces + // Unwrapping traceErr for err and assigning to target. + require.True(t, errors.As(err, &traceErr)) + require.Equal(t, traceErr.Delay(), delay) +} + func TestLogs(t *testing.T) { td := testdata.GenerateLogs(1) err := errors.New("some error") @@ -61,6 +81,17 @@ func TestLogs_Unwrap(t *testing.T) { require.Equal(t, err, target) } +func TestLogs_Delay(t *testing.T) { + delay := 4 * time.Second + td := testdata.GenerateLogs(1) + // Wrapping err with error Logs. + err := NewLogs(errors.New("some error"), td, WithRetryDelay(delay)) + var logErr Logs + // Unwrapping LogErr for err and assigning to target. + require.True(t, errors.As(err, &logErr)) + require.Equal(t, logErr.Delay(), delay) +} + func TestMetrics(t *testing.T) { td := testdata.GenerateMetrics(1) err := errors.New("some error") @@ -84,3 +115,14 @@ func TestMetrics_Unwrap(t *testing.T) { require.True(t, errors.As(metricErr, &target)) require.Equal(t, err, target) } + +func TestMetrics_Delay(t *testing.T) { + delay := 4 * time.Second + td := testdata.GenerateMetrics(1) + // Wrapping err with error Metrics. + err := NewMetrics(errors.New("some error"), td, WithRetryDelay(delay)) + var metricErr Metrics + // Unwrapping MetricErr for err and assigning to target. + require.True(t, errors.As(err, &metricErr)) + require.Equal(t, metricErr.Delay(), delay) +} diff --git a/consumer/consumererror/transporterrors.go b/consumer/consumererror/transporterrors.go new file mode 100644 index 00000000000..7c0498c375b --- /dev/null +++ b/consumer/consumererror/transporterrors.go @@ -0,0 +1,91 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +import ( + "fmt" + "net/http" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" +) + +// TransportError contains either an HTTP or gRPC status code +// resulting from a failed synchronous network request. +// It allows retrieving a type of error code, converting +// between the status codes supported by each transport if necessary. +// +// It should be created with NewHTTP or NewGRPC. +type TransportError struct { + err error + httpStatus *int + grpcStatus *status.Status +} + +var _ error = TransportError{} + +// Error returns a string specifying the transport and corresponding status code. +func (se TransportError) Error() string { + if se.httpStatus != nil { + return fmt.Sprintf("HTTP Status (%d): %s", *se.httpStatus, se.err.Error()) + } else if se.grpcStatus != nil { + return fmt.Sprintf("gRPC Status (%s): %s", se.grpcStatus.Code().String(), se.err.Error()) + } + + return "Network error (no error code set): " + se.err.Error() +} + +func (se TransportError) Unwrap() error { + return se.err +} + +// HTTPStatus returns an HTTP status code either directly +// set by the source or derived from a gRPC status code set +// by the source. +// +// If no code has been set, the return value is +// an HTTP 500 code. +func (se TransportError) HTTPStatus() int { + if se.httpStatus != nil { + return *se.httpStatus + } else if se.grpcStatus != nil { + return statusconversion.GetHTTPStatusCodeFromStatus(se.grpcStatus) + } + + return http.StatusInternalServerError +} + +// GRPCStatus returns an gRPC status code either directly +// set by the source or derived from an HTTP status code set +// by the source. +// +// If no code has been set, the return value is set +// to `false`. +func (se TransportError) GRPCStatus() *status.Status { + if se.grpcStatus != nil { + return se.grpcStatus + } else if se.httpStatus != nil { + return statusconversion.NewStatusFromMsgAndHTTPCode(se.err.Error(), *se.httpStatus) + } + + return status.New(codes.Unknown, se.Error()) +} + +// NewHTTP wraps an error with a given HTTP status code. +func NewHTTP(err error, code int) error { + return TransportError{ + err: err, + httpStatus: &code, + } +} + +// NewGRPC wraps an error with a given gRPC status code. +func NewGRPC(err error, status *status.Status) error { + return TransportError{ + err: err, + grpcStatus: status, + } +} diff --git a/consumer/consumererror/transporterrors_test.go b/consumer/consumererror/transporterrors_test.go new file mode 100644 index 00000000000..5d6d74abee4 --- /dev/null +++ b/consumer/consumererror/transporterrors_test.go @@ -0,0 +1,111 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +import ( + "errors" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestTransportError_Error(t *testing.T) { + tests := []struct { + transportError error + msgContains string + validate func(t *testing.T, se TransportError) + }{ + { + transportError: NewHTTP(errors.New("httperror"), 400), + validate: func(t *testing.T, se TransportError) { + require.Contains(t, se.Error(), "400") + }, + }, + { + transportError: NewGRPC(errors.New("status"), status.New(codes.InvalidArgument, "")), + validate: func(t *testing.T, se TransportError) { + require.Contains(t, se.Error(), "InvalidArgument") + }, + }, + { + transportError: NewGRPC(errors.New("nil"), nil), + validate: func(t *testing.T, se TransportError) { + require.NotNil(t, se.Error()) + }, + }, + } + for _, tt := range tests { + var se TransportError + require.True(t, errors.As(tt.transportError, &se)) + tt.validate(t, se) + } +} + +func TestTransportError_Unwrap(t *testing.T) { + var err error = testErrorType{"some error"} + se := NewHTTP(err, 400) + joinedErr := errors.Join(errors.New("other error"), se) + target := testErrorType{} + require.NotEqual(t, err, target) + require.True(t, errors.As(joinedErr, &target)) + require.Equal(t, err, target) +} + +func TestTransportError_GRPCStatus(t *testing.T) { + tests := []struct { + transportError error + code codes.Code + }{ + { + transportError: NewHTTP(errors.New("httperror"), 429), + code: codes.ResourceExhausted, + }, + { + transportError: NewGRPC(errors.New("status"), status.New(codes.ResourceExhausted, "")), + code: codes.ResourceExhausted, + }, + { + transportError: NewGRPC(errors.New("nil"), nil), + code: codes.Unknown, + }, + } + for _, tt := range tests { + var se TransportError + require.True(t, errors.As(tt.transportError, &se)) + status := se.GRPCStatus() + require.Equal(t, tt.code, status.Code()) + } +} + +func TestTransportError_HTTPStatus(t *testing.T) { + tests := []struct { + transportError error + code int + ok bool + }{ + { + transportError: NewHTTP(errors.New("httperror"), http.StatusTooManyRequests), + code: http.StatusTooManyRequests, + ok: true, + }, + { + transportError: NewGRPC(errors.New("status"), status.New(codes.ResourceExhausted, "")), + code: http.StatusTooManyRequests, + ok: true, + }, + { + transportError: NewGRPC(errors.New("nil"), nil), + code: http.StatusInternalServerError, + }, + } + for _, tt := range tests { + var se TransportError + require.True(t, errors.As(tt.transportError, &se)) + code := se.HTTPStatus() + require.Equal(t, tt.code, code) + } +} diff --git a/consumer/go.mod b/consumer/go.mod index 263d4c9063e..39065a04ec6 100644 --- a/consumer/go.mod +++ b/consumer/go.mod @@ -4,9 +4,11 @@ go 1.21.0 require ( github.com/stretchr/testify v1.9.0 + go.opentelemetry.io/collector/component v0.104.0 go.opentelemetry.io/collector/pdata v1.11.0 go.opentelemetry.io/collector/pdata/testdata v0.104.0 go.uber.org/goleak v1.3.0 + google.golang.org/grpc v1.65.0 ) require ( @@ -16,13 +18,17 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + go.opentelemetry.io/collector/config/configtelemetry v0.104.0 // indirect go.opentelemetry.io/collector/pdata/pprofile v0.104.0 // indirect + go.opentelemetry.io/otel v1.27.0 // indirect + go.opentelemetry.io/otel/metric v1.27.0 // indirect + go.opentelemetry.io/otel/trace v1.27.0 // indirect go.uber.org/multierr v1.11.0 // indirect + go.uber.org/zap v1.27.0 // indirect golang.org/x/net v0.25.0 // indirect golang.org/x/sys v0.20.0 // indirect golang.org/x/text v0.15.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240528184218-531527333157 // indirect - google.golang.org/grpc v1.65.0 // indirect google.golang.org/protobuf v1.34.2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/consumer/go.sum b/consumer/go.sum index 528166b78c0..e9c6786ec42 100644 --- a/consumer/go.sum +++ b/consumer/go.sum @@ -1,6 +1,10 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ= +github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= +github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= @@ -29,10 +33,22 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= +go.opentelemetry.io/collector/component v0.104.0 h1:jqu/X9rnv8ha0RNZ1a9+x7OU49KwSMsPbOuIEykHuQE= +go.opentelemetry.io/collector/component v0.104.0/go.mod h1:1C7C0hMVSbXyY1ycCmaMUAR9fVwpgyiNQqxXtEWhVpw= +go.opentelemetry.io/collector/config/configtelemetry v0.104.0 h1:eHv98XIhapZA8MgTiipvi+FDOXoFhCYOwyKReOt+E4E= +go.opentelemetry.io/collector/config/configtelemetry v0.104.0/go.mod h1:WxWKNVAQJg/Io1nA3xLgn/DWLE/W1QOB2+/Js3ACi40= +go.opentelemetry.io/otel v1.27.0 h1:9BZoF3yMK/O1AafMiQTVu0YDj5Ea4hPhxCs7sGva+cg= +go.opentelemetry.io/otel v1.27.0/go.mod h1:DMpAK8fzYRzs+bi3rS5REupisuqTheUlSZJ1WnZaPAQ= +go.opentelemetry.io/otel/metric v1.27.0 h1:hvj3vdEKyeCi4YaYfNjv2NUje8FqKqUY8IlF0FxV/ik= +go.opentelemetry.io/otel/metric v1.27.0/go.mod h1:mVFgmRlhljgBiuk/MP/oKylr4hs85GZAylncepAX/ak= +go.opentelemetry.io/otel/trace v1.27.0 h1:IqYb813p7cmbHk0a5y6pD5JPakbVfftRXABGt5/Rscw= +go.opentelemetry.io/otel/trace v1.27.0/go.mod h1:6RiD1hkAprV4/q+yd2ln1HG9GoPx39SuvvstaLBl+l4= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= +go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8= +go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= From a4f738d07aaa4ccdf1c682e3dfceafb9281efacb Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 3 Jul 2024 10:06:23 -0400 Subject: [PATCH 02/37] impi --- consumer/consumererror/component_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/consumer/consumererror/component_test.go b/consumer/consumererror/component_test.go index d56defaf31c..9029b497ef0 100644 --- a/consumer/consumererror/component_test.go +++ b/consumer/consumererror/component_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/component" ) From 6a932c19f7b5306b8a83656cc59a48c90717fd73 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 3 Jul 2024 10:11:50 -0400 Subject: [PATCH 03/37] crosslink --- consumer/go.mod | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/consumer/go.mod b/consumer/go.mod index 39065a04ec6..76518356b96 100644 --- a/consumer/go.mod +++ b/consumer/go.mod @@ -43,3 +43,7 @@ retract ( v0.76.0 // Depends on retracted pdata v1.0.0-rc10 module, use v0.76.1 v0.69.0 // Release failed, use v0.69.1 ) + +replace go.opentelemetry.io/collector/config/configtelemetry => ../config/configtelemetry + +replace go.opentelemetry.io/collector/component => ../component From 2888dbc8104d57773b4ebaf40ad956d5201077f9 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 3 Jul 2024 10:46:01 -0400 Subject: [PATCH 04/37] tidy --- consumer/go.mod | 12 ++++++------ consumer/go.sum | 32 ++++++++++++++------------------ 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/consumer/go.mod b/consumer/go.mod index 76518356b96..6417adb0411 100644 --- a/consumer/go.mod +++ b/consumer/go.mod @@ -20,14 +20,14 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect go.opentelemetry.io/collector/config/configtelemetry v0.104.0 // indirect go.opentelemetry.io/collector/pdata/pprofile v0.104.0 // indirect - go.opentelemetry.io/otel v1.27.0 // indirect - go.opentelemetry.io/otel/metric v1.27.0 // indirect - go.opentelemetry.io/otel/trace v1.27.0 // indirect + go.opentelemetry.io/otel v1.28.0 // indirect + go.opentelemetry.io/otel/metric v1.28.0 // indirect + go.opentelemetry.io/otel/trace v1.28.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.uber.org/zap v1.27.0 // indirect - golang.org/x/net v0.25.0 // indirect - golang.org/x/sys v0.20.0 // indirect - golang.org/x/text v0.15.0 // indirect + golang.org/x/net v0.26.0 // indirect + golang.org/x/sys v0.21.0 // indirect + golang.org/x/text v0.16.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240528184218-531527333157 // indirect google.golang.org/protobuf v1.34.2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/consumer/go.sum b/consumer/go.sum index e9c6786ec42..48f14e6d679 100644 --- a/consumer/go.sum +++ b/consumer/go.sum @@ -1,8 +1,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ= -github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY= +github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= @@ -33,16 +33,12 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= -go.opentelemetry.io/collector/component v0.104.0 h1:jqu/X9rnv8ha0RNZ1a9+x7OU49KwSMsPbOuIEykHuQE= -go.opentelemetry.io/collector/component v0.104.0/go.mod h1:1C7C0hMVSbXyY1ycCmaMUAR9fVwpgyiNQqxXtEWhVpw= -go.opentelemetry.io/collector/config/configtelemetry v0.104.0 h1:eHv98XIhapZA8MgTiipvi+FDOXoFhCYOwyKReOt+E4E= -go.opentelemetry.io/collector/config/configtelemetry v0.104.0/go.mod h1:WxWKNVAQJg/Io1nA3xLgn/DWLE/W1QOB2+/Js3ACi40= -go.opentelemetry.io/otel v1.27.0 h1:9BZoF3yMK/O1AafMiQTVu0YDj5Ea4hPhxCs7sGva+cg= -go.opentelemetry.io/otel v1.27.0/go.mod h1:DMpAK8fzYRzs+bi3rS5REupisuqTheUlSZJ1WnZaPAQ= -go.opentelemetry.io/otel/metric v1.27.0 h1:hvj3vdEKyeCi4YaYfNjv2NUje8FqKqUY8IlF0FxV/ik= -go.opentelemetry.io/otel/metric v1.27.0/go.mod h1:mVFgmRlhljgBiuk/MP/oKylr4hs85GZAylncepAX/ak= -go.opentelemetry.io/otel/trace v1.27.0 h1:IqYb813p7cmbHk0a5y6pD5JPakbVfftRXABGt5/Rscw= -go.opentelemetry.io/otel/trace v1.27.0/go.mod h1:6RiD1hkAprV4/q+yd2ln1HG9GoPx39SuvvstaLBl+l4= +go.opentelemetry.io/otel v1.28.0 h1:/SqNcYk+idO0CxKEUOtKQClMK/MimZihKYMruSMViUo= +go.opentelemetry.io/otel v1.28.0/go.mod h1:q68ijF8Fc8CnMHKyzqL6akLO46ePnjkgfIMIjUIX9z4= +go.opentelemetry.io/otel/metric v1.28.0 h1:f0HGvSl1KRAU1DLgLGFjrwVyismPlnuU6JD6bOeuA5Q= +go.opentelemetry.io/otel/metric v1.28.0/go.mod h1:Fb1eVBFZmLVTMb6PPohq3TO9IIhUisDsbJoL/+uQW4s= +go.opentelemetry.io/otel/trace v1.28.0 h1:GhQ9cUuQGmNDd5BTCP2dAvv75RdMxEfTmYejp+lkx9g= +go.opentelemetry.io/otel/trace v1.28.0/go.mod h1:jPyXzNPg6da9+38HEwElrQiHlVMTnVfM3/yv2OlIHaI= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= @@ -58,20 +54,20 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= -golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= +golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ= +golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= -golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws= +golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk= -golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= +golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= From 7ddbc0d45440c3f4cca1ca585ea8ef24ad4151ff Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 10 Jul 2024 08:10:34 -0400 Subject: [PATCH 05/37] tidy --- consumer/consumerprofiles/go.mod | 6 +++--- consumer/consumerprofiles/go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/consumer/consumerprofiles/go.mod b/consumer/consumerprofiles/go.mod index 7b0a06cb098..0aacc15c52c 100644 --- a/consumer/consumerprofiles/go.mod +++ b/consumer/consumerprofiles/go.mod @@ -23,9 +23,9 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect go.opentelemetry.io/collector/pdata v1.11.0 // indirect go.uber.org/multierr v1.11.0 // indirect - golang.org/x/net v0.25.0 // indirect - golang.org/x/sys v0.20.0 // indirect - golang.org/x/text v0.15.0 // indirect + golang.org/x/net v0.26.0 // indirect + golang.org/x/sys v0.21.0 // indirect + golang.org/x/text v0.16.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240528184218-531527333157 // indirect google.golang.org/grpc v1.65.0 // indirect google.golang.org/protobuf v1.34.2 // indirect diff --git a/consumer/consumerprofiles/go.sum b/consumer/consumerprofiles/go.sum index 528166b78c0..4c9c65b576f 100644 --- a/consumer/consumerprofiles/go.sum +++ b/consumer/consumerprofiles/go.sum @@ -42,20 +42,20 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= -golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= +golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ= +golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= -golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws= +golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk= -golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= +golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= From 9e27e93209f086cf9e3b358e94beccc6ec8b149d Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 10 Jul 2024 08:15:53 -0400 Subject: [PATCH 06/37] crosslink --- consumer/consumerprofiles/go.mod | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/consumer/consumerprofiles/go.mod b/consumer/consumerprofiles/go.mod index 0aacc15c52c..1cbb659dea2 100644 --- a/consumer/consumerprofiles/go.mod +++ b/consumer/consumerprofiles/go.mod @@ -33,3 +33,7 @@ require ( ) replace go.opentelemetry.io/collector/pdata/testdata => ../../pdata/testdata + +replace go.opentelemetry.io/collector/config/configtelemetry => ../../config/configtelemetry + +replace go.opentelemetry.io/collector/component => ../../component From f4216c50400eeba85a245d47918be9df6113f574 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Sun, 14 Jul 2024 15:48:05 -0400 Subject: [PATCH 07/37] Update consumer/consumererror/README.md Co-authored-by: Dmitrii Anoshin --- consumer/consumererror/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 96ccbefb73f..67d1fff5f50 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -30,7 +30,7 @@ Errors can be created through one of the three constructors provided: - `consumererror.NewComponent` for metadata around the component that produced the error. Only retryable errors will be retried, all other errors are considered permanent -and will not be retried. +and should not be retried. Errors can be joined by passing them to a call to `errors.Join`. From a1ca2e11a5a92ea626a36c2fd83f90a3eb6c934a Mon Sep 17 00:00:00 2001 From: Evan Bradley Date: Sun, 14 Jul 2024 15:48:48 -0400 Subject: [PATCH 08/37] Address PR feedback --- consumer/consumererror/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 67d1fff5f50..320d80826ef 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -29,7 +29,7 @@ Errors can be created through one of the three constructors provided: - `consumererror.NewGRPC` for errors resulting from a gRPC call with an error status code. - `consumererror.NewComponent` for metadata around the component that produced the error. -Only retryable errors will be retried, all other errors are considered permanent +Only retryable errors should be retried, all other errors are considered permanent and should not be retried. Errors can be joined by passing them to a call to `errors.Join`. From 3b80b56b2e981604a3199ba3340601521dd07d45 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 24 Jul 2024 10:56:09 -0400 Subject: [PATCH 09/37] Update design doc --- consumer/consumererror/README.md | 174 ++++++++++++++++++++----------- 1 file changed, 111 insertions(+), 63 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 320d80826ef..f7d130042fe 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -6,10 +6,18 @@ package provide functionality for communicating details about the error for use upstream in the pipeline. Ideally the error returned by a component in its `consume` function should be from this package. +## Error classes + +**Retryable**: Errors are retryable if re-submitting data to a sink may result +in a successful submission. + +**Permanent**: Errors are permanent if submission will always fail for the +current data. + ## Use cases -**Retry logic**: Errors should be allowed to embed rejected telemetry items to -be retried by the consumer. +**Retry logic**: Errors should be allowed to include information necessary to +perform retries. **Indicating partial success**: Errors can indicate that not all items were accepted, for example as in an OTLP partial success message. @@ -19,59 +27,129 @@ necessary for the Collector to act as a proxy for a backend, i.e. relay a status code returned from a backend in a response to a system upstream from the Collector. +## Current targets for using errors + +**Receivers**: Receivers should be able to consume multiple errors downstream +and determine the best course of action based on the user's configuration. This +may entail either keeping the retry queue inside of the Collector by having the +receiver keep track of retries, or may involve having the caller manage the +retry queue by returning a retryable network code with relevant information. + +**exporterhelper**: When an exporter returns a retryable error, the +exporterhelper can use this information to manage the sending queue if it is +enabled. Permanent errors will be forwarded back up the pipeline. + ## Creating Errors -Errors can be created through one of the three constructors provided: +Errors can be created by calling `consumererror.New(err, opts...)` where `err` +is the underlying error, and `opts` is one of the provided options for supplying +additional metadata: -- `consumererror.New[Signal]` for retryable errors. -- `consumererror.NewPartial` for errors where only some of the items failed. -- `consumererror.NewHTTP` for errors resulting from an HTTP call with an error status code. -- `consumererror.NewGRPC` for errors resulting from a gRPC call with an error status code. -- `consumererror.NewComponent` for metadata around the component that produced the error. +- `consumererror.WithRetry[Signal]` +- `consumererror.WithPartial` +- `consumererror.WithGRPCStatus` +- `consumererror.WithHTTPStatus` +- `consumererror.WithMetadata` -Only retryable errors should be retried, all other errors are considered permanent -and should not be retried. +### Retrying data submission -Errors can be joined by passing them to a call to `errors.Join`. +If an error is transient, the `WithRetry` option corresponding to the relevant +signal should be used to indicate that the error is retryable and to pass on any +retry settings. These settings can come from the data sink or be determined by +the component, such as through runtime conditions or from user settings. +**Usage:** -### Per-signal retryable errors +```go +consumererror.WithRetry( + componentID, + consumerrerror.WithRetryDelay(10 * time.Second) +) +``` -If an error is considered transient and data processing should be retried, the data -that should be retried should be created using `NewTraces`, `NewMetrics` or `NewLogs` -with the data that should be retried, e.g. `NewTraces(err, traces)`. The data that -is contained in the error will be the only data that is retried, the caller should -not use its copy of the data for the retry. +`componentID` should be the ID of the component creating the error, so it is +known which components to retry submission on. The delay is an optional setting +that can be provided if it is available. -## Other considerations +### Indicating partial success + +If the component receives an OTLP partial success message (or other indication +of partial success), it should include this information with a count of the +failed records. + +**Usage:** + +```go +consumererror.WithPartial(failedRecords) +``` + +### Indicating error codes from network transports + +If the failure occurred due to a network transaction, the exporter should record +the status code of the message received from the backend. This information can +be then relayed to the receiver caller if necessary. Note that when the upstream +component reads a code, it will read a code for its desired transport, and the +code may be translated depending whether the input and output transports are +different. For example, a gRPC exporter may record a gRPC status. If a gRPC +receiver reads this status, it will be exactly the provided status. If an HTTP +receiver reads the status, it wants an HTTP status, and the gRPC status will be +converted to an equivalent HTTP code. + +**Usage:** + +```go +consumererror.WithGRPCStatus(codes.InvalidArgument) +consumererror.WithHTTPStatus(http.StatusTooManyRequests) +``` + +### Including custom data + +Custom data can be included as well for any additional information that needs to +be propagated back up the pipeline. It is up to the consuming component if or +how this data will be used. + +**Usage:** + +```go +consumererror.WithMetadata(MyMetadataStuct{}) +``` To keep error analysis simple when looking at an error upstream in a pipeline, the component closest to the source of an error or set of errors should make a decision about the nature of the error. The following are a few places where special considerations may need to be made. +## Using errors + ### Fanouts -#### Permanent errors +When fanouts receive multiple errors, they will combine them with +`(consumererror.Error).Combine(errs...)` and pass them upstream. The upstream +component can then later pull all errors out for analysis. + +### Retrieving errors -When a fanout component receives multiple permanent errors from downstream in -the pipeline, it should determine the error with the highest precedence and -forward that upward in the pipeline. If a precedence cannot be determined, -the component should forward all errors. +When a receiver gets a response that includes an error, it can get the data out +by doing something similar to the following. Note that this uses the `ErrorData` +type, which is for reading error data, as opposed to the `Error` type, which is +for recording errors. -#### Retryable errors +```go +cerr := consumererror.Error{} +var errData []consumerError.ErrorData -When a fanout component receives a retryable error, it must only retry the data -the failed pipeline branch. +if errors.As(err, &cerr) { + errData := cerr.Data() -#### Mixed errors + for _, data := range errData { + data.HTTPCode() + data.RetryableTraces() + data.Partial() + } +} +``` -When a mix of retryable and permanent errors are received from downstream -pipeline branches, the fanout component should continue all retries for -retryable data and return a permanent error upstream regardless of whether -any of the retries succeed. The component performing retries should capture -which pipelines succeeded or failed using the Collector's pipeline observability -mechanisms. +## Other considerations ### Signal conversion @@ -85,33 +163,3 @@ count of pre-converted signals should be returned. The use of any components that do asynchronous processing in a pipeline will cut off error backpropagation at the asynchronous component. The asynchronous component may communicate error information using the Collector's own signals. - -## Examples - -Creating an error: - -```golang -errors.Join( - consumererror.NewComponent(exp.componentID), - consumererror.NewTraces( - error, - traces, - consumererror.WithRetryDelay(2 * time.Duration) - ), - consumererror.NewHTTP(error, http.StatusTooManyRequests) -) -``` - -Using an error: - -```golang -err := nextConsumer(ctx, traces) -var status consumererror.TransportError -var retry consumererror.Traces - -if errors.As(err, &status) { - if status.HTTPStatus >= 500 && errors.As(err, &retry) { - doRetry(retry.Data()) - } -} -``` From fe1802b73c03d64fa88cef892e74e493cabeae52 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 25 Jul 2024 17:40:43 -0400 Subject: [PATCH 10/37] Update design doc --- consumer/consumererror/README.md | 46 ++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index f7d130042fe..bb9c5b1535f 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -12,7 +12,8 @@ component in its `consume` function should be from this package. in a successful submission. **Permanent**: Errors are permanent if submission will always fail for the -current data. +current data. Errors are considered permanent unless they are explicitly marked +as retryable. ## Use cases @@ -58,6 +59,9 @@ signal should be used to indicate that the error is retryable and to pass on any retry settings. These settings can come from the data sink or be determined by the component, such as through runtime conditions or from user settings. +**Note**: If retry information is not included in an error, the error will be +considered permanent and will not be retried. + **Usage:** ```go @@ -142,13 +146,51 @@ if errors.As(err, &cerr) { errData := cerr.Data() for _, data := range errData { - data.HTTPCode() + data.HTTPStatus() data.RetryableTraces() data.Partial() } } ``` +### Error data + +Obtaining data from an error can be done using an interface that looks something like this: + +```go +type ErrorData interface { + // Returns the underlying error + Error() error + + // Returns nil if a status is not available. + HTTPStatus() *int + + // Returns nil if a status is not available. + GRPCStatus() *status.Status + + // Returns nil if the error contains no retry information. + RetryableTraces() *RetryableTraces + + // Returns nil if the error contains no retry information. + RetryableMetrics() *RetryableMetrics + + // Returns nil if the error contains no retry information. + RetryableLogs() *RetryableLogs + + // Returns a count of failed records or nil if no partial success information was recorded. + Partial() *int +} + +type RetryableTraces struct {} + +func (r *RetryableTraces) Component() component.ID {} +func (r *RetryableTraces) GetData() ptrace.Traces {} + +// Returns nil if no delay was set, indicating to use the default. +// This makes it so a delay of `0` indicates to resend immediately. +func (r *RetryableTraces) Delay() *time.Duration {} +``` + ## Other considerations ### Signal conversion From ea959b6864ba3f25ed15a9e961471efc6fefca46 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 25 Jul 2024 17:57:19 -0400 Subject: [PATCH 11/37] Update design doc --- consumer/consumererror/README.md | 40 +++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index bb9c5b1535f..5eea7b3ff7c 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -46,7 +46,7 @@ Errors can be created by calling `consumererror.New(err, opts...)` where `err` is the underlying error, and `opts` is one of the provided options for supplying additional metadata: -- `consumererror.WithRetry[Signal]` +- `consumererror.WithRetry` - `consumererror.WithPartial` - `consumererror.WithGRPCStatus` - `consumererror.WithHTTPStatus` @@ -54,11 +54,15 @@ additional metadata: ### Retrying data submission +*NOTE*: This section is a draft and will be developed separately from the rest of this proposal. + If an error is transient, the `WithRetry` option corresponding to the relevant signal should be used to indicate that the error is retryable and to pass on any retry settings. These settings can come from the data sink or be determined by the component, such as through runtime conditions or from user settings. +The data for the retry will be provided by the component performing the retry. + **Note**: If retry information is not included in an error, the error will be considered permanent and will not be retried. @@ -147,7 +151,7 @@ if errors.As(err, &cerr) { for _, data := range errData { data.HTTPStatus() - data.RetryableTraces() + data.Retryable() data.Partial() } } @@ -169,33 +173,41 @@ type ErrorData interface { GRPCStatus() *status.Status // Returns nil if the error contains no retry information. - RetryableTraces() *RetryableTraces - - // Returns nil if the error contains no retry information. - RetryableMetrics() *RetryableMetrics - - // Returns nil if the error contains no retry information. - RetryableLogs() *RetryableLogs + Retryable() *Retryable // Returns a count of failed records or nil if no partial success information was recorded. Partial() *int } -type RetryableTraces struct {} +type Retryable struct {} -func (r *RetryableTraces) Component() component.ID {} -func (r *RetryableTraces) GetData() ptrace.Traces {} +func (r *Retryable) Component() component.ID {} // Returns nil if no delay was set, indicating to use the default. // This makes it so a delay of `0` indicates to resend immediately. -func (r *RetryableTraces) Delay() *time.Duration {} +func (r *Retryable) Delay() *time.Duration {} ``` ## Other considerations +### Mixed error classes + +When a receiver sees a mixture of permanent and retryable errors from downstream +in the pipeline, it must first consider whether retries are enabled within the +Collector. + +**Retries are enabled**: Ignore the permanent errors, retry data submission for +only components that indicated retryable errors. + +**Retries are disabled**: In an asynchronous pipeline, simply do not retry any +data. In a synchronous pipeline, the receiver should return a permanent error +code indicating to the caller that it should not retry the data submission. This +is intended to not induce extra failures where we know the data submission will +fail, but this behavior could be made configurable by the user. + ### Signal conversion -When converting between signals in a pipeline, it is expected that the component +When converting between signals in a pipeline, it is expected that the connector performing the conversion should perform the translation necessary in the error for any signal item counts. If the converted count cannot be determined, the full count of pre-converted signals should be returned. From fc40a8a051f401c179888ed6a079be91ed771dad Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:00:12 -0400 Subject: [PATCH 12/37] Add example --- consumer/consumererror/README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 5eea7b3ff7c..934404fcf2d 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -52,6 +52,18 @@ additional metadata: - `consumererror.WithHTTPStatus` - `consumererror.WithMetadata` +**Example**: + +```go +consumererror.New(err, + consumererror.WithRetry( + componentID, + consumerrerror.WithRetryDelay(10 * time.Second) + ), + consumererror.WithGRPCStatus(codes.InvalidArgument), +) +``` + ### Retrying data submission *NOTE*: This section is a draft and will be developed separately from the rest of this proposal. From 05d6fa91fc25ed10b40cf39b4709c18fd051214d Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 26 Jul 2024 10:07:54 -0400 Subject: [PATCH 13/37] Address PR feedback --- consumer/consumererror/README.md | 37 ++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 934404fcf2d..455e1d3ca2d 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -52,12 +52,30 @@ additional metadata: - `consumererror.WithHTTPStatus` - `consumererror.WithMetadata` +All options can be combined, we assume that the component knows what it is doing +when seemingly conflicting options. + +Two examples: + +- `WithRetry` and `WithPartial` are included together: Partial successes are + considered permanent errors in OTLP, which conflicts with making an error + retryable by including `WithRetry`. However, per our definition of what makes + a permanent error, this error has been marked as retryable, and therefore we + assume the component producing this error supports retyable partial success + errors. +- `WithGRPCStatus` and `WithHTTPStatus` are included together: While the + component likely only sent data over one of these transports, our errors will + produce the given status if it is included on the error, otherwise it will + translate a status from the status for the other transport. If both of these + are given, we assume the component wanted to perform its own status + conversion, and we will simply give the status for the requested transport + without performing any conversion. + **Example**: ```go consumererror.New(err, consumererror.WithRetry( - componentID, consumerrerror.WithRetryDelay(10 * time.Second) ), consumererror.WithGRPCStatus(codes.InvalidArgument), @@ -66,7 +84,8 @@ consumererror.New(err, ### Retrying data submission -*NOTE*: This section is a draft and will be developed separately from the rest of this proposal. +*NOTE*: This section is a draft and will be developed separately from the rest +of this proposal. If an error is transient, the `WithRetry` option corresponding to the relevant signal should be used to indicate that the error is retryable and to pass on any @@ -74,6 +93,13 @@ retry settings. These settings can come from the data sink or be determined by the component, such as through runtime conditions or from user settings. The data for the retry will be provided by the component performing the retry. +This will require all processing to be completely redone; in the future, +including data from the failed component so as to not retry this processing may +be made as an available option. + +To ensure only the failed pipeline branch is retried, the sequence of components +that created the error will be recorded by a pipeline utility as the error goes +back up the pipeline. **Note**: If retry information is not included in an error, the error will be considered permanent and will not be retried. @@ -82,14 +108,11 @@ considered permanent and will not be retried. ```go consumererror.WithRetry( - componentID, consumerrerror.WithRetryDelay(10 * time.Second) ) ``` -`componentID` should be the ID of the component creating the error, so it is -known which components to retry submission on. The delay is an optional setting -that can be provided if it is available. +The delay is an optional setting that can be provided if it is available. ### Indicating partial success @@ -143,7 +166,7 @@ special considerations may need to be made. ### Fanouts -When fanouts receive multiple errors, they will combine them with +When a fanout receives multiple errors, it will combine them with `(consumererror.Error).Combine(errs...)` and pass them upstream. The upstream component can then later pull all errors out for analysis. From d19438abddc958bfbd85b555352a5926e4edcfe2 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 26 Jul 2024 10:10:40 -0400 Subject: [PATCH 14/37] Include obsreport as an error consumer --- consumer/consumererror/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 455e1d3ca2d..11de0d08b63 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -40,6 +40,11 @@ retry queue by returning a retryable network code with relevant information. exporterhelper can use this information to manage the sending queue if it is enabled. Permanent errors will be forwarded back up the pipeline. +**obsreport**: Recording partial success information can ensure we correctly +track the number of failed telemetry records in the pipeline. Right now, all +records will be considered to be failed, which isn't accurate when partial +successes occur. + ## Creating Errors Errors can be created by calling `consumererror.New(err, opts...)` where `err` From d5f7a37c1f9997da592a610a980e7c47f1e4ccc1 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 26 Jul 2024 13:55:10 -0400 Subject: [PATCH 15/37] Draft implementation of error type --- consumer/consumererror/README.md | 2 - consumer/consumererror/error.go | 138 ++++++++++++++++++ consumer/consumererror/partial.go | 39 ----- consumer/consumererror/partial_test.go | 36 ----- consumer/consumererror/permanent.go | 2 +- consumer/consumererror/permanent_test.go | 12 -- consumer/consumererror/signalerrors.go | 28 +--- consumer/consumererror/transporterrors.go | 91 ------------ .../consumererror/transporterrors_test.go | 111 -------------- 9 files changed, 142 insertions(+), 317 deletions(-) create mode 100644 consumer/consumererror/error.go delete mode 100644 consumer/consumererror/partial.go delete mode 100644 consumer/consumererror/partial_test.go delete mode 100644 consumer/consumererror/transporterrors.go delete mode 100644 consumer/consumererror/transporterrors_test.go diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 11de0d08b63..95efd3e0858 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -221,8 +221,6 @@ type ErrorData interface { type Retryable struct {} -func (r *Retryable) Component() component.ID {} - // Returns nil if no delay was set, indicating to use the default. // This makes it so a delay of `0` indicates to resend immediately. func (r *Retryable) Delay() *time.Duration {} diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go new file mode 100644 index 00000000000..71478275154 --- /dev/null +++ b/consumer/consumererror/error.go @@ -0,0 +1,138 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" + +import ( + "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" + "google.golang.org/grpc/status" +) + +type Error struct { + errors []ErrorData +} + +type ErrorData interface { + Error() string + + Unwrap() error + + // Returns nil if a status is not available. + HTTPStatus() (int, bool) + + // Returns nil if a status is not available. + GRPCStatus() (*status.Status, bool) +} + +type errorData struct { + error + httpStatus *int + grpcStatus *status.Status + extraErrors []error +} + +// ErrorOption allows annotating an Error with metadata. +type ErrorOption func(error *errorData) + +// NewConsumerError wraps an error that happened while consuming telemetry +// and adds metadata onto it to be passed back up the pipeline. +func NewConsumerError(origErr error, options ...ErrorOption) error { + err := &Error{} + errData := &errorData{error: origErr} + + for _, option := range options { + option(errData) + } + + return err +} + +var _ error = &Error{} + +func (e *Error) Error() string { + return e.errors[len(e.errors)-1].Error() +} + +// Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. +func (e *Error) Unwrap() []error { + errors := make([]error, 0, len(e.errors)) + + for _, err := range e.errors { + errors = append(errors, err) + } + + return errors +} + +func (e *Error) Combine(errs ...error) { + for _, err := range errs { + if ne, ok := err.(*Error); ok { + e.errors = append(e.errors, ne.errors...) + } else { + e.errors = append(e.errors, &errorData{error: err}) + } + } +} + +func (e *Error) Data() []ErrorData { + return e.errors +} + +func WithHTTPStatus(status int) ErrorOption { + return func(err *errorData) { + err.httpStatus = &status + } +} + +func WithGRPCStatus(status *status.Status) ErrorOption { + return func(err *errorData) { + err.grpcStatus = status + } +} + +func WithMetadata(err error) ErrorOption { + return func(errData *errorData) { + errData.extraErrors = append(errData.extraErrors, err) + } +} + +var _ error = &errorData{} + +func (e *errorData) Error() string { + return e.error.Error() +} + +// Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. +func (e *errorData) Unwrap() error { + return e.error +} + +// HTTPStatus returns an HTTP status code either directly +// set by the source or derived from a gRPC status code set +// by the source. +// +// If no code has been set, the second return value is `false`. +func (ed *errorData) HTTPStatus() (int, bool) { + if ed.httpStatus != nil { + return *ed.httpStatus, true + } else if ed.grpcStatus != nil { + return statusconversion.GetHTTPStatusCodeFromStatus(ed.grpcStatus), true + } + + return 0, false +} + +// GRPCStatus returns an gRPC status code either directly +// set by the source or derived from an HTTP status code set +// by the source. +// +// If no code has been set, the second return value is `false`. +func (ed *errorData) GRPCStatus() (*status.Status, bool) { + if ed.grpcStatus != nil { + return ed.grpcStatus, true + } else if ed.httpStatus != nil { + return statusconversion.NewStatusFromMsgAndHTTPCode(ed.Error(), *ed.httpStatus), true + } + + return nil, false +} diff --git a/consumer/consumererror/partial.go b/consumer/consumererror/partial.go deleted file mode 100644 index b4e6de859b5..00000000000 --- a/consumer/consumererror/partial.go +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" - -// Partial describes situations where only some telemetry data was -// accepted. -// The semantics for this error are based on OTLP partial success -// messages. -// See: https://github.com/open-telemetry/opentelemetry-proto/blob/9d139c87b52669a3e2825b835dd828b57a455a55/docs/specification.md#partial-success -type Partial struct { - err error - count int -} - -var _ error = Partial{} - -// NewPartial instantiates a new Partial error. -// `count` should be a positive number representing -// the number of failed records, i.e. spans, data points, -// or log records. -func NewPartial(err error, count int) error { - return Partial{err: err, count: count} -} - -// Error returns a string representation of the underlying error. -func (p Partial) Error() string { - return "Partial success: " + p.err.Error() -} - -// Unwrap returns the wrapped error for functions Is and As in standard package errors. -func (p Partial) Unwrap() error { - return p.err -} - -// Count returns the count of rejected records. -func (p Partial) Count() int { - return p.count -} diff --git a/consumer/consumererror/partial_test.go b/consumer/consumererror/partial_test.go deleted file mode 100644 index 596bf4547e2..00000000000 --- a/consumer/consumererror/partial_test.go +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestPartial_Error(t *testing.T) { - e := errors.New("couldn't export") - pe := NewPartial(e, 3) - require.Contains(t, pe.Error(), e.Error()) -} - -func TestPartial_Unwrap(t *testing.T) { - var err error = testErrorType{"some error"} - se := NewPartial(err, 4) - joinedErr := errors.Join(errors.New("other error"), se) - target := testErrorType{} - require.NotEqual(t, err, target) - require.True(t, errors.As(joinedErr, &target)) - require.Equal(t, err, target) -} - -func TestPartial_Count(t *testing.T) { - count := 4 - e := NewPartial(errors.New("couldn't export"), count) - var pe Partial - ok := errors.As(e, &pe) - require.True(t, ok) - require.Equal(t, count, pe.Count()) -} diff --git a/consumer/consumererror/permanent.go b/consumer/consumererror/permanent.go index 87b03967524..30274cec195 100644 --- a/consumer/consumererror/permanent.go +++ b/consumer/consumererror/permanent.go @@ -36,5 +36,5 @@ func IsPermanent(err error) bool { if err == nil { return false } - return errors.As(err, &permanent{}) || errors.As(err, &Partial{}) || errors.As(err, &TransportError{}) + return errors.As(err, &permanent{}) } diff --git a/consumer/consumererror/permanent_test.go b/consumer/consumererror/permanent_test.go index e3761213639..779fa3450e3 100644 --- a/consumer/consumererror/permanent_test.go +++ b/consumer/consumererror/permanent_test.go @@ -23,18 +23,6 @@ func TestIsPermanent(t *testing.T) { err: NewPermanent(errors.New("testError")), permanent: true, }, - { - err: NewHTTP(errors.New("testError"), 500), - permanent: true, - }, - { - err: NewGRPC(errors.New("testError"), nil), - permanent: true, - }, - { - err: NewPartial(errors.New("testError"), 123), - permanent: true, - }, { err: fmt.Errorf("%w", NewPermanent(errors.New("testError"))), permanent: true, diff --git a/consumer/consumererror/signalerrors.go b/consumer/consumererror/signalerrors.go index 1d55d665616..6d79ae4b802 100644 --- a/consumer/consumererror/signalerrors.go +++ b/consumer/consumererror/signalerrors.go @@ -39,16 +39,6 @@ func (err retryable[V]) Delay() time.Duration { return err.delay } -// RetryableOption allows adding data to a retryable error, -// e.g. the duration before sending should be retried. -type RetryableOption func(err *retryableCommon) - -func WithRetryDelay(delay time.Duration) RetryableOption { - return func(err *retryableCommon) { - err.delay = delay - } -} - // Traces is an error that may carry associated Trace data for a subset of received data // that failed to be processed or sent. type Traces struct { @@ -56,7 +46,7 @@ type Traces struct { } // NewTraces creates a Traces that can encapsulate received data that failed to be processed or sent. -func NewTraces(err error, data ptrace.Traces, options ...RetryableOption) error { +func NewTraces(err error, data ptrace.Traces) error { t := Traces{ retryable: retryable[ptrace.Traces]{ retryableCommon: retryableCommon{ @@ -66,10 +56,6 @@ func NewTraces(err error, data ptrace.Traces, options ...RetryableOption) error }, } - for _, opt := range options { - opt(&t.retryableCommon) - } - return t } @@ -80,7 +66,7 @@ type Logs struct { } // NewLogs creates a Logs that can encapsulate received data that failed to be processed or sent. -func NewLogs(err error, data plog.Logs, options ...RetryableOption) error { +func NewLogs(err error, data plog.Logs) error { l := Logs{ retryable: retryable[plog.Logs]{ retryableCommon: retryableCommon{ @@ -90,10 +76,6 @@ func NewLogs(err error, data plog.Logs, options ...RetryableOption) error { }, } - for _, opt := range options { - opt(&l.retryableCommon) - } - return l } @@ -104,7 +86,7 @@ type Metrics struct { } // NewMetrics creates a Metrics that can encapsulate received data that failed to be processed or sent. -func NewMetrics(err error, data pmetric.Metrics, options ...RetryableOption) error { +func NewMetrics(err error, data pmetric.Metrics) error { m := Metrics{ retryable: retryable[pmetric.Metrics]{ retryableCommon: retryableCommon{ @@ -114,9 +96,5 @@ func NewMetrics(err error, data pmetric.Metrics, options ...RetryableOption) err }, } - for _, opt := range options { - opt(&m.retryableCommon) - } - return m } diff --git a/consumer/consumererror/transporterrors.go b/consumer/consumererror/transporterrors.go deleted file mode 100644 index 7c0498c375b..00000000000 --- a/consumer/consumererror/transporterrors.go +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" - -import ( - "fmt" - "net/http" - - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" - - "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" -) - -// TransportError contains either an HTTP or gRPC status code -// resulting from a failed synchronous network request. -// It allows retrieving a type of error code, converting -// between the status codes supported by each transport if necessary. -// -// It should be created with NewHTTP or NewGRPC. -type TransportError struct { - err error - httpStatus *int - grpcStatus *status.Status -} - -var _ error = TransportError{} - -// Error returns a string specifying the transport and corresponding status code. -func (se TransportError) Error() string { - if se.httpStatus != nil { - return fmt.Sprintf("HTTP Status (%d): %s", *se.httpStatus, se.err.Error()) - } else if se.grpcStatus != nil { - return fmt.Sprintf("gRPC Status (%s): %s", se.grpcStatus.Code().String(), se.err.Error()) - } - - return "Network error (no error code set): " + se.err.Error() -} - -func (se TransportError) Unwrap() error { - return se.err -} - -// HTTPStatus returns an HTTP status code either directly -// set by the source or derived from a gRPC status code set -// by the source. -// -// If no code has been set, the return value is -// an HTTP 500 code. -func (se TransportError) HTTPStatus() int { - if se.httpStatus != nil { - return *se.httpStatus - } else if se.grpcStatus != nil { - return statusconversion.GetHTTPStatusCodeFromStatus(se.grpcStatus) - } - - return http.StatusInternalServerError -} - -// GRPCStatus returns an gRPC status code either directly -// set by the source or derived from an HTTP status code set -// by the source. -// -// If no code has been set, the return value is set -// to `false`. -func (se TransportError) GRPCStatus() *status.Status { - if se.grpcStatus != nil { - return se.grpcStatus - } else if se.httpStatus != nil { - return statusconversion.NewStatusFromMsgAndHTTPCode(se.err.Error(), *se.httpStatus) - } - - return status.New(codes.Unknown, se.Error()) -} - -// NewHTTP wraps an error with a given HTTP status code. -func NewHTTP(err error, code int) error { - return TransportError{ - err: err, - httpStatus: &code, - } -} - -// NewGRPC wraps an error with a given gRPC status code. -func NewGRPC(err error, status *status.Status) error { - return TransportError{ - err: err, - grpcStatus: status, - } -} diff --git a/consumer/consumererror/transporterrors_test.go b/consumer/consumererror/transporterrors_test.go deleted file mode 100644 index 5d6d74abee4..00000000000 --- a/consumer/consumererror/transporterrors_test.go +++ /dev/null @@ -1,111 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" - -import ( - "errors" - "net/http" - "testing" - - "github.com/stretchr/testify/require" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" -) - -func TestTransportError_Error(t *testing.T) { - tests := []struct { - transportError error - msgContains string - validate func(t *testing.T, se TransportError) - }{ - { - transportError: NewHTTP(errors.New("httperror"), 400), - validate: func(t *testing.T, se TransportError) { - require.Contains(t, se.Error(), "400") - }, - }, - { - transportError: NewGRPC(errors.New("status"), status.New(codes.InvalidArgument, "")), - validate: func(t *testing.T, se TransportError) { - require.Contains(t, se.Error(), "InvalidArgument") - }, - }, - { - transportError: NewGRPC(errors.New("nil"), nil), - validate: func(t *testing.T, se TransportError) { - require.NotNil(t, se.Error()) - }, - }, - } - for _, tt := range tests { - var se TransportError - require.True(t, errors.As(tt.transportError, &se)) - tt.validate(t, se) - } -} - -func TestTransportError_Unwrap(t *testing.T) { - var err error = testErrorType{"some error"} - se := NewHTTP(err, 400) - joinedErr := errors.Join(errors.New("other error"), se) - target := testErrorType{} - require.NotEqual(t, err, target) - require.True(t, errors.As(joinedErr, &target)) - require.Equal(t, err, target) -} - -func TestTransportError_GRPCStatus(t *testing.T) { - tests := []struct { - transportError error - code codes.Code - }{ - { - transportError: NewHTTP(errors.New("httperror"), 429), - code: codes.ResourceExhausted, - }, - { - transportError: NewGRPC(errors.New("status"), status.New(codes.ResourceExhausted, "")), - code: codes.ResourceExhausted, - }, - { - transportError: NewGRPC(errors.New("nil"), nil), - code: codes.Unknown, - }, - } - for _, tt := range tests { - var se TransportError - require.True(t, errors.As(tt.transportError, &se)) - status := se.GRPCStatus() - require.Equal(t, tt.code, status.Code()) - } -} - -func TestTransportError_HTTPStatus(t *testing.T) { - tests := []struct { - transportError error - code int - ok bool - }{ - { - transportError: NewHTTP(errors.New("httperror"), http.StatusTooManyRequests), - code: http.StatusTooManyRequests, - ok: true, - }, - { - transportError: NewGRPC(errors.New("status"), status.New(codes.ResourceExhausted, "")), - code: http.StatusTooManyRequests, - ok: true, - }, - { - transportError: NewGRPC(errors.New("nil"), nil), - code: http.StatusInternalServerError, - }, - } - for _, tt := range tests { - var se TransportError - require.True(t, errors.As(tt.transportError, &se)) - code := se.HTTPStatus() - require.Equal(t, tt.code, code) - } -} From 9fe7880d4ef1e97c67a35ada93d9f81141d5fc8b Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 26 Jul 2024 13:58:27 -0400 Subject: [PATCH 16/37] Update --- consumer/consumererror/component.go | 38 ------------------------ consumer/consumererror/component_test.go | 38 ------------------------ consumer/consumertest/go.mod | 6 ++-- consumer/consumertest/go.sum | 12 ++++---- consumer/go.mod | 6 ---- consumer/go.sum | 12 -------- 6 files changed, 9 insertions(+), 103 deletions(-) delete mode 100644 consumer/consumererror/component.go delete mode 100644 consumer/consumererror/component_test.go diff --git a/consumer/consumererror/component.go b/consumer/consumererror/component.go deleted file mode 100644 index c8fc91289fa..00000000000 --- a/consumer/consumererror/component.go +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" - -import "go.opentelemetry.io/collector/component" - -// Component contains information about the component that returned an -// error. This is helpful when reasoning about the context an error -// occurred in when handling errors in upstream components within a -// pipeline. -type Component struct { - err error - id component.ID -} - -var _ error = Component{} - -// NewComponent instantiates a new Component error. -// `id` should be the component's component.ID object. -func NewComponent(err error, id component.ID) error { - return Component{err: err, id: id} -} - -// Error returns a string representation of the underlying error. -func (c Component) Error() string { - return c.id.String() + " returned an error: " + c.err.Error() -} - -// Unwrap returns the wrapped error for functions Is and As in standard package errors. -func (c Component) Unwrap() error { - return c.err -} - -// Count returns the count of rejected records. -func (c Component) ID() component.ID { - return c.id -} diff --git a/consumer/consumererror/component_test.go b/consumer/consumererror/component_test.go deleted file mode 100644 index 9029b497ef0..00000000000 --- a/consumer/consumererror/component_test.go +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/require" - - "go.opentelemetry.io/collector/component" -) - -func TestComponent_Error(t *testing.T) { - e := errors.New("couldn't export") - pe := NewComponent(e, component.MustNewID("nop")) - require.Contains(t, pe.Error(), e.Error()) -} - -func TestComponent_Unwrap(t *testing.T) { - var err error = testErrorType{"some error"} - ce := NewComponent(err, component.MustNewIDWithName("nop", "test")) - joinedErr := errors.Join(errors.New("other error"), ce) - target := testErrorType{} - require.NotEqual(t, err, target) - require.True(t, errors.As(joinedErr, &target)) - require.Equal(t, err, target) -} - -func TestComponent_ID(t *testing.T) { - id := component.MustNewIDWithName("nop", "test") - e := NewComponent(errors.New("couldn't export"), id) - var ce Component - ok := errors.As(e, &ce) - require.True(t, ok) - require.Equal(t, id, ce.ID()) -} diff --git a/consumer/consumertest/go.mod b/consumer/consumertest/go.mod index 24f5ecec4a0..e42ac0fbf64 100644 --- a/consumer/consumertest/go.mod +++ b/consumer/consumertest/go.mod @@ -22,9 +22,9 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect go.uber.org/multierr v1.11.0 // indirect - golang.org/x/net v0.25.0 // indirect - golang.org/x/sys v0.20.0 // indirect - golang.org/x/text v0.15.0 // indirect + golang.org/x/net v0.26.0 // indirect + golang.org/x/sys v0.21.0 // indirect + golang.org/x/text v0.16.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240528184218-531527333157 // indirect google.golang.org/grpc v1.65.0 // indirect google.golang.org/protobuf v1.34.2 // indirect diff --git a/consumer/consumertest/go.sum b/consumer/consumertest/go.sum index 528166b78c0..4c9c65b576f 100644 --- a/consumer/consumertest/go.sum +++ b/consumer/consumertest/go.sum @@ -42,20 +42,20 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= -golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= +golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ= +golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= -golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws= +golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk= -golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= +golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= diff --git a/consumer/go.mod b/consumer/go.mod index 9644877419e..bd7ff7fc242 100644 --- a/consumer/go.mod +++ b/consumer/go.mod @@ -4,7 +4,6 @@ go 1.21.0 require ( github.com/stretchr/testify v1.9.0 - go.opentelemetry.io/collector/component v0.105.0 go.opentelemetry.io/collector/pdata v1.12.0 go.opentelemetry.io/collector/pdata/testdata v0.105.0 go.uber.org/goleak v1.3.0 @@ -18,13 +17,8 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - go.opentelemetry.io/collector/config/configtelemetry v0.105.0 // indirect go.opentelemetry.io/collector/pdata/pprofile v0.105.0 // indirect - go.opentelemetry.io/otel v1.28.0 // indirect - go.opentelemetry.io/otel/metric v1.28.0 // indirect - go.opentelemetry.io/otel/trace v1.28.0 // indirect go.uber.org/multierr v1.11.0 // indirect - go.uber.org/zap v1.27.0 // indirect golang.org/x/net v0.26.0 // indirect golang.org/x/sys v0.21.0 // indirect golang.org/x/text v0.16.0 // indirect diff --git a/consumer/go.sum b/consumer/go.sum index 48f14e6d679..4c9c65b576f 100644 --- a/consumer/go.sum +++ b/consumer/go.sum @@ -1,10 +1,6 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY= -github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= -github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= -github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= @@ -33,18 +29,10 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= -go.opentelemetry.io/otel v1.28.0 h1:/SqNcYk+idO0CxKEUOtKQClMK/MimZihKYMruSMViUo= -go.opentelemetry.io/otel v1.28.0/go.mod h1:q68ijF8Fc8CnMHKyzqL6akLO46ePnjkgfIMIjUIX9z4= -go.opentelemetry.io/otel/metric v1.28.0 h1:f0HGvSl1KRAU1DLgLGFjrwVyismPlnuU6JD6bOeuA5Q= -go.opentelemetry.io/otel/metric v1.28.0/go.mod h1:Fb1eVBFZmLVTMb6PPohq3TO9IIhUisDsbJoL/+uQW4s= -go.opentelemetry.io/otel/trace v1.28.0 h1:GhQ9cUuQGmNDd5BTCP2dAvv75RdMxEfTmYejp+lkx9g= -go.opentelemetry.io/otel/trace v1.28.0/go.mod h1:jPyXzNPg6da9+38HEwElrQiHlVMTnVfM3/yv2OlIHaI= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= -go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8= -go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= From 53cbc05d3430126385aa6d6f6b702ab54d23e91f Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 26 Jul 2024 14:03:13 -0400 Subject: [PATCH 17/37] Improve metadata --- consumer/consumererror/error.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 71478275154..1676a176abd 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -26,9 +26,9 @@ type ErrorData interface { type errorData struct { error - httpStatus *int - grpcStatus *status.Status - extraErrors []error + httpStatus *int + grpcStatus *status.Status + metadata []any } // ErrorOption allows annotating an Error with metadata. @@ -90,9 +90,9 @@ func WithGRPCStatus(status *status.Status) ErrorOption { } } -func WithMetadata(err error) ErrorOption { +func WithMetadata(data any) ErrorOption { return func(errData *errorData) { - errData.extraErrors = append(errData.extraErrors, err) + errData.metadata = append(errData.metadata, data) } } @@ -136,3 +136,8 @@ func (ed *errorData) GRPCStatus() (*status.Status, bool) { return nil, false } + +// Returns any extra user-defined metadata that has been propagated upstream. +func (ed *errorData) Metadata() []any { + return ed.metadata +} From 0d5af43affea91f4eb8ec783e47f72e65ea53df9 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 26 Jul 2024 14:03:34 -0400 Subject: [PATCH 18/37] Add metadata to interface --- consumer/consumererror/error.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 1676a176abd..6216ef93a24 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -22,6 +22,8 @@ type ErrorData interface { // Returns nil if a status is not available. GRPCStatus() (*status.Status, bool) + + Metadata() []any } type errorData struct { From 2aa9e86d8665b2b903692fde669a6b1690e5b915 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 30 Jul 2024 15:00:08 -0400 Subject: [PATCH 19/37] Address PR feedback --- consumer/consumererror/README.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 95efd3e0858..6ffaa5b66ee 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -21,7 +21,9 @@ as retryable. perform retries. **Indicating partial success**: Errors can indicate that not all items were -accepted, for example as in an OTLP partial success message. +accepted, for example as in an OTLP partial success message. OTLP backends +will return failed item counts if a partial success occurs, and this information +can be propagated up to a receiver and returned to the caller. **Communicating network error codes**: Errors should allow embedding information necessary for the Collector to act as a proxy for a backend, i.e. relay a status @@ -36,6 +38,11 @@ may entail either keeping the retry queue inside of the Collector by having the receiver keep track of retries, or may involve having the caller manage the retry queue by returning a retryable network code with relevant information. +**scraperhelper**: The scraper helper can use information about errors from +downstream to affect scraping. For example, if the backend is struggling with +the volume of data, scraping could be slowed, or the amount of data collected +could be reduced. + **exporterhelper**: When an exporter returns a retryable error, the exporterhelper can use this information to manage the sending queue if it is enabled. Permanent errors will be forwarded back up the pipeline. From c8c40ef1d2ae845c8d098f1cd40130347ad5dabb Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 5 Aug 2024 16:13:30 -0400 Subject: [PATCH 20/37] Address PR feedback --- consumer/consumererror/README.md | 62 +++++++++++++++++++++----------- consumer/consumererror/error.go | 20 +++-------- 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 6ffaa5b66ee..0e48719c68d 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -2,9 +2,9 @@ This package contains error types that should be returned by a consumer when an error occurs while processing telemetry. The error types included in this -package provide functionality for communicating details about -the error for use upstream in the pipeline. Ideally the error returned by a -component in its `consume` function should be from this package. +package provide functionality for communicating details about the error for use +upstream in the pipeline. Ideally the error returned by a component in its +`consume` function should be from this package. ## Error classes @@ -21,9 +21,9 @@ as retryable. perform retries. **Indicating partial success**: Errors can indicate that not all items were -accepted, for example as in an OTLP partial success message. OTLP backends -will return failed item counts if a partial success occurs, and this information -can be propagated up to a receiver and returned to the caller. +accepted, for example as in an OTLP partial success message. OTLP backends will +return failed item counts if a partial success occurs, and this information can +be propagated up to a receiver and returned to the caller. **Communicating network error codes**: Errors should allow embedding information necessary for the Collector to act as a proxy for a backend, i.e. relay a status @@ -58,10 +58,14 @@ Errors can be created by calling `consumererror.New(err, opts...)` where `err` is the underlying error, and `opts` is one of the provided options for supplying additional metadata: -- `consumererror.WithRetry` -- `consumererror.WithPartial` - `consumererror.WithGRPCStatus` - `consumererror.WithHTTPStatus` + +The following options are not currently available, but may be made available in +the future: + +- `consumererror.WithRetry` +- `consumererror.WithPartial` - `consumererror.WithMetadata` All options can be combined, we assume that the component knows what it is doing @@ -96,8 +100,8 @@ consumererror.New(err, ### Retrying data submission -*NOTE*: This section is a draft and will be developed separately from the rest -of this proposal. +> [!WARNING] This function is currently in the design phase. It is not available +> and may not be added. The below is a design describing how this may work. If an error is transient, the `WithRetry` option corresponding to the relevant signal should be used to indicate that the error is retryable and to pass on any @@ -128,6 +132,9 @@ The delay is an optional setting that can be provided if it is available. ### Indicating partial success +> [!WARNING] This function is currently in the design phase. It is not available +> and may not be added. The below is a design describing how this may work. + If the component receives an OTLP partial success message (or other indication of partial success), it should include this information with a count of the failed records. @@ -159,6 +166,9 @@ consumererror.WithHTTPStatus(http.StatusTooManyRequests) ### Including custom data +> [!WARNING] This function is currently in the design phase. It is not available +> and may not be added. The below is a design describing how this may work. + Custom data can be included as well for any additional information that needs to be propagated back up the pipeline. It is up to the consuming component if or how this data will be used. @@ -184,6 +194,9 @@ component can then later pull all errors out for analysis. ### Retrieving errors +> [!WARNING] This functionality is currently experimental, and the description +> here is for design purposes. The code snippet may not work as-written. + When a receiver gets a response that includes an error, it can get the data out by doing something similar to the following. Note that this uses the `ErrorData` type, which is for reading error data, as opposed to the `Error` type, which is @@ -206,24 +219,29 @@ if errors.As(err, &cerr) { ### Error data -Obtaining data from an error can be done using an interface that looks something like this: +> [!WARNING] The description below is a design proposal for how this +> functionality may work. See `error.go` within this package for the current +> functionality. + +Obtaining data from an error can be done using an interface that looks something +like this: ```go type ErrorData interface { // Returns the underlying error Error() error - // Returns nil if a status is not available. - HTTPStatus() *int + // Second argument is `false` if no code is available. + HTTPStatus() (int, bool) - // Returns nil if a status is not available. - GRPCStatus() *status.Status + // Second argument is `false` if no code is available. + GRPCStatus() (*status.Status, bool) - // Returns nil if the error contains no retry information. - Retryable() *Retryable + // Second argument is `false` if no retry information is available. + Retryable() (Retryable, bool) - // Returns a count of failed records or nil if no partial success information was recorded. - Partial() *int + // Second argument is `false` if no partial counts were recorded. + Partial() (Partial, bool) } type Retryable struct {} @@ -231,6 +249,8 @@ type Retryable struct {} // Returns nil if no delay was set, indicating to use the default. // This makes it so a delay of `0` indicates to resend immediately. func (r *Retryable) Delay() *time.Duration {} + +type Partial struct {} ``` ## Other considerations @@ -254,8 +274,8 @@ fail, but this behavior could be made configurable by the user. When converting between signals in a pipeline, it is expected that the connector performing the conversion should perform the translation necessary in the error -for any signal item counts. If the converted count cannot be determined, the full -count of pre-converted signals should be returned. +for any signal item counts. If the converted count cannot be determined, the +full count of pre-converted signals should be returned. ### Asynchronous processing diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 6216ef93a24..57194b32125 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -17,20 +17,19 @@ type ErrorData interface { Unwrap() error - // Returns nil if a status is not available. + // Returns a status code and a boolean indicating whether a status was set + // by the error creator. HTTPStatus() (int, bool) - // Returns nil if a status is not available. + // Returns a status code and a boolean indicating whether a status was set + // by the error creator. GRPCStatus() (*status.Status, bool) - - Metadata() []any } type errorData struct { error httpStatus *int grpcStatus *status.Status - metadata []any } // ErrorOption allows annotating an Error with metadata. @@ -92,12 +91,6 @@ func WithGRPCStatus(status *status.Status) ErrorOption { } } -func WithMetadata(data any) ErrorOption { - return func(errData *errorData) { - errData.metadata = append(errData.metadata, data) - } -} - var _ error = &errorData{} func (e *errorData) Error() string { @@ -138,8 +131,3 @@ func (ed *errorData) GRPCStatus() (*status.Status, bool) { return nil, false } - -// Returns any extra user-defined metadata that has been propagated upstream. -func (ed *errorData) Metadata() []any { - return ed.metadata -} From c26e31a7067020df2030078b922486e3c0305fd6 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 5 Aug 2024 16:15:12 -0400 Subject: [PATCH 21/37] Update consumer/consumererror/README.md Co-authored-by: Dmitrii Anoshin --- consumer/consumererror/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 0e48719c68d..f1e9ed252c6 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -44,8 +44,8 @@ the volume of data, scraping could be slowed, or the amount of data collected could be reduced. **exporterhelper**: When an exporter returns a retryable error, the -exporterhelper can use this information to manage the sending queue if it is -enabled. Permanent errors will be forwarded back up the pipeline. +exporterhelper can use this information to retry. Permanent errors will be +forwarded back up the pipeline. **obsreport**: Recording partial success information can ensure we correctly track the number of failed telemetry records in the pipeline. Right now, all From 3aadc29c90ebc113766e20a688c82d64649cd863 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 5 Aug 2024 16:29:21 -0400 Subject: [PATCH 22/37] Address PR feedback --- consumer/consumererror/README.md | 22 ++++++++++++++++++++++ consumer/consumererror/error.go | 4 ++++ 2 files changed, 26 insertions(+) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index f1e9ed252c6..1c6bac5b237 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -282,3 +282,25 @@ full count of pre-converted signals should be returned. The use of any components that do asynchronous processing in a pipeline will cut off error backpropagation at the asynchronous component. The asynchronous component may communicate error information using the Collector's own signals. + +## Transitioning + +> [!WARNING] This functionality is currently in the design phase. It is not +> available and may not be added. The below is a design describing how this may +> work. + +The following describes how to transition to these error types: + +- `NewPermanent`: To transition to new permanent errors, call + `consumererror.New` with the relevant metadata included in the invocation. + Errors will be permanent by default going forward. +- `New[Traces|Metrics|Logs]`: These functions will be deprecated in favor of + having the caller provide the data to retry. Current uses can invoke + `consumererror.New` with the `WithRetry` option to retry a request. +- `exporterhelper.NewThrottleRetry`: This will be replaced with `WithRetry`, and + can follow a similar approach as above. + +`consumererror.IsPermanent` will be deprecated in favor of checking whether +retry information is available, and only retrying if it has been provided. This +will be possible by calling `ErrorData.Retryable()` and checking for retry +information. diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 57194b32125..2ba59da6a15 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -24,6 +24,10 @@ type ErrorData interface { // Returns a status code and a boolean indicating whether a status was set // by the error creator. GRPCStatus() (*status.Status, bool) + + // Disallow implementations outside this package to allow later extending + // the interface. + unexported() } type errorData struct { From 6f85be236071056368f862a0fea2b6ba5789838b Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 7 Aug 2024 11:00:29 -0400 Subject: [PATCH 23/37] Fix interface implementation --- consumer/consumererror/error.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 2ba59da6a15..ac8de6be323 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -97,6 +97,8 @@ func WithGRPCStatus(status *status.Status) ErrorOption { var _ error = &errorData{} +func (e *errorData) unexported() {} + func (e *errorData) Error() string { return e.error.Error() } From b55f661a34c45d64c0ced99bb7e49dab0ec7e9d9 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 7 Aug 2024 11:28:56 -0400 Subject: [PATCH 24/37] go mod tidy --- client/go.mod | 6 +++--- client/go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/client/go.mod b/client/go.mod index 94d4defb837..5968a8f3978 100644 --- a/client/go.mod +++ b/client/go.mod @@ -18,9 +18,9 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect go.uber.org/multierr v1.11.0 // indirect - golang.org/x/net v0.25.0 // indirect - golang.org/x/sys v0.20.0 // indirect - golang.org/x/text v0.15.0 // indirect + golang.org/x/net v0.26.0 // indirect + golang.org/x/sys v0.21.0 // indirect + golang.org/x/text v0.16.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240528184218-531527333157 // indirect google.golang.org/grpc v1.65.0 // indirect google.golang.org/protobuf v1.34.2 // indirect diff --git a/client/go.sum b/client/go.sum index 2d7de4069f1..5b43b909021 100644 --- a/client/go.sum +++ b/client/go.sum @@ -43,20 +43,20 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= -golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= +golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ= +golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= -golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws= +golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk= -golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= +golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= From ef40c0df54776c2df699932dfd015795994ec6e9 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 7 Aug 2024 11:41:16 -0400 Subject: [PATCH 25/37] Reorder imports --- consumer/consumererror/error.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index ac8de6be323..46464e5d2c6 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -4,8 +4,9 @@ package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" import ( - "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" "google.golang.org/grpc/status" + + "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" ) type Error struct { From dba92c6a624fb7439347072512844354c6a099d3 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 7 Aug 2024 13:58:23 -0400 Subject: [PATCH 26/37] make crosslink --- consumer/consumerprofiles/go.mod | 4 ---- consumer/go.mod | 4 ---- 2 files changed, 8 deletions(-) diff --git a/consumer/consumerprofiles/go.mod b/consumer/consumerprofiles/go.mod index 82f56d509c9..527a3f5d706 100644 --- a/consumer/consumerprofiles/go.mod +++ b/consumer/consumerprofiles/go.mod @@ -33,7 +33,3 @@ require ( ) replace go.opentelemetry.io/collector/pdata/testdata => ../../pdata/testdata - -replace go.opentelemetry.io/collector/config/configtelemetry => ../../config/configtelemetry - -replace go.opentelemetry.io/collector/component => ../../component diff --git a/consumer/go.mod b/consumer/go.mod index 9da571d8949..8b29ecc82a6 100644 --- a/consumer/go.mod +++ b/consumer/go.mod @@ -37,7 +37,3 @@ retract ( v0.76.0 // Depends on retracted pdata v1.0.0-rc10 module, use v0.76.1 v0.69.0 // Release failed, use v0.69.1 ) - -replace go.opentelemetry.io/collector/config/configtelemetry => ../config/configtelemetry - -replace go.opentelemetry.io/collector/component => ../component From dd69f4a6648eb8f186c761e041fb15614bd687fc Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 7 Aug 2024 14:16:04 -0400 Subject: [PATCH 27/37] Fix CI --- consumer/consumererror/error.go | 17 +++++--- consumer/consumererror/permanent_test.go | 48 ++++++++------------- consumer/consumererror/signalerrors.go | 47 +++++--------------- consumer/consumererror/signalerrors_test.go | 42 ------------------ 4 files changed, 38 insertions(+), 116 deletions(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 46464e5d2c6..9e3ae1da97e 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -4,6 +4,8 @@ package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" import ( + "errors" + "google.golang.org/grpc/status" "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" @@ -72,8 +74,9 @@ func (e *Error) Unwrap() []error { func (e *Error) Combine(errs ...error) { for _, err := range errs { - if ne, ok := err.(*Error); ok { - e.errors = append(e.errors, ne.errors...) + var otherErr *Error + if errors.As(err, &otherErr) { + e.errors = append(e.errors, otherErr.errors...) } else { e.errors = append(e.errors, &errorData{error: err}) } @@ -98,15 +101,15 @@ func WithGRPCStatus(status *status.Status) ErrorOption { var _ error = &errorData{} -func (e *errorData) unexported() {} +func (ed *errorData) unexported() {} -func (e *errorData) Error() string { - return e.error.Error() +func (ed *errorData) Error() string { + return ed.error.Error() } // Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. -func (e *errorData) Unwrap() error { - return e.error +func (ed *errorData) Unwrap() error { + return ed.error } // HTTPStatus returns an HTTP status code either directly diff --git a/consumer/consumererror/permanent_test.go b/consumer/consumererror/permanent_test.go index 779fa3450e3..a8291db0496 100644 --- a/consumer/consumererror/permanent_test.go +++ b/consumer/consumererror/permanent_test.go @@ -10,40 +10,28 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "go.opentelemetry.io/collector/pdata/ptrace" ) +type testErrorType struct { + s string +} + +func (t testErrorType) Error() string { + return "" +} + func TestIsPermanent(t *testing.T) { - tests := []struct { - err error - permanent bool - }{ - { - err: NewPermanent(errors.New("testError")), - permanent: true, - }, - { - err: fmt.Errorf("%w", NewPermanent(errors.New("testError"))), - permanent: true, - }, - { - err: errors.New("testError"), - permanent: false, - }, - { - err: NewTraces(errors.New("testError"), ptrace.Traces{}), - permanent: false, - }, - { - err: NewTraces(NewPermanent(errors.New("testError")), ptrace.Traces{}), - permanent: true, - }, - } + var err error + assert.False(t, IsPermanent(err)) + + err = errors.New("testError") + assert.False(t, IsPermanent(err)) + + err = NewPermanent(err) + assert.True(t, IsPermanent(err)) - for _, tt := range tests { - assert.Equal(t, tt.permanent, IsPermanent(tt.err)) - } + err = fmt.Errorf("%w", err) + assert.True(t, IsPermanent(err)) } func TestPermanent_Unwrap(t *testing.T) { diff --git a/consumer/consumererror/signalerrors.go b/consumer/consumererror/signalerrors.go index 6d79ae4b802..1d7558ce1ca 100644 --- a/consumer/consumererror/signalerrors.go +++ b/consumer/consumererror/signalerrors.go @@ -4,25 +4,16 @@ package consumererror // import "go.opentelemetry.io/collector/consumer/consumererror" import ( - "time" - "go.opentelemetry.io/collector/pdata/plog" "go.opentelemetry.io/collector/pdata/pmetric" "go.opentelemetry.io/collector/pdata/ptrace" ) -type retryableCommon struct { - error - delay time.Duration -} - type retryable[V ptrace.Traces | pmetric.Metrics | plog.Logs] struct { - retryableCommon + error data V } -var _ error = retryable[ptrace.Traces]{} - // Unwrap returns the wrapped error for functions Is and As in standard package errors. func (err retryable[V]) Unwrap() error { return err.error @@ -33,12 +24,6 @@ func (err retryable[V]) Data() V { return err.data } -// Delay returns the time duration before the telemetry data should -// be resent to the destination. -func (err retryable[V]) Delay() time.Duration { - return err.delay -} - // Traces is an error that may carry associated Trace data for a subset of received data // that failed to be processed or sent. type Traces struct { @@ -47,16 +32,12 @@ type Traces struct { // NewTraces creates a Traces that can encapsulate received data that failed to be processed or sent. func NewTraces(err error, data ptrace.Traces) error { - t := Traces{ + return Traces{ retryable: retryable[ptrace.Traces]{ - retryableCommon: retryableCommon{ - error: err, - }, - data: data, + error: err, + data: data, }, } - - return t } // Logs is an error that may carry associated Log data for a subset of received data @@ -67,16 +48,12 @@ type Logs struct { // NewLogs creates a Logs that can encapsulate received data that failed to be processed or sent. func NewLogs(err error, data plog.Logs) error { - l := Logs{ + return Logs{ retryable: retryable[plog.Logs]{ - retryableCommon: retryableCommon{ - error: err, - }, - data: data, + error: err, + data: data, }, } - - return l } // Metrics is an error that may carry associated Metrics data for a subset of received data @@ -87,14 +64,10 @@ type Metrics struct { // NewMetrics creates a Metrics that can encapsulate received data that failed to be processed or sent. func NewMetrics(err error, data pmetric.Metrics) error { - m := Metrics{ + return Metrics{ retryable: retryable[pmetric.Metrics]{ - retryableCommon: retryableCommon{ - error: err, - }, - data: data, + error: err, + data: data, }, } - - return m } diff --git a/consumer/consumererror/signalerrors_test.go b/consumer/consumererror/signalerrors_test.go index fbc013a424e..475d3173bb9 100644 --- a/consumer/consumererror/signalerrors_test.go +++ b/consumer/consumererror/signalerrors_test.go @@ -6,7 +6,6 @@ package consumererror import ( "errors" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -14,14 +13,6 @@ import ( "go.opentelemetry.io/collector/pdata/testdata" ) -type testErrorType struct { - s string -} - -func (t testErrorType) Error() string { - return "" -} - func TestTraces(t *testing.T) { td := testdata.GenerateTraces(1) err := errors.New("some error") @@ -46,17 +37,6 @@ func TestTraces_Unwrap(t *testing.T) { require.Equal(t, err, target) } -func TestTraces_Delay(t *testing.T) { - delay := 4 * time.Second - td := testdata.GenerateTraces(1) - // Wrapping err with error Traces. - err := NewTraces(errors.New("some error"), td, WithRetryDelay(delay)) - var traceErr Traces - // Unwrapping traceErr for err and assigning to target. - require.True(t, errors.As(err, &traceErr)) - require.Equal(t, traceErr.Delay(), delay) -} - func TestLogs(t *testing.T) { td := testdata.GenerateLogs(1) err := errors.New("some error") @@ -81,17 +61,6 @@ func TestLogs_Unwrap(t *testing.T) { require.Equal(t, err, target) } -func TestLogs_Delay(t *testing.T) { - delay := 4 * time.Second - td := testdata.GenerateLogs(1) - // Wrapping err with error Logs. - err := NewLogs(errors.New("some error"), td, WithRetryDelay(delay)) - var logErr Logs - // Unwrapping LogErr for err and assigning to target. - require.True(t, errors.As(err, &logErr)) - require.Equal(t, logErr.Delay(), delay) -} - func TestMetrics(t *testing.T) { td := testdata.GenerateMetrics(1) err := errors.New("some error") @@ -115,14 +84,3 @@ func TestMetrics_Unwrap(t *testing.T) { require.True(t, errors.As(metricErr, &target)) require.Equal(t, err, target) } - -func TestMetrics_Delay(t *testing.T) { - delay := 4 * time.Second - td := testdata.GenerateMetrics(1) - // Wrapping err with error Metrics. - err := NewMetrics(errors.New("some error"), td, WithRetryDelay(delay)) - var metricErr Metrics - // Unwrapping MetricErr for err and assigning to target. - require.True(t, errors.As(err, &metricErr)) - require.Equal(t, metricErr.Delay(), delay) -} From 7300bda61eceb6b9bc24719b1a27bc9bcf2d2923 Mon Sep 17 00:00:00 2001 From: Evan Bradley Date: Thu, 8 Aug 2024 16:03:23 -0400 Subject: [PATCH 28/37] Polish and tests --- consumer/consumererror/error.go | 54 +++++- consumer/consumererror/error_test.go | 248 +++++++++++++++++++++++++++ 2 files changed, 293 insertions(+), 9 deletions(-) create mode 100644 consumer/consumererror/error_test.go diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 9e3ae1da97e..56b30d9c727 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -11,10 +11,26 @@ import ( "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" ) +// Error acts as a container for errors coming from pipeline components. +// It may hold multiple errors from downstream components, and is designed +// to act as a way to accumulate errors as it travels upstream in a pipeline. +// `Error` should be obtained using `errors.As` and as a result, ideally +// a single instance should exist in an error stack. If this is not possible, +// the most `Error` object should be highest on the stack. +// +// Experimental: This API is at the early stage of development and may change without backward compatibility type Error struct { errors []ErrorData } +// ErrorData is intended to be used to encapsulate various information +// that can add context to an error that occurred within a pipeline component. +// ErrorData objects are constructed through calling `New` with the relevant +// options to capture data around the error that occurred. It can then be pulled +// out by an upstream component by calling `Error.Data`. +// +// Experimental: This API is at the early stage of development and may change without backward compatibility + type ErrorData interface { Error() string @@ -42,11 +58,13 @@ type errorData struct { // ErrorOption allows annotating an Error with metadata. type ErrorOption func(error *errorData) -// NewConsumerError wraps an error that happened while consuming telemetry +// New wraps an error that happened while consuming telemetry // and adds metadata onto it to be passed back up the pipeline. -func NewConsumerError(origErr error, options ...ErrorOption) error { - err := &Error{} +// +// Experimental: This API is at the early stage of development and may change without backward compatibility +func New(origErr error, options ...ErrorOption) error { errData := &errorData{error: origErr} + err := &Error{errors: []ErrorData{errData}} for _, option := range options { option(errData) @@ -57,6 +75,7 @@ func NewConsumerError(origErr error, options ...ErrorOption) error { var _ error = &Error{} +// Error implements the `error` interface. func (e *Error) Error() string { return e.errors[len(e.errors)-1].Error() } @@ -72,7 +91,19 @@ func (e *Error) Unwrap() []error { return errors } -func (e *Error) Combine(errs ...error) { +// Data returns all the accumulated ErrorData errors +// emitted by components downstream in the pipeline. +// These can then be aggregated or worked with individually. +func (e *Error) Data() []ErrorData { + return e.errors +} + +// Combine joins errors that occur at a fanout into a single +// `Error` object. The component that created the data submission +// can then work with the `Error` object to understand the failure. +func Combine(errs ...error) *Error { + e := &Error{errors: make([]ErrorData, 0, len(errs))} + for _, err := range errs { var otherErr *Error if errors.As(err, &otherErr) { @@ -81,18 +112,20 @@ func (e *Error) Combine(errs ...error) { e.errors = append(e.errors, &errorData{error: err}) } } -} -func (e *Error) Data() []ErrorData { - return e.errors + return e } +// WithHTTPStatus records an HTTP status code that was received +// from a server during data submission. func WithHTTPStatus(status int) ErrorOption { return func(err *errorData) { err.httpStatus = &status } } +// WithGRPCStatus records a gRPC status code that was received +// from a server during data submission. func WithGRPCStatus(status *status.Status) ErrorOption { return func(err *errorData) { err.grpcStatus = status @@ -103,6 +136,7 @@ var _ error = &errorData{} func (ed *errorData) unexported() {} +// Error implements the error interface. func (ed *errorData) Error() string { return ed.error.Error() } @@ -114,7 +148,8 @@ func (ed *errorData) Unwrap() error { // HTTPStatus returns an HTTP status code either directly // set by the source or derived from a gRPC status code set -// by the source. +// by the source. If both statuses are set, the HTTP status +// code is returned. // // If no code has been set, the second return value is `false`. func (ed *errorData) HTTPStatus() (int, bool) { @@ -129,7 +164,8 @@ func (ed *errorData) HTTPStatus() (int, bool) { // GRPCStatus returns an gRPC status code either directly // set by the source or derived from an HTTP status code set -// by the source. +// by the source. If both statuses are set, the gRPC status +// code is returned. // // If no code has been set, the second return value is `false`. func (ed *errorData) GRPCStatus() (*status.Status, bool) { diff --git a/consumer/consumererror/error_test.go b/consumer/consumererror/error_test.go new file mode 100644 index 00000000000..aec888460b0 --- /dev/null +++ b/consumer/consumererror/error_test.go @@ -0,0 +1,248 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package consumererror + +import ( + "errors" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +var errTest = errors.New("consumererror testing error") + +func Test_New(t *testing.T) { + httpStatus := 500 + grpcStatus := status.New(codes.Aborted, "aborted") + wantErr := &Error{ + errors: []ErrorData{ + &errorData{ + error: errTest, + httpStatus: &httpStatus, + grpcStatus: grpcStatus, + }, + }, + } + + newErr := New(errTest, + WithHTTPStatus(httpStatus), + WithGRPCStatus(grpcStatus), + ) + + require.Equal(t, wantErr, newErr) +} + +func Test_Error(t *testing.T) { + newErr := New(errTest) + + require.Equal(t, errTest.Error(), newErr.Error()) +} + +func TestUnwrap(t *testing.T) { + err := &Error{ + errors: []ErrorData{ + &errorData{ + error: errTest, + }, + &errorData{ + error: errTest, + }, + }, + } + + unwrapped := err.Unwrap() + + require.Len(t, unwrapped, 2) + + for _, e := range unwrapped { + require.ErrorIs(t, e, errTest) + } +} + +func TestData(t *testing.T) { + httpStatus := 500 + err := &Error{ + errors: []ErrorData{ + &errorData{ + error: errTest, + httpStatus: &httpStatus, + }, + &errorData{ + error: errTest, + httpStatus: &httpStatus, + }, + }, + } + + data := err.Data() + + require.Len(t, data, 2) + + for _, e := range data { + status, ok := e.HTTPStatus() + require.True(t, ok) + require.Equal(t, httpStatus, status) + } +} + +func TestCombine(t *testing.T) { + err0 := &Error{ + errors: []ErrorData{ + &errorData{ + error: errTest, + }, + }, + } + err1 := &Error{ + errors: []ErrorData{ + &errorData{ + error: errTest, + }, + &errorData{ + error: errTest, + }, + }, + } + want := &Error{ + errors: []ErrorData{ + &errorData{ + error: errTest, + }, + &errorData{ + error: errTest, + }, + &errorData{ + error: errTest, + }, + &errorData{ + error: errTest, + }, + }, + } + + err := Combine(err0, err1, errTest) + + require.Equal(t, want, err) +} + +func TestErrorDataError(t *testing.T) { + err := &errorData{ + error: errTest, + } + + require.Equal(t, errTest.Error(), err.Error()) +} + +func TestErrorDataUnwrap(t *testing.T) { + err := &errorData{ + error: errTest, + } + + require.Equal(t, errTest, err.Unwrap()) +} + +func TestErrorDataHTTPStatus(t *testing.T) { + serverErr := http.StatusTooManyRequests + testCases := []struct { + name string + httpStatus *int + grpcStatus *status.Status + want int + hasCode bool + }{ + { + name: "Passes through HTTP status", + httpStatus: &serverErr, + want: serverErr, + hasCode: true, + }, + { + name: "Converts gRPC status", + grpcStatus: status.New(codes.ResourceExhausted, errTest.Error()), + want: serverErr, + hasCode: true, + }, + { + name: "Passes through HTTP status when gRPC status also present", + httpStatus: &serverErr, + grpcStatus: status.New(codes.OK, errTest.Error()), + want: serverErr, + hasCode: true, + }, + { + name: "No statuses set", + hasCode: false, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + err := errorData{ + error: errTest, + httpStatus: tt.httpStatus, + grpcStatus: tt.grpcStatus, + } + + s, ok := err.HTTPStatus() + + require.Equal(t, tt.hasCode, ok) + require.Equal(t, tt.want, s) + }) + } +} + +func TestErrorDataGRPCStatus(t *testing.T) { + httpStatus := http.StatusTooManyRequests + otherHTTPStatus := http.StatusOK + serverErr := status.New(codes.ResourceExhausted, errTest.Error()) + testCases := []struct { + name string + httpStatus *int + grpcStatus *status.Status + want *status.Status + hasCode bool + }{ + { + name: "Converts HTTP status", + httpStatus: &httpStatus, + want: serverErr, + hasCode: true, + }, + { + name: "Passes through gRPC status", + grpcStatus: serverErr, + want: serverErr, + hasCode: true, + }, + { + name: "Passes through gRPC status when gRPC status also present", + httpStatus: &otherHTTPStatus, + grpcStatus: serverErr, + want: serverErr, + hasCode: true, + }, + { + name: "No statuses set", + hasCode: false, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + err := errorData{ + error: errTest, + httpStatus: tt.httpStatus, + grpcStatus: tt.grpcStatus, + } + + s, ok := err.GRPCStatus() + + require.Equal(t, tt.hasCode, ok) + require.Equal(t, tt.want, s) + }) + } +} From e953920d3a2468035ef6e13ef9b3d52c38c125a7 Mon Sep 17 00:00:00 2001 From: Evan Bradley Date: Thu, 8 Aug 2024 16:05:15 -0400 Subject: [PATCH 29/37] Update changelog --- .chloggen/breaking-retryable-errors.yaml | 25 ------------------------ .chloggen/issue-7047.yaml | 6 ++++-- 2 files changed, 4 insertions(+), 27 deletions(-) delete mode 100644 .chloggen/breaking-retryable-errors.yaml diff --git a/.chloggen/breaking-retryable-errors.yaml b/.chloggen/breaking-retryable-errors.yaml deleted file mode 100644 index a36f16000c2..00000000000 --- a/.chloggen/breaking-retryable-errors.yaml +++ /dev/null @@ -1,25 +0,0 @@ -# Use this changelog template to create an entry for release notes. - -# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: breaking - -# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) -component: consumererror - -# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Add optional arguments to `NewTraces`, `NewMetrics`, and `NewLogs` - -# One or more tracking issues or pull requests related to the change -issues: [7047] - -# (Optional) One or more lines of additional information to render under the primary note. -# These lines will be padded with 2 spaces and then inserted directly into the document. -# Use pipe (|) for multiline entries. -subtext: This change has no impact unless you depend on the type signature of these functions. - -# Optional: The change log or logs in which this entry should be included. -# e.g. '[user]' or '[user, api]' -# Include 'user' if the change is relevant to end users. -# Include 'api' if there is a change to a library API. -# Default: '[user]' -change_logs: [api] diff --git a/.chloggen/issue-7047.yaml b/.chloggen/issue-7047.yaml index 6dc9d1bcfce..89cadccdf4e 100644 --- a/.chloggen/issue-7047.yaml +++ b/.chloggen/issue-7047.yaml @@ -7,7 +7,7 @@ change_type: enhancement component: consumererror # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Provide additional error types to allow annotating errors +note: Introduce an `Error` type that allows recording contextual information # One or more tracking issues or pull requests related to the change issues: [7047] @@ -15,7 +15,9 @@ issues: [7047] # (Optional) One or more lines of additional information to render under the primary note. # These lines will be padded with 2 spaces and then inserted directly into the document. # Use pipe (|) for multiline entries. -subtext: Allows putting things like status codes, retry information, etc. on consumer errors. +subtext: | + Currently allows recording status codes on consumer errors, + but will be expanded in the future to record additional data. # Optional: The change log or logs in which this entry should be included. # e.g. '[user]' or '[user, api]' From a3fb82d2d493b909bc876dc936134325dd5e1c9b Mon Sep 17 00:00:00 2001 From: Evan Bradley Date: Thu, 8 Aug 2024 17:28:09 -0400 Subject: [PATCH 30/37] Minor cleanup --- .chloggen/issue-7047.yaml | 2 +- consumer/consumererror/error.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.chloggen/issue-7047.yaml b/.chloggen/issue-7047.yaml index 89cadccdf4e..472e532d762 100644 --- a/.chloggen/issue-7047.yaml +++ b/.chloggen/issue-7047.yaml @@ -24,4 +24,4 @@ subtext: | # Include 'user' if the change is relevant to end users. # Include 'api' if there is a change to a library API. # Default: '[user]' -change_logs: [] +change_logs: [api] diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 56b30d9c727..179f1f9a56c 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -30,7 +30,6 @@ type Error struct { // out by an upstream component by calling `Error.Data`. // // Experimental: This API is at the early stage of development and may change without backward compatibility - type ErrorData interface { Error() string From b3cbde5439fa08c06bad4412cd274c139640ab6f Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 9 Aug 2024 08:05:32 -0400 Subject: [PATCH 31/37] Update consumer/consumererror/error.go Co-authored-by: Bogdan Drutu --- consumer/consumererror/error.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 179f1f9a56c..e58fd38fd64 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -131,7 +131,7 @@ func WithGRPCStatus(status *status.Status) ErrorOption { } } -var _ error = &errorData{} +var _ error = (*errorData)(nil) func (ed *errorData) unexported() {} From 1d1affc80d1a97e25ef26f3250ba825820b5fecf Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 9 Aug 2024 08:27:40 -0400 Subject: [PATCH 32/37] Address PR feedback --- consumer/consumererror/README.md | 23 +++++----- consumer/consumererror/error.go | 54 +++++++++++++---------- consumer/consumererror/error_test.go | 65 ++++++++++++++++++++-------- 3 files changed, 90 insertions(+), 52 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 1c6bac5b237..53c985f1690 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -198,21 +198,20 @@ component can then later pull all errors out for analysis. > here is for design purposes. The code snippet may not work as-written. When a receiver gets a response that includes an error, it can get the data out -by doing something similar to the following. Note that this uses the `ErrorData` -type, which is for reading error data, as opposed to the `Error` type, which is -for recording errors. +by doing something similar to the following. Note that this uses the `Error` +type, which is for reading error data, as opposed to the `ErrorContainer` type, +which is for recording errors. ```go -cerr := consumererror.Error{} -var errData []consumerError.ErrorData +cerr := consumererror.ErrorContainer{} if errors.As(err, &cerr) { - errData := cerr.Data() + errs := cerr.Errors() - for _, data := range errData { - data.HTTPStatus() - data.Retryable() - data.Partial() + for _, e := range errs { + e.HTTPStatus() + e.Retryable() + e.Partial() } } ``` @@ -227,7 +226,7 @@ Obtaining data from an error can be done using an interface that looks something like this: ```go -type ErrorData interface { +type Error interface { // Returns the underlying error Error() error @@ -302,5 +301,5 @@ The following describes how to transition to these error types: `consumererror.IsPermanent` will be deprecated in favor of checking whether retry information is available, and only retrying if it has been provided. This -will be possible by calling `ErrorData.Retryable()` and checking for retry +will be possible by calling `Error.Retryable()` and checking for retry information. diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index e58fd38fd64..4f8f1ce83bf 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -11,26 +11,26 @@ import ( "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" ) -// Error acts as a container for errors coming from pipeline components. +// ErrorContainer acts as a container for errors coming from pipeline components. // It may hold multiple errors from downstream components, and is designed // to act as a way to accumulate errors as it travels upstream in a pipeline. -// `Error` should be obtained using `errors.As` and as a result, ideally +// `ErrorContainer` should be obtained using `errors.As` and as a result, ideally // a single instance should exist in an error stack. If this is not possible, -// the most `Error` object should be highest on the stack. +// the most `ErrorContainer` object should be highest on the stack. // // Experimental: This API is at the early stage of development and may change without backward compatibility -type Error struct { - errors []ErrorData +type ErrorContainer struct { + errors []Error } -// ErrorData is intended to be used to encapsulate various information +// Error is intended to be used to encapsulate various information // that can add context to an error that occurred within a pipeline component. -// ErrorData objects are constructed through calling `New` with the relevant +// Error objects are constructed through calling `New` with the relevant // options to capture data around the error that occurred. It can then be pulled // out by an upstream component by calling `Error.Data`. // // Experimental: This API is at the early stage of development and may change without backward compatibility -type ErrorData interface { +type Error interface { Error() string Unwrap() error @@ -55,7 +55,15 @@ type errorData struct { } // ErrorOption allows annotating an Error with metadata. -type ErrorOption func(error *errorData) +type ErrorOption interface { + applyOption(*errorData) +} + +type errorOptionFunc func(*errorData) + +func (f errorOptionFunc) applyOption(e *errorData) { + f(e) +} // New wraps an error that happened while consuming telemetry // and adds metadata onto it to be passed back up the pipeline. @@ -63,24 +71,24 @@ type ErrorOption func(error *errorData) // Experimental: This API is at the early stage of development and may change without backward compatibility func New(origErr error, options ...ErrorOption) error { errData := &errorData{error: origErr} - err := &Error{errors: []ErrorData{errData}} + err := &ErrorContainer{errors: []Error{errData}} for _, option := range options { - option(errData) + option.applyOption(errData) } return err } -var _ error = &Error{} +var _ error = (*ErrorContainer)(nil) // Error implements the `error` interface. -func (e *Error) Error() string { +func (e *ErrorContainer) Error() string { return e.errors[len(e.errors)-1].Error() } // Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. -func (e *Error) Unwrap() []error { +func (e *ErrorContainer) Unwrap() []error { errors := make([]error, 0, len(e.errors)) for _, err := range e.errors { @@ -90,21 +98,21 @@ func (e *Error) Unwrap() []error { return errors } -// Data returns all the accumulated ErrorData errors +// Errors returns all the accumulated Error objects // emitted by components downstream in the pipeline. // These can then be aggregated or worked with individually. -func (e *Error) Data() []ErrorData { +func (e *ErrorContainer) Errors() []Error { return e.errors } // Combine joins errors that occur at a fanout into a single // `Error` object. The component that created the data submission // can then work with the `Error` object to understand the failure. -func Combine(errs ...error) *Error { - e := &Error{errors: make([]ErrorData, 0, len(errs))} +func Combine(errs ...error) *ErrorContainer { + e := &ErrorContainer{errors: make([]Error, 0, len(errs))} for _, err := range errs { - var otherErr *Error + var otherErr *ErrorContainer if errors.As(err, &otherErr) { e.errors = append(e.errors, otherErr.errors...) } else { @@ -118,17 +126,17 @@ func Combine(errs ...error) *Error { // WithHTTPStatus records an HTTP status code that was received // from a server during data submission. func WithHTTPStatus(status int) ErrorOption { - return func(err *errorData) { + return errorOptionFunc(func(err *errorData) { err.httpStatus = &status - } + }) } // WithGRPCStatus records a gRPC status code that was received // from a server during data submission. func WithGRPCStatus(status *status.Status) ErrorOption { - return func(err *errorData) { + return errorOptionFunc(func(err *errorData) { err.grpcStatus = status - } + }) } var _ error = (*errorData)(nil) diff --git a/consumer/consumererror/error_test.go b/consumer/consumererror/error_test.go index aec888460b0..9562738cfa9 100644 --- a/consumer/consumererror/error_test.go +++ b/consumer/consumererror/error_test.go @@ -18,8 +18,8 @@ var errTest = errors.New("consumererror testing error") func Test_New(t *testing.T) { httpStatus := 500 grpcStatus := status.New(codes.Aborted, "aborted") - wantErr := &Error{ - errors: []ErrorData{ + wantErr := &ErrorContainer{ + errors: []Error{ &errorData{ error: errTest, httpStatus: &httpStatus, @@ -43,8 +43,8 @@ func Test_Error(t *testing.T) { } func TestUnwrap(t *testing.T) { - err := &Error{ - errors: []ErrorData{ + err := &ErrorContainer{ + errors: []Error{ &errorData{ error: errTest, }, @@ -65,8 +65,8 @@ func TestUnwrap(t *testing.T) { func TestData(t *testing.T) { httpStatus := 500 - err := &Error{ - errors: []ErrorData{ + err := &ErrorContainer{ + errors: []Error{ &errorData{ error: errTest, httpStatus: &httpStatus, @@ -78,7 +78,7 @@ func TestData(t *testing.T) { }, } - data := err.Data() + data := err.Errors() require.Len(t, data, 2) @@ -89,16 +89,47 @@ func TestData(t *testing.T) { } } +func TestErrorSliceIsCopy(t *testing.T) { + httpStatus := 500 + err := &ErrorContainer{ + errors: []Error{ + &errorData{ + error: errTest, + httpStatus: &httpStatus, + }, + &errorData{ + error: errTest, + httpStatus: &httpStatus, + }, + }, + } + + errs := err.Errors() + + require.Len(t, errs, 2) + + for _, e := range errs { + status, ok := e.HTTPStatus() + require.True(t, ok) + require.Equal(t, httpStatus, status) + } + + errs = append(errs, &errorData{error: errTest}) + + require.Len(t, errs, 3) + require.Len(t, err.errors, 2) +} + func TestCombine(t *testing.T) { - err0 := &Error{ - errors: []ErrorData{ + err0 := &ErrorContainer{ + errors: []Error{ &errorData{ error: errTest, }, }, } - err1 := &Error{ - errors: []ErrorData{ + err1 := &ErrorContainer{ + errors: []Error{ &errorData{ error: errTest, }, @@ -107,8 +138,8 @@ func TestCombine(t *testing.T) { }, }, } - want := &Error{ - errors: []ErrorData{ + want := &ErrorContainer{ + errors: []Error{ &errorData{ error: errTest, }, @@ -129,7 +160,7 @@ func TestCombine(t *testing.T) { require.Equal(t, want, err) } -func TestErrorDataError(t *testing.T) { +func TestError_Error(t *testing.T) { err := &errorData{ error: errTest, } @@ -137,7 +168,7 @@ func TestErrorDataError(t *testing.T) { require.Equal(t, errTest.Error(), err.Error()) } -func TestErrorDataUnwrap(t *testing.T) { +func TestError_Unwrap(t *testing.T) { err := &errorData{ error: errTest, } @@ -145,7 +176,7 @@ func TestErrorDataUnwrap(t *testing.T) { require.Equal(t, errTest, err.Unwrap()) } -func TestErrorDataHTTPStatus(t *testing.T) { +func TestError_HTTPStatus(t *testing.T) { serverErr := http.StatusTooManyRequests testCases := []struct { name string @@ -195,7 +226,7 @@ func TestErrorDataHTTPStatus(t *testing.T) { } } -func TestErrorDataGRPCStatus(t *testing.T) { +func TestError_GRPCStatus(t *testing.T) { httpStatus := http.StatusTooManyRequests otherHTTPStatus := http.StatusOK serverErr := status.New(codes.ResourceExhausted, errTest.Error()) From 404ebd563be2b40ec19cff4d5c8d68c587b9eeaa Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:52:14 -0400 Subject: [PATCH 33/37] Combine ErrorContainer and Error into a single type --- consumer/consumererror/error.go | 188 +++++++++--------- consumer/consumererror/error_test.go | 109 +++++----- .../internal/statusconversion/conversion.go | 12 ++ 3 files changed, 158 insertions(+), 151 deletions(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 4f8f1ce83bf..2d90c4f2158 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -5,161 +5,131 @@ package consumererror // import "go.opentelemetry.io/collector/consumer/consumer import ( "errors" + "net/http" "google.golang.org/grpc/status" "go.opentelemetry.io/collector/consumer/consumererror/internal/statusconversion" ) -// ErrorContainer acts as a container for errors coming from pipeline components. -// It may hold multiple errors from downstream components, and is designed -// to act as a way to accumulate errors as it travels upstream in a pipeline. -// `ErrorContainer` should be obtained using `errors.As` and as a result, ideally -// a single instance should exist in an error stack. If this is not possible, -// the most `ErrorContainer` object should be highest on the stack. +// Error is intended to be used to encapsulate various information that can add +// context to an error that occurred within a pipeline component. Error objects +// are constructed through calling `New` with the relevant options to capture +// data around the error that occurred. // -// Experimental: This API is at the early stage of development and may change without backward compatibility -type ErrorContainer struct { - errors []Error -} - -// Error is intended to be used to encapsulate various information -// that can add context to an error that occurred within a pipeline component. -// Error objects are constructed through calling `New` with the relevant -// options to capture data around the error that occurred. It can then be pulled -// out by an upstream component by calling `Error.Data`. +// It may hold multiple errors from downstream components, and can +// be merged with other errors as it travels upstream using `Combine`. `Error` +// should be obtained using `errors.As`. If this is not possible, the most +// `ErrorContainer` object should be highest on the stack. // -// Experimental: This API is at the early stage of development and may change without backward compatibility -type Error interface { - Error() string - - Unwrap() error - - // Returns a status code and a boolean indicating whether a status was set - // by the error creator. - HTTPStatus() (int, bool) - - // Returns a status code and a boolean indicating whether a status was set - // by the error creator. - GRPCStatus() (*status.Status, bool) - - // Disallow implementations outside this package to allow later extending - // the interface. - unexported() -} - -type errorData struct { +// Experimental: This API is at the early stage of development and may change +// without backward compatibility +type Error struct { error httpStatus *int grpcStatus *status.Status + + errors []*Error } +var _ error = (*Error)(nil) + // ErrorOption allows annotating an Error with metadata. type ErrorOption interface { - applyOption(*errorData) + applyOption(*Error) } -type errorOptionFunc func(*errorData) +type errorOptionFunc func(*Error) -func (f errorOptionFunc) applyOption(e *errorData) { +func (f errorOptionFunc) applyOption(e *Error) { f(e) } -// New wraps an error that happened while consuming telemetry -// and adds metadata onto it to be passed back up the pipeline. +// New wraps an error that happened while consuming telemetry and adds metadata +// onto it to be passed back up the pipeline. // -// Experimental: This API is at the early stage of development and may change without backward compatibility +// Experimental: This API is at the early stage of development and may change +// without backward compatibility func New(origErr error, options ...ErrorOption) error { - errData := &errorData{error: origErr} - err := &ErrorContainer{errors: []Error{errData}} + err := &Error{error: origErr} for _, option := range options { - option.applyOption(errData) + option.applyOption(err) } return err } -var _ error = (*ErrorContainer)(nil) - -// Error implements the `error` interface. -func (e *ErrorContainer) Error() string { - return e.errors[len(e.errors)-1].Error() -} - -// Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. -func (e *ErrorContainer) Unwrap() []error { - errors := make([]error, 0, len(e.errors)) - - for _, err := range e.errors { - errors = append(errors, err) - } - - return errors -} - -// Errors returns all the accumulated Error objects -// emitted by components downstream in the pipeline. -// These can then be aggregated or worked with individually. -func (e *ErrorContainer) Errors() []Error { - return e.errors -} +// Combine joins errors into a single `Error` object. The component that +// initiated the data submission can then work with the `Error` object to +// understand the failure. +func Combine(errs ...error) error { + e := &Error{errors: make([]*Error, 0, len(errs))} -// Combine joins errors that occur at a fanout into a single -// `Error` object. The component that created the data submission -// can then work with the `Error` object to understand the failure. -func Combine(errs ...error) *ErrorContainer { - e := &ErrorContainer{errors: make([]Error, 0, len(errs))} + resultingStatus := 0 for _, err := range errs { - var otherErr *ErrorContainer + var otherErr *Error if errors.As(err, &otherErr) { + if otherErr.httpStatus != nil { + resultingStatus = aggregateStatuses(resultingStatus, *otherErr.httpStatus) + } else if otherErr.grpcStatus != nil { + resultingStatus = aggregateStatuses(resultingStatus, statusconversion.GetHTTPStatusCodeFromStatus(otherErr.grpcStatus)) + } + e.errors = append(e.errors, otherErr.errors...) } else { - e.errors = append(e.errors, &errorData{error: err}) + e.errors = append(e.errors, &Error{error: err}) } } + if resultingStatus != 0 { + e.httpStatus = &resultingStatus + } + return e } -// WithHTTPStatus records an HTTP status code that was received -// from a server during data submission. +// WithHTTPStatus records an HTTP status code that was received from a server +// during data submission. func WithHTTPStatus(status int) ErrorOption { - return errorOptionFunc(func(err *errorData) { + return errorOptionFunc(func(err *Error) { err.httpStatus = &status }) } -// WithGRPCStatus records a gRPC status code that was received -// from a server during data submission. +// WithGRPCStatus records a gRPC status code that was received from a server +// during data submission. func WithGRPCStatus(status *status.Status) ErrorOption { - return errorOptionFunc(func(err *errorData) { + return errorOptionFunc(func(err *Error) { err.grpcStatus = status }) } -var _ error = (*errorData)(nil) - -func (ed *errorData) unexported() {} - // Error implements the error interface. -func (ed *errorData) Error() string { +func (ed *Error) Error() string { return ed.error.Error() } // Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. -func (ed *errorData) Unwrap() error { - return ed.error +func (e *Error) Unwrap() []error { + errors := make([]error, 0, len(e.errors)+1) + + errors = append(errors, e.error) + + for _, err := range e.errors { + errors = append(errors, err) + } + + return errors } -// HTTPStatus returns an HTTP status code either directly -// set by the source or derived from a gRPC status code set -// by the source. If both statuses are set, the HTTP status -// code is returned. +// HTTPStatus returns an HTTP status code either directly set by the source or +// derived from a gRPC status code set by the source. If both statuses are set, +// the HTTP status code is returned. // // If no code has been set, the second return value is `false`. -func (ed *errorData) HTTPStatus() (int, bool) { +func (ed *Error) HTTPStatus() (int, bool) { if ed.httpStatus != nil { return *ed.httpStatus, true } else if ed.grpcStatus != nil { @@ -169,13 +139,12 @@ func (ed *errorData) HTTPStatus() (int, bool) { return 0, false } -// GRPCStatus returns an gRPC status code either directly -// set by the source or derived from an HTTP status code set -// by the source. If both statuses are set, the gRPC status -// code is returned. +// GRPCStatus returns an gRPC status code either directly set by the source or +// derived from an HTTP status code set by the source. If both statuses are set, +// the gRPC status code is returned. // // If no code has been set, the second return value is `false`. -func (ed *errorData) GRPCStatus() (*status.Status, bool) { +func (ed *Error) GRPCStatus() (*status.Status, bool) { if ed.grpcStatus != nil { return ed.grpcStatus, true } else if ed.httpStatus != nil { @@ -184,3 +153,24 @@ func (ed *errorData) GRPCStatus() (*status.Status, bool) { return nil, false } + +func aggregateStatuses(a int, b int) int { + switch { + // If a is unset, keep b + case a == 0: + return b + // If b is unset, keep a + case b == 0: + return a + // If a and b have been set and one is a 4xx code, the correct code is + // ambiguous, so return a 400, which is permanent. + case (a >= 400 && a < 500) || (b >= 400 && b < 500): + return http.StatusBadRequest + // If a and b have been set and one is a 5xx code, the correct code is + // ambiguous, so return a 500, which is permanent. + case a >= 500 || b >= 500: + return http.StatusInternalServerError + default: + return http.StatusInternalServerError + } +} diff --git a/consumer/consumererror/error_test.go b/consumer/consumererror/error_test.go index 9562738cfa9..7b26e8bc956 100644 --- a/consumer/consumererror/error_test.go +++ b/consumer/consumererror/error_test.go @@ -18,14 +18,10 @@ var errTest = errors.New("consumererror testing error") func Test_New(t *testing.T) { httpStatus := 500 grpcStatus := status.New(codes.Aborted, "aborted") - wantErr := &ErrorContainer{ - errors: []Error{ - &errorData{ - error: errTest, - httpStatus: &httpStatus, - grpcStatus: grpcStatus, - }, - }, + wantErr := &Error{ + error: errTest, + httpStatus: &httpStatus, + grpcStatus: grpcStatus, } newErr := New(errTest, @@ -43,12 +39,13 @@ func Test_Error(t *testing.T) { } func TestUnwrap(t *testing.T) { - err := &ErrorContainer{ - errors: []Error{ - &errorData{ + err := &Error{ + error: errTest, + errors: []*Error{ + { error: errTest, }, - &errorData{ + { error: errTest, }, }, @@ -56,7 +53,7 @@ func TestUnwrap(t *testing.T) { unwrapped := err.Unwrap() - require.Len(t, unwrapped, 2) + require.Len(t, unwrapped, 3) for _, e := range unwrapped { require.ErrorIs(t, e, errTest) @@ -65,91 +62,99 @@ func TestUnwrap(t *testing.T) { func TestData(t *testing.T) { httpStatus := 500 - err := &ErrorContainer{ - errors: []Error{ - &errorData{ + err := &Error{ + error: errTest, + errors: []*Error{ + { error: errTest, httpStatus: &httpStatus, }, - &errorData{ + { error: errTest, httpStatus: &httpStatus, }, }, } - data := err.Errors() + errs := err.Unwrap() - require.Len(t, data, 2) + require.Len(t, errs, 3) - for _, e := range data { - status, ok := e.HTTPStatus() - require.True(t, ok) - require.Equal(t, httpStatus, status) + for _, e := range errs { + ce := &Error{} + if errors.As(e, &ce) { + status, ok := ce.HTTPStatus() + require.True(t, ok) + require.Equal(t, httpStatus, status) + } } } func TestErrorSliceIsCopy(t *testing.T) { httpStatus := 500 - err := &ErrorContainer{ - errors: []Error{ - &errorData{ + err := &Error{ + error: errTest, + errors: []*Error{ + { error: errTest, httpStatus: &httpStatus, }, - &errorData{ + { error: errTest, httpStatus: &httpStatus, }, }, } - errs := err.Errors() + errs := err.Unwrap() - require.Len(t, errs, 2) + require.Len(t, errs, 3) for _, e := range errs { - status, ok := e.HTTPStatus() - require.True(t, ok) - require.Equal(t, httpStatus, status) + ce := &Error{} + if errors.As(e, &ce) { + status, ok := ce.HTTPStatus() + require.True(t, ok) + require.Equal(t, httpStatus, status) + } } - errs = append(errs, &errorData{error: errTest}) + errs = append(errs, &Error{error: errTest}) - require.Len(t, errs, 3) + require.Len(t, errs, 4) require.Len(t, err.errors, 2) } func TestCombine(t *testing.T) { - err0 := &ErrorContainer{ - errors: []Error{ - &errorData{ + err0 := &Error{ + errors: []*Error{ + { error: errTest, }, }, } - err1 := &ErrorContainer{ - errors: []Error{ - &errorData{ + err1 := &Error{ + errors: []*Error{ + { error: errTest, }, - &errorData{ + { error: errTest, }, }, } - want := &ErrorContainer{ - errors: []Error{ - &errorData{ + want := &Error{ + errors: []*Error{ + { error: errTest, }, - &errorData{ + { error: errTest, }, - &errorData{ + { error: errTest, }, - &errorData{ + { error: errTest, }, }, @@ -161,7 +166,7 @@ func TestCombine(t *testing.T) { } func TestError_Error(t *testing.T) { - err := &errorData{ + err := &Error{ error: errTest, } @@ -169,11 +174,11 @@ func TestError_Error(t *testing.T) { } func TestError_Unwrap(t *testing.T) { - err := &errorData{ + err := &Error{ error: errTest, } - require.Equal(t, errTest, err.Unwrap()) + require.Equal(t, []error{errTest}, err.Unwrap()) } func TestError_HTTPStatus(t *testing.T) { @@ -212,7 +217,7 @@ func TestError_HTTPStatus(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - err := errorData{ + err := Error{ error: errTest, httpStatus: tt.httpStatus, grpcStatus: tt.grpcStatus, @@ -264,7 +269,7 @@ func TestError_GRPCStatus(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - err := errorData{ + err := Error{ error: errTest, httpStatus: tt.httpStatus, grpcStatus: tt.grpcStatus, diff --git a/consumer/consumererror/internal/statusconversion/conversion.go b/consumer/consumererror/internal/statusconversion/conversion.go index a193206dfb6..4c34cd0681c 100644 --- a/consumer/consumererror/internal/statusconversion/conversion.go +++ b/consumer/consumererror/internal/statusconversion/conversion.go @@ -23,6 +23,18 @@ func GetHTTPStatusCodeFromStatus(s *status.Status) int { case codes.ResourceExhausted: return http.StatusTooManyRequests // Not Retryable + case codes.InvalidArgument: + return http.StatusBadRequest + // Not Retryable + case codes.Unauthenticated: + return http.StatusUnauthorized + // Not Retryable + case codes.PermissionDenied: + return http.StatusForbidden + // Not Retryable + case codes.Unimplemented: + return http.StatusNotFound + // Not Retryable default: return http.StatusInternalServerError } From be9b41aac12bcc0118ed364e11277f67f5e38676 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 29 Aug 2024 16:57:26 -0400 Subject: [PATCH 34/37] Address PR feedback --- consumer/consumererror/error.go | 61 +++++-------- consumer/consumererror/error_test.go | 130 +++------------------------ 2 files changed, 35 insertions(+), 156 deletions(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index 2d90c4f2158..c5fde1cd46a 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -17,19 +17,16 @@ import ( // are constructed through calling `New` with the relevant options to capture // data around the error that occurred. // -// It may hold multiple errors from downstream components, and can -// be merged with other errors as it travels upstream using `Combine`. `Error` -// should be obtained using `errors.As`. If this is not possible, the most -// `ErrorContainer` object should be highest on the stack. +// It may hold multiple errors from downstream components, and can be merged +// with other errors as it travels upstream using `Combine`. The `Error` should +// be obtained from a given `error` object using `errors.As`. // // Experimental: This API is at the early stage of development and may change // without backward compatibility type Error struct { error - httpStatus *int + httpStatus int grpcStatus *status.Status - - errors []*Error } var _ error = (*Error)(nil) @@ -64,27 +61,23 @@ func New(origErr error, options ...ErrorOption) error { // initiated the data submission can then work with the `Error` object to // understand the failure. func Combine(errs ...error) error { - e := &Error{errors: make([]*Error, 0, len(errs))} + e := &Error{error: errors.Join(errs...)} resultingStatus := 0 for _, err := range errs { var otherErr *Error if errors.As(err, &otherErr) { - if otherErr.httpStatus != nil { - resultingStatus = aggregateStatuses(resultingStatus, *otherErr.httpStatus) + if otherErr.httpStatus != 0 { + resultingStatus = aggregateStatuses(resultingStatus, otherErr.httpStatus) } else if otherErr.grpcStatus != nil { resultingStatus = aggregateStatuses(resultingStatus, statusconversion.GetHTTPStatusCodeFromStatus(otherErr.grpcStatus)) } - - e.errors = append(e.errors, otherErr.errors...) - } else { - e.errors = append(e.errors, &Error{error: err}) } } if resultingStatus != 0 { - e.httpStatus = &resultingStatus + e.httpStatus = resultingStatus } return e @@ -94,7 +87,7 @@ func Combine(errs ...error) error { // during data submission. func WithHTTPStatus(status int) ErrorOption { return errorOptionFunc(func(err *Error) { - err.httpStatus = &status + err.httpStatus = status }) } @@ -107,21 +100,13 @@ func WithGRPCStatus(status *status.Status) ErrorOption { } // Error implements the error interface. -func (ed *Error) Error() string { - return ed.error.Error() +func (e *Error) Error() string { + return e.error.Error() } // Unwrap returns the wrapped error for use by `errors.Is` and `errors.As`. -func (e *Error) Unwrap() []error { - errors := make([]error, 0, len(e.errors)+1) - - errors = append(errors, e.error) - - for _, err := range e.errors { - errors = append(errors, err) - } - - return errors +func (e *Error) Unwrap() error { + return e.error } // HTTPStatus returns an HTTP status code either directly set by the source or @@ -129,11 +114,11 @@ func (e *Error) Unwrap() []error { // the HTTP status code is returned. // // If no code has been set, the second return value is `false`. -func (ed *Error) HTTPStatus() (int, bool) { - if ed.httpStatus != nil { - return *ed.httpStatus, true - } else if ed.grpcStatus != nil { - return statusconversion.GetHTTPStatusCodeFromStatus(ed.grpcStatus), true +func (e *Error) HTTPStatus() (int, bool) { + if e.httpStatus != 0 { + return e.httpStatus, true + } else if e.grpcStatus != nil { + return statusconversion.GetHTTPStatusCodeFromStatus(e.grpcStatus), true } return 0, false @@ -144,11 +129,11 @@ func (ed *Error) HTTPStatus() (int, bool) { // the gRPC status code is returned. // // If no code has been set, the second return value is `false`. -func (ed *Error) GRPCStatus() (*status.Status, bool) { - if ed.grpcStatus != nil { - return ed.grpcStatus, true - } else if ed.httpStatus != nil { - return statusconversion.NewStatusFromMsgAndHTTPCode(ed.Error(), *ed.httpStatus), true +func (e *Error) GRPCStatus() (*status.Status, bool) { + if e.grpcStatus != nil { + return e.grpcStatus, true + } else if e.httpStatus != 0 { + return statusconversion.NewStatusFromMsgAndHTTPCode(e.Error(), e.httpStatus), true } return nil, false diff --git a/consumer/consumererror/error_test.go b/consumer/consumererror/error_test.go index 7b26e8bc956..60dfbf11fab 100644 --- a/consumer/consumererror/error_test.go +++ b/consumer/consumererror/error_test.go @@ -20,7 +20,7 @@ func Test_New(t *testing.T) { grpcStatus := status.New(codes.Aborted, "aborted") wantErr := &Error{ error: errTest, - httpStatus: &httpStatus, + httpStatus: httpStatus, grpcStatus: grpcStatus, } @@ -41,128 +41,22 @@ func Test_Error(t *testing.T) { func TestUnwrap(t *testing.T) { err := &Error{ error: errTest, - errors: []*Error{ - { - error: errTest, - }, - { - error: errTest, - }, - }, } unwrapped := err.Unwrap() - require.Len(t, unwrapped, 3) - - for _, e := range unwrapped { - require.ErrorIs(t, e, errTest) - } -} - -func TestData(t *testing.T) { - httpStatus := 500 - err := &Error{ - error: errTest, - errors: []*Error{ - { - error: errTest, - httpStatus: &httpStatus, - }, - { - error: errTest, - httpStatus: &httpStatus, - }, - }, - } - - errs := err.Unwrap() - - require.Len(t, errs, 3) - - for _, e := range errs { - ce := &Error{} - if errors.As(e, &ce) { - status, ok := ce.HTTPStatus() - require.True(t, ok) - require.Equal(t, httpStatus, status) - } - } -} - -func TestErrorSliceIsCopy(t *testing.T) { - httpStatus := 500 - err := &Error{ - error: errTest, - errors: []*Error{ - { - error: errTest, - httpStatus: &httpStatus, - }, - { - error: errTest, - httpStatus: &httpStatus, - }, - }, - } - - errs := err.Unwrap() - - require.Len(t, errs, 3) - - for _, e := range errs { - ce := &Error{} - if errors.As(e, &ce) { - status, ok := ce.HTTPStatus() - require.True(t, ok) - require.Equal(t, httpStatus, status) - } - } - - errs = append(errs, &Error{error: errTest}) - - require.Len(t, errs, 4) - require.Len(t, err.errors, 2) + require.Equal(t, errTest, unwrapped) } func TestCombine(t *testing.T) { - err0 := &Error{ - errors: []*Error{ - { - error: errTest, - }, - }, - } - err1 := &Error{ - errors: []*Error{ - { - error: errTest, - }, - { - error: errTest, - }, - }, - } - want := &Error{ - errors: []*Error{ - { - error: errTest, - }, - { - error: errTest, - }, - { - error: errTest, - }, - { - error: errTest, - }, - }, - } + err0 := &Error{} + err1 := &Error{} + want := &Error{error: errors.Join(err0, err1, errTest)} err := Combine(err0, err1, errTest) require.Equal(t, want, err) + } func TestError_Error(t *testing.T) { @@ -185,14 +79,14 @@ func TestError_HTTPStatus(t *testing.T) { serverErr := http.StatusTooManyRequests testCases := []struct { name string - httpStatus *int + httpStatus int grpcStatus *status.Status want int hasCode bool }{ { name: "Passes through HTTP status", - httpStatus: &serverErr, + httpStatus: serverErr, want: serverErr, hasCode: true, }, @@ -204,7 +98,7 @@ func TestError_HTTPStatus(t *testing.T) { }, { name: "Passes through HTTP status when gRPC status also present", - httpStatus: &serverErr, + httpStatus: serverErr, grpcStatus: status.New(codes.OK, errTest.Error()), want: serverErr, hasCode: true, @@ -237,14 +131,14 @@ func TestError_GRPCStatus(t *testing.T) { serverErr := status.New(codes.ResourceExhausted, errTest.Error()) testCases := []struct { name string - httpStatus *int + httpStatus int grpcStatus *status.Status want *status.Status hasCode bool }{ { name: "Converts HTTP status", - httpStatus: &httpStatus, + httpStatus: httpStatus, want: serverErr, hasCode: true, }, @@ -256,7 +150,7 @@ func TestError_GRPCStatus(t *testing.T) { }, { name: "Passes through gRPC status when gRPC status also present", - httpStatus: &otherHTTPStatus, + httpStatus: otherHTTPStatus, grpcStatus: serverErr, want: serverErr, hasCode: true, From ebe59e695a38c66f8809c60889ee87d79d9b6532 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 30 Aug 2024 11:47:34 -0400 Subject: [PATCH 35/37] Add more tests --- consumer/consumererror/error.go | 13 +- consumer/consumererror/error_test.go | 130 +++++++++++++++++- .../statusconversion/conversion_test.go | 2 +- 3 files changed, 135 insertions(+), 10 deletions(-) diff --git a/consumer/consumererror/error.go b/consumer/consumererror/error.go index c5fde1cd46a..ea60c4a7ed5 100644 --- a/consumer/consumererror/error.go +++ b/consumer/consumererror/error.go @@ -141,20 +141,17 @@ func (e *Error) GRPCStatus() (*status.Status, bool) { func aggregateStatuses(a int, b int) int { switch { - // If a is unset, keep b + // If a is unset, keep b. b is guaranteed to be non-zero by the caller. case a == 0: return b - // If b is unset, keep a - case b == 0: - return a - // If a and b have been set and one is a 4xx code, the correct code is - // ambiguous, so return a 400, which is permanent. - case (a >= 400 && a < 500) || (b >= 400 && b < 500): - return http.StatusBadRequest // If a and b have been set and one is a 5xx code, the correct code is // ambiguous, so return a 500, which is permanent. case a >= 500 || b >= 500: return http.StatusInternalServerError + // If a and b have been set and one is a 4xx code, the correct code is + // ambiguous, so return a 400, which is permanent. + case (a >= 400 && a < 500) || (b >= 400 && b < 500): + return http.StatusBadRequest default: return http.StatusInternalServerError } diff --git a/consumer/consumererror/error_test.go b/consumer/consumererror/error_test.go index 60dfbf11fab..1f380127930 100644 --- a/consumer/consumererror/error_test.go +++ b/consumer/consumererror/error_test.go @@ -59,6 +59,134 @@ func TestCombine(t *testing.T) { } +func TestCombineStatusAggregation(t *testing.T) { + cases := []struct { + name string + errors []error + wantHTTP int + wantGRPC codes.Code + }{ + { + name: "No status codes", + errors: []error{}, + }, + { + name: "One status code first", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + New(errTest), + }, + wantHTTP: http.StatusTooManyRequests, + wantGRPC: codes.ResourceExhausted, + }, + { + name: "One status code second", + errors: []error{ + New(errTest), + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + }, + wantHTTP: http.StatusTooManyRequests, + wantGRPC: codes.ResourceExhausted, + }, + { + name: "Two HTTP statuses", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + New(errTest, WithHTTPStatus(http.StatusNotFound)), + }, + wantHTTP: http.StatusBadRequest, + wantGRPC: codes.InvalidArgument, + }, + { + name: "Three HTTP statuses", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + New(errTest, WithHTTPStatus(http.StatusNotFound)), + New(errTest, WithHTTPStatus(http.StatusUnauthorized)), + }, + wantHTTP: http.StatusBadRequest, + wantGRPC: codes.InvalidArgument, + }, + { + name: "Two gRPC statuses", + errors: []error{ + New(errTest, WithGRPCStatus(status.New(codes.PermissionDenied, ""))), + New(errTest, WithGRPCStatus(status.New(codes.Unauthenticated, ""))), + }, + wantHTTP: http.StatusBadRequest, + wantGRPC: codes.InvalidArgument, + }, + { + name: "One HTTP, one gRPC status", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + New(errTest, WithGRPCStatus(status.New(codes.PermissionDenied, ""))), + }, + wantHTTP: http.StatusBadRequest, + wantGRPC: codes.InvalidArgument, + }, + { + name: "Two 4xx", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + New(errTest, WithHTTPStatus(http.StatusUnauthorized)), + }, + wantHTTP: http.StatusBadRequest, + wantGRPC: codes.InvalidArgument, + }, + { + name: "One 4xx, one 5xx", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusTooManyRequests)), + New(errTest, WithHTTPStatus(http.StatusServiceUnavailable)), + }, + wantHTTP: http.StatusInternalServerError, + wantGRPC: codes.Unknown, + }, + { + name: "Two 5xx", + errors: []error{ + New(errTest, WithGRPCStatus(status.New(codes.DeadlineExceeded, ""))), + New(errTest, WithHTTPStatus(http.StatusBadGateway)), + }, + wantHTTP: http.StatusInternalServerError, + wantGRPC: codes.Unknown, + }, + { + name: "Neither 4xx nor 5xx", + errors: []error{ + New(errTest, WithHTTPStatus(http.StatusPermanentRedirect)), + New(errTest, WithHTTPStatus(http.StatusTemporaryRedirect)), + }, + wantHTTP: http.StatusInternalServerError, + wantGRPC: codes.Unknown, + }, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + e := Combine(tt.errors...) + ce := &Error{} + + if errors.As(e, &ce) { + if tt.wantHTTP != 0 { + status, ok := ce.HTTPStatus() + require.True(t, ok) + require.Equal(t, tt.wantHTTP, status) + } + + if tt.wantGRPC != codes.OK { + status, ok := ce.GRPCStatus() + require.True(t, ok) + require.Equal(t, tt.wantGRPC, status.Code()) + } + } else { + require.Fail(t, "Combine did not return an Error type") + } + }) + } +} + func TestError_Error(t *testing.T) { err := &Error{ error: errTest, @@ -72,7 +200,7 @@ func TestError_Unwrap(t *testing.T) { error: errTest, } - require.Equal(t, []error{errTest}, err.Unwrap()) + require.Equal(t, errTest, err.Unwrap()) } func TestError_HTTPStatus(t *testing.T) { diff --git a/consumer/consumererror/internal/statusconversion/conversion_test.go b/consumer/consumererror/internal/statusconversion/conversion_test.go index e87294e680d..99316253c96 100644 --- a/consumer/consumererror/internal/statusconversion/conversion_test.go +++ b/consumer/consumererror/internal/statusconversion/conversion_test.go @@ -26,7 +26,7 @@ func Test_GetHTTPStatusCodeFromStatus(t *testing.T) { { name: "Non-retryable Status", input: status.New(codes.InvalidArgument, "test"), - expected: http.StatusInternalServerError, + expected: http.StatusBadRequest, }, { name: "Specifically 429", From 951b019023cdc08777ad8c980893a844b3be951e Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 30 Aug 2024 16:39:47 -0400 Subject: [PATCH 36/37] Minor updates to readme --- consumer/consumererror/README.md | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 53c985f1690..5f05f754f93 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -198,21 +198,16 @@ component can then later pull all errors out for analysis. > here is for design purposes. The code snippet may not work as-written. When a receiver gets a response that includes an error, it can get the data out -by doing something similar to the following. Note that this uses the `Error` -type, which is for reading error data, as opposed to the `ErrorContainer` type, -which is for recording errors. +by doing something similar to the following: ```go -cerr := consumererror.ErrorContainer{} +err := nextConsumer.ConsumeLogs(ctx, ld) +cerr := &consumererror.Error{} if errors.As(err, &cerr) { - errs := cerr.Errors() - - for _, e := range errs { - e.HTTPStatus() - e.Retryable() - e.Partial() - } + e.HTTPStatus() + e.Retryable() + e.Partial() } ``` From 3a56990cd4674d15aee58cc8233e6dd411e51251 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 3 Sep 2024 09:18:27 -0400 Subject: [PATCH 37/37] Clarify example --- consumer/consumererror/README.md | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/consumer/consumererror/README.md b/consumer/consumererror/README.md index 5f05f754f93..a76011b3b27 100644 --- a/consumer/consumererror/README.md +++ b/consumer/consumererror/README.md @@ -61,6 +61,14 @@ additional metadata: - `consumererror.WithGRPCStatus` - `consumererror.WithHTTPStatus` +**Example**: + +```go +consumererror.New(err, + consumererror.WithGRPCStatus(codes.InvalidArgument), +) +``` + The following options are not currently available, but may be made available in the future: @@ -87,17 +95,6 @@ Two examples: conversion, and we will simply give the status for the requested transport without performing any conversion. -**Example**: - -```go -consumererror.New(err, - consumererror.WithRetry( - consumerrerror.WithRetryDelay(10 * time.Second) - ), - consumererror.WithGRPCStatus(codes.InvalidArgument), -) -``` - ### Retrying data submission > [!WARNING] This function is currently in the design phase. It is not available