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] 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())