Repository: atlassian/gostatsd
Pull request #257 (2019-08-22 02:07:52)

Revert retries logic (#252-#254) and updates documentation


Patch:
diff --git a/README.md b/README.md
index e626c7f..0903bad 100644
--- a/README.md
+++ b/README.md
@@ -72,13 +72,11 @@ following configuration options:
 - `enable-http2`: boolean to enable the usage of http2 on the request.  There seems to be some incompatibility with the
   golang http2 implementation and AWS ELB/ALBs.  If you experience strange timeouts and hangs, this should be the first
   thing to disable.  Defaults to `false`
-- `enable-retries`: This will tell the forwarder to attempt to send payloads again in case it failed for the first time, 
-   with exponential back off. Defaults to `true`
 - `api-endpoint`: configures the endpoint to submit raw metrics to.  This setting should be just a base URL, for example
   `https://statsd-aggregator.private`, with no path.  Required, no default
 - `max-requests`: maximum number of requests in flight.  Defaults to `1000` (which is probably too high)
 - `max-request-elapsed-time`: duration for the maximum amount of time to try submitting data before giving up.  This
-  includes retries.  Defaults to `30s` (which is probably too high)
+  includes retries.  Defaults to `30s` (which is probably too high). Setting this value to `-1` will disable retries.
 - `network`: the network type to use, probably `tcp`, `tcp4`, or `tcp6`.  Defaults to `tcp`
 - `consolidator-slots`: number of slots in the metric consolidator.  Memory usage is a function of this.  Lower values
   may cause blocking in the pipeline (back pressure).  A UDP only receiver will never use more than the number of
diff --git a/pkg/backends/datadog/datadog.go b/pkg/backends/datadog/datadog.go
index 90eea55..107534a 100644
--- a/pkg/backends/datadog/datadog.go
+++ b/pkg/backends/datadog/datadog.go
@@ -392,7 +392,7 @@ func NewClient(apiEndpoint, apiKey, userAgent, network string, metricsPerBatch i
 	if clientTimeout <= 0 {
 		return nil, fmt.Errorf("[%s] clientTimeout must be positive", BackendName)
 	}
-	if maxRequestElapsedTime <= 0 {
+	if maxRequestElapsedTime <= 0 && maxRequestElapsedTime != -1 {
 		return nil, fmt.Errorf("[%s] maxRequestElapsedTime must be positive", BackendName)
 	}
 
diff --git a/pkg/backends/newrelic/newrelic.go b/pkg/backends/newrelic/newrelic.go
index f5c9c70..ec97918 100644
--- a/pkg/backends/newrelic/newrelic.go
+++ b/pkg/backends/newrelic/newrelic.go
@@ -437,7 +437,7 @@ func NewClient(address, eventType, flushType, apiKey, tagPrefix,
 	if clientTimeout <= 0 {
 		return nil, fmt.Errorf("[%s] clientTimeout must be positive", BackendName)
 	}
-	if maxRequestElapsedTime <= 0 {
+	if maxRequestElapsedTime <= 0 && maxRequestElapsedTime != -1 {
 		return nil, fmt.Errorf("[%s] maxRequestElapsedTime must be positive", BackendName)
 	}
 	if !contains(flushTypes, flushType) && flushType != "" {
diff --git a/pkg/statsd/handler_http_forwarder_v2.go b/pkg/statsd/handler_http_forwarder_v2.go
index 1c051d5..1077532 100644
--- a/pkg/statsd/handler_http_forwarder_v2.go
+++ b/pkg/statsd/handler_http_forwarder_v2.go
@@ -31,7 +31,6 @@ const (
 	defaultClientTimeout             = 10 * time.Second
 	defaultCompress                  = true
 	defaultEnableHttp2               = false
-	defaultEnableRetires             = true
 	defaultApiEndpoint               = ""
 	defaultMaxRequestElapsedTime     = 30 * time.Second
 	defaultMaxRequests               = 1000
@@ -56,7 +55,6 @@ type HttpForwarderHandlerV2 struct {
 	consolidatedMetrics   <-chan []*gostatsd.MetricMap
 	eventWg               sync.WaitGroup
 	compress              bool
-	enableRetries         bool
 }
 
 // NewHttpForwarderHandlerV2FromViper returns a new http API client.
@@ -65,7 +63,6 @@ func NewHttpForwarderHandlerV2FromViper(logger logrus.FieldLogger, v *viper.Vipe
 	subViper.SetDefault("client-timeout", defaultClientTimeout)
 	subViper.SetDefault("compress", defaultCompress)
 	subViper.SetDefault("enable-http2", defaultEnableHttp2)
-	subViper.SetDefault("enable-retries", defaultEnableRetires)
 	subViper.SetDefault("api-endpoint", defaultApiEndpoint)
 	subViper.SetDefault("max-requests", defaultMaxRequests)
 	subViper.SetDefault("max-request-elapsed-time", defaultMaxRequestElapsedTime)
@@ -80,7 +77,6 @@ func NewHttpForwarderHandlerV2FromViper(logger logrus.FieldLogger, v *viper.Vipe
 		subViper.GetInt("consolidator-slots"),
 		subViper.GetInt("max-requests"),
 		subViper.GetBool("compress"),
-		subViper.GetBool("enable-retries"),
 		subViper.GetBool("enable-http2"),
 		subViper.GetDuration("client-timeout"),
 		subViper.GetDuration("max-request-elapsed-time"),
@@ -89,19 +85,7 @@ func NewHttpForwarderHandlerV2FromViper(logger logrus.FieldLogger, v *viper.Vipe
 }
 
 // NewHttpForwarderHandlerV2 returns a new handler which dispatches metrics over http to another gostatsd server.
-func NewHttpForwarderHandlerV2(
-	logger logrus.FieldLogger,
-	apiEndpoint,
-	network string,
-	consolidatorSlots,
-	maxRequests int,
-	compress,
-	enableRetries,
-	enableHttp2 bool,
-	clientTimeout,
-	maxRequestElapsedTime time.Duration,
-	flushInterval time.Duration,
-) (*HttpForwarderHandlerV2, error) {
+func NewHttpForwarderHandlerV2(logger logrus.FieldLogger, apiEndpoint, network string, consolidatorSlots, maxRequests int, compress, enableHttp2 bool, clientTimeout, maxRequestElapsedTime time.Duration, flushInterval time.Duration) (*HttpForwarderHandlerV2, error) {
 	if apiEndpoint == "" {
 		return nil, fmt.Errorf("api-endpoint is required")
 	}
@@ -114,7 +98,7 @@ func NewHttpForwarderHandlerV2(
 	if clientTimeout <= 0 {
 		return nil, fmt.Errorf("client-timeout must be positive")
 	}
-	if maxRequestElapsedTime <= 0 {
+	if maxRequestElapsedTime <= 0 && maxRequestElapsedTime != -1 {
 		return nil, fmt.Errorf("max-request-elapsed-time must be positive")
 	}
 	if flushInterval <= 0 {
@@ -126,7 +110,6 @@ func NewHttpForwarderHandlerV2(
 		"client-timeout":           clientTimeout,
 		"compress":                 compress,
 		"enable-http2":             enableHttp2,
-		"enable-retries":           enableRetries,
 		"max-request-elapsed-time": maxRequestElapsedTime,
 		"max-requests":             maxRequests,
 		"consolidator-slots":       consolidatorSlots,
@@ -179,7 +162,6 @@ func NewHttpForwarderHandlerV2(
 			Transport: transport,
 			Timeout:   clientTimeout,
 		},
-		enableRetries: enableRetries,
 	}, nil
 }
 
@@ -359,7 +341,7 @@ func (hfh *HttpForwarderHandlerV2) post(ctx context.Context, message proto.Messa
 		}
 
 		next := b.NextBackOff()
-		if next == backoff.Stop || !hfh.enableRetries {
+		if next == backoff.Stop {
 			atomic.AddUint64(&hfh.messagesDropped, 1)
 			logger.WithError(err).Info("failed to send, giving up")
 			return


Test patch:
diff --git a/pkg/web/http_receiver_v2_test.go b/pkg/web/http_receiver_v2_test.go
index 71ced27..f5ac86b 100644
--- a/pkg/web/http_receiver_v2_test.go
+++ b/pkg/web/http_receiver_v2_test.go
@@ -46,7 +46,6 @@ func TestForwardingEndToEndV2(t *testing.T) {
 		5, // deliberately prime, so the loop below doesn't send the same thing to the same MetricMap every time.
 		10,
 		false,
-		true,
 		false,
 		10*time.Second,
 		10*time.Second,