Merge pull request #3 from willnewrelic/infinity

Properly handle NaN and infinity

CHANGELOG.md

@@ -1,5 +1,11 @@
 ## ChangeLog
 
+* The SDK will now check metrics for infinity and NaN.  Metrics with invalid
+values will be rejected, and will result in an error logged.
+
+* Added `Config.Product` and `Config.ProductVersion` fields which are
+used to the `User-Agent` header if set.
+
 ## 0.1.0
 
 First release!

internal/attributes_test.go

@@ -5,6 +5,7 @@ package internal
 
 import (
 	"bytes"
+	"math"
 	"strconv"
 	"testing"
 )
@@ -32,6 +33,9 @@ func TestAttributesWriteJSON(t *testing.T) {
 		{"float32", float32(1), `{"float32":1}`},
 		{"float64", float64(1), `{"float64":1}`},
 		{"default", func() {}, `{"default":"func()"}`},
+		{"NaN", math.NaN(), `{"NaN":"NaN"}`},
+		{"positive-infinity", math.Inf(1), `{"positive-infinity":"infinity"}`},
+		{"negative-infinity", math.Inf(-1), `{"negative-infinity":"infinity"}`},
 	}
 
 	for _, test := range tests {

internal/jsonx/encode.go

@@ -8,9 +8,7 @@ package jsonx
 
 import (
 	"bytes"
-	"encoding/json"
 	"math"
-	"reflect"
 	"strconv"
 	"unicode/utf8"
 )
@@ -104,33 +102,30 @@ func AppendStringArray(buf *bytes.Buffer, a ...string) {
 }
 
 // AppendFloat appends a numeric literal representing the value to buf.
-func AppendFloat(buf *bytes.Buffer, x float64) error {
+func AppendFloat(buf *bytes.Buffer, x float64) {
 	var scratch [64]byte
 
-	if math.IsInf(x, 0) || math.IsNaN(x) {
-		return &json.UnsupportedValueError{
-			Value: reflect.ValueOf(x),
-			Str:   strconv.FormatFloat(x, 'g', -1, 64),
-		}
+	if math.IsInf(x, 0) {
+		AppendString(buf, "infinity")
+		return
+	}
+	if math.IsNaN(x) {
+		AppendString(buf, "NaN")
+		return
 	}
-
 	buf.Write(strconv.AppendFloat(scratch[:0], x, 'g', -1, 64))
-	return nil
 }
 
 // AppendFloatArray appends an array of numeric literals to buf.
-func AppendFloatArray(buf *bytes.Buffer, a ...float64) error {
+func AppendFloatArray(buf *bytes.Buffer, a ...float64) {
 	buf.WriteByte('[')
 	for i, x := range a {
 		if i > 0 {
 			buf.WriteByte(',')
 		}
-		if err := AppendFloat(buf, x); err != nil {
-			return err
-		}
+		AppendFloat(buf, x)
 	}
 	buf.WriteByte(']')
-	return nil
 }
 
 // AppendInt appends a numeric literal representing the value to buf.

internal/jsonx/encode_test.go

@@ -12,20 +12,21 @@ import (
 
 func TestAppendFloat(t *testing.T) {
 	buf := &bytes.Buffer{}
-
-	err := AppendFloat(buf, math.NaN())
-	if err == nil {
-		t.Error("AppendFloat(NaN) should return an error")
+	AppendFloat(buf, math.NaN())
+	if want, got := `"NaN"`, buf.String(); want != got {
+		t.Error(got, want)
 	}
 
-	err = AppendFloat(buf, math.Inf(1))
-	if err == nil {
-		t.Error("AppendFloat(+Inf) should return an error")
+	buf.Reset()
+	AppendFloat(buf, math.Inf(1))
+	if want, got := `"infinity"`, buf.String(); want != got {
+		t.Error(got, want)
 	}
 
-	err = AppendFloat(buf, math.Inf(-1))
-	if err == nil {
-		t.Error("AppendFloat(-Inf) should return an error")
+	buf.Reset()
+	AppendFloat(buf, math.Inf(-1))
+	if want, got := `"infinity"`, buf.String(); want != got {
+		t.Error(got, want)
 	}
 }
 

telemetry/aggregated_metrics.go

@@ -5,9 +5,26 @@ package telemetry
 
 import (
 	"encoding/json"
+	"errors"
+	"math"
 	"time"
 )
 
+var (
+	errFloatInfinity = errors.New("invalid float is infinity")
+	errFloatNaN      = errors.New("invalid float is NaN")
+)
+
+func isFloatValid(f float64) error {
+	if math.IsInf(f, 0) {
+		return errFloatInfinity
+	}
+	if math.IsNaN(f) {
+		return errFloatNaN
+	}
+	return nil
+}
+
 // AggregatedCount is the metric type that counts the number of times an event occurred.
 // This counter is reset every time the data is reported, meaning the value
 // reported represents the difference in count over the reporting time window.
@@ -44,6 +61,14 @@ func (c *AggregatedCount) Increase(val float64) {
 	h.lock.Lock()
 	defer h.lock.Unlock()
 
+	if err := isFloatValid(val); err != nil {
+		h.config.logError(map[string]interface{}{
+			"message": "invalid aggregated count value",
+			"err":     err.Error(),
+		})
+		return
+	}
+
 	m := h.findOrCreateMetric(c.metricIdentity)
 	if nil == m.c {
 		m.c = &Count{
@@ -83,6 +108,14 @@ func (g *AggregatedGauge) valueNow(val float64, now time.Time) {
 	h.lock.Lock()
 	defer h.lock.Unlock()
 
+	if err := isFloatValid(val); err != nil {
+		h.config.logError(map[string]interface{}{
+			"message": "invalid aggregated gauge value",
+			"err":     err.Error(),
+		})
+		return
+	}
+
 	m := h.findOrCreateMetric(g.metricIdentity)
 	if nil == m.g {
 		m.g = &Gauge{
@@ -129,6 +162,14 @@ func (s *AggregatedSummary) Record(val float64) {
 	h.lock.Lock()
 	defer h.lock.Unlock()
 
+	if err := isFloatValid(val); err != nil {
+		h.config.logError(map[string]interface{}{
+			"message": "invalid aggregated summary value",
+			"err":     err.Error(),
+		})
+		return
+	}
+
 	m := h.findOrCreateMetric(s.metricIdentity)
 	if nil == m.s {
 		m.s = &Summary{

telemetry/aggregated_metrics_test.go

@@ -4,7 +4,9 @@
 package telemetry
 
 import (
+	"math"
 	"os"
+	"reflect"
 	"strconv"
 	"testing"
 	"time"
@@ -208,3 +210,71 @@ func TestSummaryMinMax(t *testing.T) {
 	expect := `[{"name":"sum","type":"summary","value":{"sum":6,"count":3,"min":1,"max":3},"attributes":{}}]`
 	testHarvesterMetrics(t, h, expect)
 }
+
+func configSaveErrors(savedErrors *[]map[string]interface{}) func(cfg *Config) {
+	return func(cfg *Config) {
+		cfg.ErrorLogger = func(e map[string]interface{}) {
+			*savedErrors = append(*savedErrors, e)
+		}
+	}
+}
+
+func TestInvalidAggregatedSummaryValue(t *testing.T) {
+	var summary *AggregatedSummary
+	summary.Record(math.NaN())
+
+	var savedErrors []map[string]interface{}
+	h, _ := NewHarvester(configTesting, configSaveErrors(&savedErrors))
+	summary = h.MetricAggregator().Summary("summary", map[string]interface{}{})
+	summary.Record(math.NaN())
+
+	if len(savedErrors) != 1 || !reflect.DeepEqual(savedErrors[0], map[string]interface{}{
+		"err":     errFloatNaN.Error(),
+		"message": "invalid aggregated summary value",
+	}) {
+		t.Error(savedErrors)
+	}
+	if len(h.aggregatedMetrics) != 0 {
+		t.Error(h.aggregatedMetrics)
+	}
+}
+
+func TestInvalidAggregatedCountValue(t *testing.T) {
+	var count *AggregatedCount
+	count.Increase(math.Inf(1))
+
+	var savedErrors []map[string]interface{}
+	h, _ := NewHarvester(configTesting, configSaveErrors(&savedErrors))
+	count = h.MetricAggregator().Count("count", map[string]interface{}{})
+	count.Increase(math.Inf(1))
+
+	if len(savedErrors) != 1 || !reflect.DeepEqual(savedErrors[0], map[string]interface{}{
+		"err":     errFloatInfinity.Error(),
+		"message": "invalid aggregated count value",
+	}) {
+		t.Error(savedErrors)
+	}
+	if len(h.aggregatedMetrics) != 0 {
+		t.Error(h.aggregatedMetrics)
+	}
+}
+
+func TestInvalidAggregatedGaugeValue(t *testing.T) {
+	var gauge *AggregatedGauge
+	gauge.Value(math.Inf(-1))
+
+	var savedErrors []map[string]interface{}
+	h, _ := NewHarvester(configTesting, configSaveErrors(&savedErrors))
+	gauge = h.MetricAggregator().Gauge("gauge", map[string]interface{}{})
+	gauge.Value(math.Inf(-1))
+
+	if len(savedErrors) != 1 || !reflect.DeepEqual(savedErrors[0], map[string]interface{}{
+		"err":     errFloatInfinity.Error(),
+		"message": "invalid aggregated gauge value",
+	}) {
+		t.Error(savedErrors)
+	}
+	if len(h.aggregatedMetrics) != 0 {
+		t.Error(h.aggregatedMetrics)
+	}
+}

telemetry/harvester.go

@@ -136,6 +136,11 @@ func (h *Harvester) RecordMetric(m Metric) {
 	h.lock.Lock()
 	defer h.lock.Unlock()
 
+	if fields := m.validate(); nil != fields {
+		h.config.logError(fields)
+		return
+	}
+
 	h.rawMetrics = append(h.rawMetrics, m)
 }
 

telemetry/harvester_test.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"math"
 	"net/http"
 	"reflect"
 	"sort"
@@ -604,3 +605,22 @@ func TestRequiredSpanFields(t *testing.T) {
 		t.Error(err)
 	}
 }
+
+func TestRecordInvalidMetric(t *testing.T) {
+	var savedErrors []map[string]interface{}
+	h, _ := NewHarvester(configTesting, configSaveErrors(&savedErrors))
+	h.RecordMetric(Count{
+		Name:  "bad-metric",
+		Value: math.NaN(),
+	})
+	if len(savedErrors) != 1 || !reflect.DeepEqual(savedErrors[0], map[string]interface{}{
+		"err":     errFloatNaN.Error(),
+		"message": "invalid count value",
+		"name":    "bad-metric",
+	}) {
+		t.Error(savedErrors)
+	}
+	if len(h.rawMetrics) != 0 {
+		t.Error(len(h.rawMetrics))
+	}
+}

telemetry/metrics.go

@@ -41,9 +41,21 @@ type Count struct {
 	Interval time.Duration
 }
 
+func (m Count) validate() map[string]interface{} {
+	if err := isFloatValid(m.Value); err != nil {
+		return map[string]interface{}{
+			"message": "invalid count value",
+			"name":    m.Name,
+			"err":     err.Error(),
+		}
+	}
+	return nil
+}
+
 // Metric is implemented by Count, Gauge, and Summary.
 type Metric interface {
 	writeJSON(buf *bytes.Buffer)
+	validate() map[string]interface{}
 }
 
 func writeTimestampInterval(w *internal.JSONFieldsWriter, timestamp time.Time, interval time.Duration) {
@@ -104,6 +116,24 @@ type Summary struct {
 	Interval time.Duration
 }
 
+func (m Summary) validate() map[string]interface{} {
+	for _, v := range []float64{
+		m.Count,
+		m.Sum,
+		m.Min,
+		m.Max,
+	} {
+		if err := isFloatValid(v); err != nil {
+			return map[string]interface{}{
+				"message": "invalid summary field",
+				"name":    m.Name,
+				"err":     err.Error(),
+			}
+		}
+	}
+	return nil
+}
+
 func (m Summary) writeJSON(buf *bytes.Buffer) {
 	w := internal.JSONFieldsWriter{Buf: buf}
 	buf.WriteByte('{')
@@ -155,6 +185,17 @@ type Gauge struct {
 	Timestamp time.Time
 }
 
+func (m Gauge) validate() map[string]interface{} {
+	if err := isFloatValid(m.Value); err != nil {
+		return map[string]interface{}{
+			"message": "invalid gauge field",
+			"name":    m.Name,
+			"err":     err.Error(),
+		}
+	}
+	return nil
+}
+
 func (m Gauge) writeJSON(buf *bytes.Buffer) {
 	w := internal.JSONFieldsWriter{Buf: buf}
 	buf.WriteByte('{')