Repository: open-telemetry/opamp-go
Pull request #123 (2022-08-07 13:54:10)

fix: incorrect switch case eval when server response code is 429
While working on tests for http sender I found that it doesn't respect the `retry-after` for too many requests response. I believe this is a bug.

codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/123?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
> Merging [#123](https://codecov.io/gh/open-telemetry/opamp-go/pull/123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (dbf2b5d) into [main](https://codecov.io/gh/open-telemetry/opamp-go/commit/1993a3803e4228f0b37e6d8105bc4a4410a90b0f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (1993a38) will **increase** coverage by `0.10%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
+ Coverage   75.49%   75.60%   +0.10%     
==========================================
  Files          23       23              
  Lines        1706     1705       -1     
==========================================
+ Hits         1288     1289       +1     
+ Misses        313      312       -1     
+ Partials      105      104       -1     
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opamp-go/pull/123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [client/internal/httpsender.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2h0dHBzZW5kZXIuZ28=) | `72.79% <100.00%> (+0.53%)` | :arrow_up: |
| [client/internal/tcpproxy.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL3RjcHByb3h5Lmdv) | `76.19% <0.00%> (-4.77%)` | :arrow_down: |
| [client/wsclient.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L3dzY2xpZW50Lmdv) | `87.19% <0.00%> (+2.43%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)


Patch:
diff --git a/client/internal/httpsender.go b/client/internal/httpsender.go
index cd2f25c..b2a9938 100644
--- a/client/internal/httpsender.go
+++ b/client/internal/httpsender.go
@@ -162,8 +162,7 @@ func (h *HTTPSender) sendRequestWithRetries(ctx context.Context) (*http.Response
 						h.callbacks.OnConnect()
 						return resp, nil
 
-					case http.StatusTooManyRequests:
-					case http.StatusServiceUnavailable:
+					case http.StatusTooManyRequests, http.StatusServiceUnavailable:
 						interval = recalculateInterval(interval, resp)
 						err = fmt.Errorf("server response code=%d", resp.StatusCode)
 


Test patch:
diff --git a/client/internal/httpsender_test.go b/client/internal/httpsender_test.go
new file mode 100644
index 0000000..31f1aab
--- /dev/null
+++ b/client/internal/httpsender_test.go
@@ -0,0 +1,57 @@
+package internal
+
+import (
+	"context"
+	"net/http"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/open-telemetry/opamp-go/client/types"
+	sharedinternal "github.com/open-telemetry/opamp-go/internal"
+	"github.com/open-telemetry/opamp-go/protobufs"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestHTTPSenderRetryForStatusTooManyRequests(t *testing.T) {
+
+	var connectionAttempts int64
+	srv := StartMockServer(t)
+	srv.OnRequest = func(w http.ResponseWriter, r *http.Request) {
+		attempt := atomic.AddInt64(&connectionAttempts, 1)
+		// Return a Retry-After header with a value of 1 second for first attempt.
+		if attempt == 1 {
+			w.Header().Set("Retry-After", "1")
+			w.WriteHeader(http.StatusTooManyRequests)
+		} else {
+			w.WriteHeader(http.StatusOK)
+		}
+	}
+	ctx, cancel := context.WithCancel(context.Background())
+	url := "http://" + srv.Endpoint
+	sender := NewHTTPSender(&sharedinternal.NopLogger{})
+	sender.NextMessage().Update(func(msg *protobufs.AgentToServer) {
+		msg.AgentDescription = &protobufs.AgentDescription{
+			IdentifyingAttributes: []*protobufs.KeyValue{{
+				Key: "service.name",
+				Value: &protobufs.AnyValue{
+					Value: &protobufs.AnyValue_StringValue{StringValue: "test-service"},
+				},
+			}},
+		}
+	})
+	sender.callbacks = types.CallbacksStruct{
+		OnConnectFunc: func() {
+		},
+		OnConnectFailedFunc: func(_ error) {
+		},
+	}
+	sender.url = url
+	start := time.Now()
+	resp, err := sender.sendRequestWithRetries(ctx)
+	assert.NoError(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	assert.True(t, time.Since(start) > time.Second)
+	cancel()
+	srv.Close()
+}