Repository: atlassian/gostatsd
Pull request #376 (2021-08-11 14:22:41)

Improved New Relic backend to be resilient to special floats
### Changes
- Improved New Relic backend to be resilient to special float values (`+Inf`, `-Inf`, `NaN`) which break `json.Marshal`
- Updated tests

### Description
This PR introduces changes in how tags are processed for New Relic backend. Now, when a special float (NaN, -Inf, +Inf) is found it is replaced with its string representation instead of using the float representation.
The same approach has been used in the past for `infinite` (float `+Inf`) but not for `-infinite` (float `-Inf`) neither `NaN`. This cleanup logic aims to avoid tag's values that break `json.Marshal`

### Context

We have been investigating an issue in our setup of GoStatsD using New Relic backend. It seemed to be a data lost issue related with UDP network. After several tests we realised the network traffic was OK but there was a log line in GoStatsD saying that was imposible to marshal data coming from New Relic backend.
```json
{ "error": "[newrelic] unable to marshal: json: unsupported value: NaN", "level": "error", "msg": "Sending metrics to backend failed", "time": "2021-XX-XXT00:00:00Z" }
```

So I've decided to dig into the New Relic backend implementation and realised that when a marshalling error occurs it discards the entire batch, that supported the data lost effect.

This situation, indicates some client is sending `NaN` values to GoStatsD.

To validate the hypothesis we decided to run GoStatsD with `--verbose` and also `log-raw-metric` and after some minutes, we found some metrics with **NaN values in tags!**

Go's [Json float encoder](https://cs.opensource.google/go/go/+/master:src/encoding/json/encode.go;l=577-578) returns an error with `NaN` and `Inf` (`-Inf` and `+Inf`) values.

We do not have control over all the clients which sends data, so trying to avoid sending those values is not a valid solution. Even though, I prefer to make GoStatsD more **resilient** to this kind of situations to minimise data loss.


tiedotguy: lgtm

Patch:
diff --git a/pkg/backends/newrelic/newrelic.go b/pkg/backends/newrelic/newrelic.go
index b665f16..953373c 100644
--- a/pkg/backends/newrelic/newrelic.go
+++ b/pkg/backends/newrelic/newrelic.go
@@ -595,12 +595,20 @@ func (n *Client) newMetricsPayload(data []interface{}) NRMetricsPayload {
 	}
 }
 
+// isSpecial returns whether value is special (NaN, -Inf, +Inf) or not.
+// Float representation of those values break JSON marshalling
+// so to make gostatsd more resilient we should avoid those values as Float
+func isSpecial(value float64) bool {
+	return math.IsInf(value, 0) || math.IsNaN(value)
+}
+
 func (n *Client) setTags(tags gostatsd.Tags, data map[string]interface{}) {
 	for _, tag := range tags {
 		if strings.Contains(tag, ":") {
 			keyvalpair := strings.SplitN(tag, ":", 2)
 			parsed, err := strconv.ParseFloat(keyvalpair[1], 64)
-			if err != nil || strings.EqualFold(keyvalpair[1], "infinity") {
+
+			if err != nil || isSpecial(parsed) {
 				data[n.tagPrefix+keyvalpair[0]] = keyvalpair[1]
 			} else {
 				data[n.tagPrefix+keyvalpair[0]] = parsed


Test patch:
diff --git a/pkg/backends/newrelic/newrelic_test.go b/pkg/backends/newrelic/newrelic_test.go
index 33d7848..24888f5 100644
--- a/pkg/backends/newrelic/newrelic_test.go
+++ b/pkg/backends/newrelic/newrelic_test.go
@@ -369,15 +369,18 @@ func TestEventFormatter(t *testing.T) {
 		{
 			name: "infra",
 			expected: `{"name":"com.newrelic.gostatsd","protocol_version":"2","integration_version":"2.3.1","data":` +
-				`[{"metrics":[{"AggregationKey":"","AlertType":"","DateHappened":0,"Hostname":"blah","Priority":"low","SourceTypeName":"","Text":"hi","Title":"EventTitle","event_type":"GoStatsD","name":"event"}]}]}`,
+				`[{"metrics":[{"AggregationKey":"","AlertType":"","DateHappened":0,"Hostname":"blah","Priority":"low","SourceTypeName":"","Text":"hi","Title":"EventTitle","event_type":"GoStatsD","name":"event",` +
+				`"tag_1":"-infinity","tag_2":"infinity","tag_3":"+infinity","tag_4":"NaN"}]}]}`,
 		},
 		{
-			name:     "insights",
-			expected: `[{"AggregationKey":"","AlertType":"","DateHappened":0,"Hostname":"blah","Priority":"low","SourceTypeName":"","Text":"hi","Title":"EventTitle","eventType":"GoStatsD","name":"event"}]`,
+			name: "insights",
+			expected: `[{"AggregationKey":"","AlertType":"","DateHappened":0,"Hostname":"blah","Priority":"low","SourceTypeName":"","Text":"hi","Title":"EventTitle","eventType":"GoStatsD","name":"event",` +
+				`"tag_1":"-infinity","tag_2":"infinity","tag_3":"+infinity","tag_4":"NaN"}]`,
 		},
 		{
-			name:     "metrics",
-			expected: `[{"AggregationKey":"","AlertType":"","DateHappened":0,"Hostname":"blah","Priority":"low","SourceTypeName":"","Text":"hi","Title":"EventTitle","eventType":"GoStatsD","name":"event"}]`,
+			name: "metrics",
+			expected: `[{"AggregationKey":"","AlertType":"","DateHappened":0,"Hostname":"blah","Priority":"low","SourceTypeName":"","Text":"hi","Title":"EventTitle","eventType":"GoStatsD","name":"event",` +
+				`"tag_1":"-infinity","tag_2":"infinity","tag_3":"+infinity","tag_4":"NaN"}]`,
 		},
 	}
 
@@ -393,7 +396,8 @@ func TestEventFormatter(t *testing.T) {
 				defaultMetricsPerBatch, defaultMaxRequests, 2*time.Second, 1*time.Second, gostatsd.TimerSubtypes{}, logrus.New(), p)
 			require.NoError(t, err)
 
-			gostatsdEvent := gostatsd.Event{Title: "EventTitle", Text: "hi", Source: "blah", Priority: 1}
+			tags := []string{"tag_1:-infinity", "tag_2:infinity", "tag_3:+infinity", "tag_4:NaN"}
+			gostatsdEvent := gostatsd.Event{Title: "EventTitle", Text: "hi", Source: "blah", Priority: 1, Tags: tags}
 			formattedEvent := client.EventFormatter(&gostatsdEvent)
 			fevent, err := json.Marshal(formattedEvent)
 			require.NoError(t, err)