From 54250ef48e295a9ec945749cab1b2c2f5a00ef77 Mon Sep 17 00:00:00 2001 From: Teodora Sandu <81559517+teodora-sandu@users.noreply.github.com> Date: Fri, 19 Apr 2024 16:49:10 +0100 Subject: [PATCH] refactor: http logger option (#33) --- http/http.go | 37 +++++++++++++++++------------- http/http_test.go | 30 +++++++++++++----------- internal/analysis/analysis_test.go | 2 +- 3 files changed, 38 insertions(+), 31 deletions(-) diff --git a/http/http.go b/http/http.go index 452e0dc..576151b 100644 --- a/http/http.go +++ b/http/http.go @@ -66,6 +66,7 @@ func WithErrorReporter(errorReporter observability.ErrorReporter) OptionFunc { func WithLogger(logger *zerolog.Logger) OptionFunc { return func(h *httpClient) { h.logger = logger + h.errorReporter = observability.NewErrorReporter(logger) } } @@ -98,36 +99,40 @@ var retryErrorCodes = map[int]bool{ http.StatusInternalServerError: true, } -func (s *httpClient) Do(req *http.Request) (response *http.Response, err error) { +func (s *httpClient) Do(req *http.Request) (*http.Response, error) { span := s.instrumentor.StartSpan(req.Context(), "http.Do") defer s.instrumentor.Finish(span) - for i := 0; i < s.retryCount; i++ { + retryCount := s.retryCount + for { requestId := span.GetTraceId() req.Header.Set("snyk-request-id", requestId) - response, err = s.httpCall(req) + response, err := s.httpCall(req) if err != nil { return nil, err // no retries for errors } - if retryErrorCodes[response.StatusCode] { - s.logger.Debug().Err(err).Str("method", req.Method).Int("attempts done", i+1).Msg("retrying") - if i < s.retryCount-1 { - time.Sleep(5 * time.Second) - continue - } + if retryCount > 0 && retryErrorCodes[response.StatusCode] { + s.logger.Debug().Err(err).Int("attempts left", retryCount).Msg("retrying") + retryCount-- + time.Sleep(5 * time.Second) + continue } - // no error, we can break the retry loop - break + // should return + return response, err } - return response, err } func (s *httpClient) httpCall(req *http.Request) (*http.Response, error) { - log := s.logger.With().Str("method", "http.httpCall").Logger() requestId := req.Header.Get("snyk-request-id") + log := s.logger.With(). + Str("method", "http.httpCall"). + Str("reqMethod", req.Method). + Str("url", req.URL.String()). + Str("snyk-request-id", requestId). + Logger() // store the request body so that after retrying it can be read again var copyReqBody io.ReadCloser @@ -137,7 +142,7 @@ func (s *httpClient) httpCall(req *http.Request) (*http.Response, error) { reqBody := io.NopCloser(bytes.NewBuffer(reqBuf)) copyReqBody = io.NopCloser(bytes.NewBuffer(reqBuf)) req.Body = reqBody - s.logger.Debug().Str("url", req.URL.String()).Str("snyk-request-id", requestId).Str("requestBody", string(reqBuf)).Msg("SEND TO REMOTE") + s.logger.Debug().Msg("SEND TO REMOTE") } response, err := s.httpClientFactory().Do(req) req.Body = copyReqBody @@ -147,9 +152,9 @@ func (s *httpClient) httpCall(req *http.Request) (*http.Response, error) { resBuf, _ = io.ReadAll(response.Body) copyResBody = io.NopCloser(bytes.NewBuffer(resBuf)) response.Body = copyResBody - s.logger.Debug().Str("url", req.URL.String()).Str("response.Status", response.Status).Str("snyk-request-id", requestId).Str("responseBody", string(resBuf)).Msg("RECEIVED FROM REMOTE") + s.logger.Debug().Str("response.Status", response.Status).Msg("RECEIVED FROM REMOTE") } else { - s.logger.Debug().Str("url", req.URL.String()).Str("snyk-request-id", requestId).Msg("RECEIVED FROM REMOTE") + s.logger.Debug().Msg("RECEIVED FROM REMOTE") } if err != nil { diff --git a/http/http_test.go b/http/http_test.go index 2c8b3c1..2b08b59 100644 --- a/http/http_test.go +++ b/http/http_test.go @@ -28,7 +28,6 @@ import ( "github.com/stretchr/testify/require" codeClientHTTP "github.com/snyk/code-client-go/http" - "github.com/snyk/code-client-go/observability" "github.com/snyk/code-client-go/observability/mocks" ) @@ -56,7 +55,7 @@ func (d *dummyTransport) RoundTrip(req *http.Request) (*http.Response, error) { }, nil } -func TestSnykCodeBackendService_DoCall_shouldRetry(t *testing.T) { +func TestSnykCodeBackendService_DoCall_shouldRetryWithARequestBody(t *testing.T) { d := &dummyTransport{responseCode: 502, status: "502 Bad Gateway"} dummyClientFactory := func() *http.Client { return &http.Client{ @@ -72,7 +71,7 @@ func TestSnykCodeBackendService_DoCall_shouldRetry(t *testing.T) { mockInstrumentor.EXPECT().Finish(gomock.Any()).Times(1) mockErrorReporter := mocks.NewMockErrorReporter(ctrl) - req, err := http.NewRequest(http.MethodGet, "https://httpstat.us/500", nil) + req, err := http.NewRequest(http.MethodGet, "https://127.0.0.1", io.NopCloser(strings.NewReader("body"))) require.NoError(t, err) s := codeClientHTTP.NewHTTPClient( @@ -85,11 +84,11 @@ func TestSnykCodeBackendService_DoCall_shouldRetry(t *testing.T) { res, err := s.Do(req) assert.NoError(t, err) assert.NotNil(t, res) - assert.Equal(t, 3, d.calls) + assert.Equal(t, 4, d.calls) } -func TestSnykCodeBackendService_DoCall_shouldRetryWithARequestBody(t *testing.T) { - d := &dummyTransport{responseCode: 502, status: "502 Bad Gateway"} +func TestSnykCodeBackendService_DoCall_shouldSucceed(t *testing.T) { + d := &dummyTransport{responseCode: 200} dummyClientFactory := func() *http.Client { return &http.Client{ Transport: d, @@ -104,7 +103,7 @@ func TestSnykCodeBackendService_DoCall_shouldRetryWithARequestBody(t *testing.T) mockInstrumentor.EXPECT().Finish(gomock.Any()).Times(1) mockErrorReporter := mocks.NewMockErrorReporter(ctrl) - req, err := http.NewRequest(http.MethodGet, "https://httpstat.us/500", io.NopCloser(strings.NewReader("body"))) + req, err := http.NewRequest(http.MethodGet, "https://httpstat.us/200", nil) require.NoError(t, err) s := codeClientHTTP.NewHTTPClient( @@ -117,12 +116,15 @@ func TestSnykCodeBackendService_DoCall_shouldRetryWithARequestBody(t *testing.T) res, err := s.Do(req) assert.NoError(t, err) assert.NotNil(t, res) - assert.Equal(t, 3, d.calls) + assert.Equal(t, 1, d.calls) } -func TestSnykCodeBackendService_doCall_rejected(t *testing.T) { +func TestSnykCodeBackendService_DoCall_shouldFail(t *testing.T) { + d := &dummyTransport{responseCode: 400, status: "400 Bad Request"} dummyClientFactory := func() *http.Client { - return &http.Client{} + return &http.Client{ + Transport: d, + } } ctrl := gomock.NewController(t) @@ -132,20 +134,20 @@ func TestSnykCodeBackendService_doCall_rejected(t *testing.T) { mockInstrumentor.EXPECT().StartSpan(gomock.Any(), gomock.Any()).Return(mockSpan).Times(1) mockInstrumentor.EXPECT().Finish(gomock.Any()).Times(1) mockErrorReporter := mocks.NewMockErrorReporter(ctrl) - mockErrorReporter.EXPECT().CaptureError(gomock.Any(), observability.ErrorReporterOptions{ErrorDiagnosticPath: ""}) req, err := http.NewRequest(http.MethodGet, "https://127.0.0.1", nil) require.NoError(t, err) s := codeClientHTTP.NewHTTPClient( dummyClientFactory, - codeClientHTTP.WithRetryCount(3), + codeClientHTTP.WithRetryCount(1), codeClientHTTP.WithInstrumentor(mockInstrumentor), codeClientHTTP.WithErrorReporter(mockErrorReporter), codeClientHTTP.WithLogger(newLogger(t)), ) - _, err = s.Do(req) - assert.Error(t, err) + response, err := s.Do(req) + assert.Equal(t, "400 Bad Request", response.Status) + assert.NoError(t, err) } func newLogger(t *testing.T) *zerolog.Logger { diff --git a/internal/analysis/analysis_test.go b/internal/analysis/analysis_test.go index bf28281..86c565c 100644 --- a/internal/analysis/analysis_test.go +++ b/internal/analysis/analysis_test.go @@ -209,7 +209,7 @@ func TestAnalysis_CreateWorkspace_KnownErrors(t *testing.T) { logger := zerolog.Nop() - analysisOrchestrator := analysis.NewAnalysisOrchestrator(&logger, mockHTTPClient, mockInstrumentor, mockErrorReporter, mockConfig) + analysisOrchestrator := analysis.NewAnalysisOrchestrator(mockConfig, &logger, mockHTTPClient, mockInstrumentor, mockErrorReporter) _, err := analysisOrchestrator.CreateWorkspace( context.Background(), "4a72d1db-b465-4764-99e1-ecedad03b06a",