feat: HTTP headers in Error type #404

karel-rehor · 2024-08-07T08:43:38Z

Proposed Changes

Add http.Header as Header property to api/http/Error type to expose response headers for analysis and debugging.
leverage this in api/http/service,go.
add selected headers to error message log.
add new tests covering the new property.
update example to show how the new property can be used.

Checklist

codecov-commenter · 2024-08-07T08:51:44Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.99%. Comparing base (b3496ca) to head (5858f61).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   92.94%   92.99%   +0.04%     
==========================================
  Files          25       25              
  Lines        2310     2326      +16     
==========================================
+ Hits         2147     2163      +16     
  Misses        123      123              
  Partials       40       40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vlastahajek · 2024-08-07T18:53:34Z

api/write_test.go

+	assert.Equal(t, calls, 3)
+}
+
+func TestWriteErrorHeaderToString(t *testing.T) {


This is a unit test for http.Error. Move this to error_test.go

refactored to error_test.go

vlastahajek · 2024-08-07T18:54:37Z

internal/write/service.go

@@ -164,6 +164,7 @@ func (w *Service) HandleWrite(ctx context.Context, batch *Batch) error {
 		if batchToWrite != nil {
 			perror := w.WriteBatch(ctx, batchToWrite)
 			if perror != nil {
+				// fmt.Printf("DEBUG perror type %s\n", reflect.TypeOf(perror))


Remove commented code

vlastahajek · 2024-08-07T18:59:24Z

internal/write/service.go

+							"X-Influxdb-Version",
+						})
+						if len(logHeaders) > 0 {
+							logMessage += fmt.Sprintf("\nSelect Response Headers:\n%s", logHeaders)


Suggested change

logMessage += fmt.Sprintf("\nSelect Response Headers:\n%s", logHeaders)

logMessage += fmt.Sprintf("\nSelected Response Headers:\n%s", logHeaders)

Changed to "Selected"

vlastahajek · 2024-08-07T19:04:25Z

internal/write/service_test.go

+	//fmt.Printf("DEBUG err %v\n", err)
+	//fmt.Printf("DEBUG err %v\n", err)


Remove commented code

vlastahajek · 2024-08-07T19:04:31Z

internal/write/service_test.go

 	require.NotNil(t, err)
 	// 1st Batch expires and writing 2nd trows error
 	assert.Equal(t, "write failed (attempts 1): Unexpected status code 429", err.Error())
 	assert.Equal(t, 1, srv.retryQueue.list.Len())
+	// fmt.Printf("DEBUG Header len: %d\n", len(err.(*http.Error).Header))


Remove commented code

vlastahajek · 2024-08-07T19:46:49Z

internal/write/service.go

+					return &http2.Error{
+						StatusCode: int(perror.StatusCode),
+						Code:       perror.Code,
+						Message: fmt.Errorf(
+							"write failed (attempts %d): %w", batchToWrite.RetryAttempts, perror,
+						).Error(),
+						Err:        perror.Err,
+						RetryAfter: perror.RetryAfter,
+						Header:     perror.Header,


It doesn't look good from the API perspective to return http.Error from the write service. And the more serious problem is that the final message looks silly: "400 Bad Request: write failed (attempts 0): 400 Bad Request: { \"code\": \"bad request\", \"message\": \"test header\".

The solution could be to create a write error that would wrap the previous:

package write type Error struct { origin error message string } func NewError(err error, message string) *Error { return &Error{ origin: err, message: message, } } func (e *Error) Error() string { return fmt.Sprintf("%s: %s", e.message, e.origin) } func (e *Error) Unwrap() error { return e.origin }

Then this code would be:

return write.NewError(perror, fmt.Sprintf("write failed (attempts %d)", batchToWrite.RetryAttempts))

Agree. When I wrote it, I was thinking there has to be a more elegant way of implementing this. I'm still fairly new to golang and the suggested solution did not occur to me. Implemented as suggested, and with helper methods for extracting header values.

…ders.

…ttp.Error.Error() response.

…b-client-go into feat/httpErrorHeaders

vlastahajek · 2024-08-09T06:20:41Z

api/http/error_test.go

+		err := Error{
+			StatusCode: test.statusCode,
+			Code:       test.code,
+			Message:    test.message,
+			Err:        test.err,
+			RetryAfter: 0,
+			Header:     ihttp.Header{},
+		}
+		assert.Equal(t, test.expected, err.Error())


Use t.Run() construct for data-driven test (also add a test name to the table)

refactored using t.Run()

vlastahajek · 2024-08-09T06:21:05Z

api/write/error.go

+	"errors"
+	"fmt"
+	"github.com/influxdata/influxdb-client-go/v2/api/http"
+	iHttp "net/http"
+	"net/textproto"
+)


Use go imports

file removed

vlastahajek · 2024-08-09T06:21:18Z

api/write/error_test.go

+	"errors"
+	"fmt"
+	"github.com/influxdata/influxdb-client-go/v2/api/http"
+	"github.com/stretchr/testify/assert"
+	ihttp "net/http"
+	"testing"


Use go imports

file removed

vlastahajek · 2024-08-09T06:21:44Z

api/write_test.go

 	"fmt"
 	"io"
 	"math"
+	ihttp "net/http"
+	"net/http/httptest"
 	"runtime"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"


Use go imports

activated goimports in IDE. Updated.

vlastahajek · 2024-08-09T08:18:38Z

internal/write/service.go

 					}
-					return fmt.Errorf("write failed (attempts %d): %w", batchToWrite.RetryAttempts, perror)
+					return write.NewError(perror, fmt.Sprintf("write failed (retry attempts %d)", batchToWrite.RetryAttempts))


I'm sorry for the previous suggestion which now looks misleading. I looked at WriteAPI and WriteBlockingAPI and also at several issues regarding complaints about errors. The best solution will be to return perror, and log a message about attempts.

now logs failed attempt count and status code before returning perror directly.

vlastahajek · 2024-08-09T09:08:38Z

CHANGELOG.md

+
+### Features
+
+- [#404](https://github.com/influxdata/influxdb-client-go/pull/404) Expose HTTP response headers in the Error type to aid analysis and debugging of error results.  Add selected response headers to the error log.


Suggested change

- [#404](https://github.com/influxdata/influxdb-client-go/pull/404) Expose HTTP response headers in the Error type to aid analysis and debugging of error results. Add selected response headers to the error log.

- [#404](https://github.com/influxdata/influxdb-client-go/pull/404) Expose HTTP response headers in the Error type to aid analysis and debugging of error results. Add selected response headers to the error log.

Also, unified errors returned by WriteAPI, which now always returns `http.Error`

…m Service.HandleWrite

vlastahajek

Nice improvement 👍
Good job!

karel-rehor added 3 commits August 2, 2024 15:11

feat: adds HTTP response headers to Error type

97e14d3

feat: add http/Error to error Channel making Error.Header accessible

e087f4d

docs: update ExampleWriteApi_errors to show use of Header field

8203776

karel-rehor requested review from bednar and vlastahajek August 7, 2024 08:43

docs: updates CHANGELOG.md

a83a386

bednar removed their request for review August 7, 2024 11:36

karel-rehor added 2 commits August 7, 2024 13:52

chore: improve Error.HeaderToString and cover with test.

758784a

chore: better leverage http.CanonicalHeaderKey in Error.HeaderToString()

0da64b3

vlastahajek requested changes Aug 7, 2024

View reviewed changes

karel-rehor and others added 5 commits August 8, 2024 10:11

test: refactor to error_test.go and remove commented lines

4e3ff5f

feat: create write/Error wrapper to better handle http/Error with Hea…

af74d7a

…ders.

Merge branch 'master' into feat/httpErrorHeaders

5e83bf6

chore: remove case used in investigating http.Error and add test of h…

20a74f5

…ttp.Error.Error() response.

Merge branch 'feat/httpErrorHeaders' of github.com:influxdata/influxd…

767aeb8

…b-client-go into feat/httpErrorHeaders

karel-rehor requested a review from vlastahajek August 8, 2024 15:33

docs: update ExampleWriteAPI_errors

580d1ea

vlastahajek requested changes Aug 9, 2024

View reviewed changes

vlastahajek reviewed Aug 9, 2024

View reviewed changes

karel-rehor added 3 commits August 9, 2024 14:52

chore: refactor - rm write/Error, exec goimports, simplify return fro…

584c614

…m Service.HandleWrite

test: use t.Run for data driven test, exec goimports

40eaebb

docs: update CHANGELOG.md

5858f61

vlastahajek approved these changes Aug 9, 2024

View reviewed changes

karel-rehor merged commit 017c8c8 into master Aug 9, 2024
2 checks passed

bednar added this to the 2.14.0 milestone Aug 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: HTTP headers in Error type #404

feat: HTTP headers in Error type #404

karel-rehor commented Aug 7, 2024 •

edited

Loading

codecov-commenter commented Aug 7, 2024 •

edited

Loading

vlastahajek Aug 7, 2024

karel-rehor Aug 8, 2024

vlastahajek Aug 7, 2024

karel-rehor Aug 8, 2024

vlastahajek Aug 7, 2024

karel-rehor Aug 8, 2024

vlastahajek Aug 7, 2024

karel-rehor Aug 8, 2024

vlastahajek Aug 7, 2024

karel-rehor Aug 8, 2024

vlastahajek Aug 7, 2024 •

edited

Loading

karel-rehor Aug 8, 2024

vlastahajek Aug 9, 2024

karel-rehor Aug 9, 2024

vlastahajek Aug 9, 2024

karel-rehor Aug 9, 2024

vlastahajek Aug 9, 2024

karel-rehor Aug 9, 2024

vlastahajek Aug 9, 2024

karel-rehor Aug 9, 2024

vlastahajek Aug 9, 2024

karel-rehor Aug 9, 2024

vlastahajek Aug 9, 2024

vlastahajek left a comment

	logMessage += fmt.Sprintf("\nSelect Response Headers:\n%s", logHeaders)
	logMessage += fmt.Sprintf("\nSelected Response Headers:\n%s", logHeaders)

		//fmt.Printf("DEBUG err %v\n", err)
		//fmt.Printf("DEBUG err %v\n", err)


		### Features

		- [#404](https://github.com/influxdata/influxdb-client-go/pull/404) Expose HTTP response headers in the Error type to aid analysis and debugging of error results. Add selected response headers to the error log.

	- [#404](https://github.com/influxdata/influxdb-client-go/pull/404) Expose HTTP response headers in the Error type to aid analysis and debugging of error results. Add selected response headers to the error log.
	- [#404](https://github.com/influxdata/influxdb-client-go/pull/404) Expose HTTP response headers in the Error type to aid analysis and debugging of error results. Add selected response headers to the error log.
	Also, unified errors returned by WriteAPI, which now always returns `http.Error`

feat: HTTP headers in Error type #404

feat: HTTP headers in Error type #404

Conversation

karel-rehor commented Aug 7, 2024 • edited Loading

Proposed Changes

Checklist

codecov-commenter commented Aug 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek left a comment

Choose a reason for hiding this comment

karel-rehor commented Aug 7, 2024 •

edited

Loading

codecov-commenter commented Aug 7, 2024 •

edited

Loading

vlastahajek Aug 7, 2024 •

edited

Loading