Merge pull request #1646 from prometheus/beorn7/valuecomparison

Correctly identify no-op appends if the value is NaN

storage/local/storage.go

@@ -640,7 +640,7 @@ func (s *memorySeriesStorage) Append(sample *model.Sample) error {
 		// (e.g. Pushgateway or federation).
 		if sample.Timestamp == series.lastTime &&
 			series.lastSampleValueSet &&
-			sample.Value == series.lastSampleValue {
+			sample.Value.Equal(series.lastSampleValue) {
 			return nil
 		}
 		s.discardedSamplesCount.WithLabelValues(duplicateSample).Inc()

storage/local/storage_test.go

@@ -16,9 +16,9 @@ package local
 import (
 	"fmt"
 	"hash/fnv"
+	"math"
 	"math/rand"
 	"os"
-	"reflect"
 	"testing"
 	"testing/quick"
 	"time"
@@ -1768,12 +1768,83 @@ func TestAppendOutOfOrder(t *testing.T) {
 		model.MetricNameLabel: "out_of_order",
 	}
 
-	for i, t := range []int{0, 2, 2, 1} {
-		s.Append(&model.Sample{
+	tests := []struct {
+		name      string
+		timestamp model.Time
+		value     model.SampleValue
+		wantErr   error
+	}{
+		{
+			name:      "1st sample",
+			timestamp: 0,
+			value:     0,
+			wantErr:   nil,
+		},
+		{
+			name:      "regular append",
+			timestamp: 2,
+			value:     1,
+			wantErr:   nil,
+		},
+		{
+			name:      "same timestamp, same value (no-op)",
+			timestamp: 2,
+			value:     1,
+			wantErr:   nil,
+		},
+		{
+			name:      "same timestamp, different value",
+			timestamp: 2,
+			value:     2,
+			wantErr:   ErrDuplicateSampleForTimestamp,
+		},
+		{
+			name:      "earlier timestamp, same value",
+			timestamp: 1,
+			value:     2,
+			wantErr:   ErrOutOfOrderSample,
+		},
+		{
+			name:      "earlier timestamp, different value",
+			timestamp: 1,
+			value:     3,
+			wantErr:   ErrOutOfOrderSample,
+		},
+		{
+			name:      "regular append of NaN",
+			timestamp: 3,
+			value:     model.SampleValue(math.NaN()),
+			wantErr:   nil,
+		},
+		{
+			name:      "no-op append of NaN",
+			timestamp: 3,
+			value:     model.SampleValue(math.NaN()),
+			wantErr:   nil,
+		},
+		{
+			name:      "append of NaN with earlier timestamp",
+			timestamp: 2,
+			value:     model.SampleValue(math.NaN()),
+			wantErr:   ErrOutOfOrderSample,
+		},
+		{
+			name:      "append of normal sample after NaN with same timestamp",
+			timestamp: 3,
+			value:     3.14,
+			wantErr:   ErrDuplicateSampleForTimestamp,
+		},
+	}
+
+	for _, test := range tests {
+		gotErr := s.Append(&model.Sample{
 			Metric:    m,
-			Timestamp: model.Time(t),
-			Value:     model.SampleValue(i),
+			Timestamp: test.timestamp,
+			Value:     test.value,
 		})
+		if gotErr != test.wantErr {
+			t.Errorf("%s: got %q, want %q", test.name, gotErr, test.wantErr)
+		}
 	}
 
 	fp := s.mapper.mapFP(m.FastFingerprint(), m)
@@ -1792,9 +1863,18 @@ func TestAppendOutOfOrder(t *testing.T) {
 			Timestamp: 2,
 			Value:     1,
 		},
+		{
+			Timestamp: 3,
+			Value:     model.SampleValue(math.NaN()),
+		},
 	}
-	got := it.RangeValues(metric.Interval{OldestInclusive: 0, NewestInclusive: 2})
-	if !reflect.DeepEqual(want, got) {
-		t.Fatalf("want %v, got %v", want, got)
+	got := it.RangeValues(metric.Interval{OldestInclusive: 0, NewestInclusive: 3})
+	// Note that we cannot just reflect.DeepEqual(want, got) because it has
+	// the semantics of NaN != NaN.
+	for i, gotSamplePair := range got {
+		wantSamplePair := want[i]
+		if !wantSamplePair.Equal(&gotSamplePair) {
+			t.Fatalf("want %v, got %v", wantSamplePair, gotSamplePair)
+		}
 	}
 }

vendor/github.com/prometheus/common/expfmt/fuzz.go

@@ -20,8 +20,8 @@ import "bytes"
 
 // Fuzz text metric parser with with github.com/dvyukov/go-fuzz:
 //
-//     go-fuzz-build github.com/prometheus/client_golang/text
-//     go-fuzz -bin text-fuzz.zip -workdir fuzz
+//     go-fuzz-build github.com/prometheus/common/expfmt
+//     go-fuzz -bin expfmt-fuzz.zip -workdir fuzz
 //
 // Further input samples should go in the folder fuzz/corpus.
 func Fuzz(in []byte) int {

vendor/github.com/prometheus/common/model/value.go

@@ -16,6 +16,7 @@ package model
 import (
 	"encoding/json"
 	"fmt"
+	"math"
 	"sort"
 	"strconv"
 	"strings"
@@ -43,8 +44,14 @@ func (v *SampleValue) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
+// Equal returns true if the value of v and o is equal or if both are NaN. Note
+// that v==o is false if both are NaN. If you want the conventional float
+// behavior, use == to compare two SampleValues.
 func (v SampleValue) Equal(o SampleValue) bool {
-	return v == o
+	if v == o {
+		return true
+	}
+	return math.IsNaN(float64(v)) && math.IsNaN(float64(o))
 }
 
 func (v SampleValue) String() string {
@@ -77,9 +84,9 @@ func (s *SamplePair) UnmarshalJSON(b []byte) error {
 }
 
 // Equal returns true if this SamplePair and o have equal Values and equal
-// Timestamps.
+// Timestamps. The sematics of Value equality is defined by SampleValue.Equal.
 func (s *SamplePair) Equal(o *SamplePair) bool {
-	return s == o || (s.Value == o.Value && s.Timestamp.Equal(o.Timestamp))
+	return s == o || (s.Value.Equal(o.Value) && s.Timestamp.Equal(o.Timestamp))
 }
 
 func (s SamplePair) String() string {
@@ -93,7 +100,8 @@ type Sample struct {
 	Timestamp Time        `json:"timestamp"`
 }
 
-// Equal compares first the metrics, then the timestamp, then the value.
+// Equal compares first the metrics, then the timestamp, then the value. The
+// sematics of value equality is defined by SampleValue.Equal.
 func (s *Sample) Equal(o *Sample) bool {
 	if s == o {
 		return true
@@ -105,7 +113,7 @@ func (s *Sample) Equal(o *Sample) bool {
 	if !s.Timestamp.Equal(o.Timestamp) {
 		return false
 	}
-	if s.Value != o.Value {
+	if s.Value.Equal(o.Value) {
 		return false
 	}
 

vendor/vendor.json

@@ -161,35 +161,40 @@
 			"revisionTime": "2015-02-12T10:17:44Z"
 		},
 		{
+			"checksumSHA1": "l0JT1CAPc/22KdnxCke1LKtR+4M=",
 			"path": "github.com/prometheus/common/expfmt",
-			"revision": "23070236b1ebff452f494ae831569545c2b61d26",
-			"revisionTime": "2016-02-11T16:03:55+01:00"
+			"revision": "c16e34897a744c32f6733ee720e60c4de13887fb",
+			"revisionTime": "2016-05-19T16:20:33Z"
 		},
 		{
+			"checksumSHA1": "GWlM3d2vPYyNATtTFgftS10/A9w=",
 			"path": "github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg",
-			"revision": "4fdc91a58c9d3696b982e8a680f4997403132d44",
-			"revisionTime": "2015-10-26T12:04:34+01:00"
+			"revision": "c16e34897a744c32f6733ee720e60c4de13887fb",
+			"revisionTime": "2016-05-19T16:20:33Z"
 		},
 		{
+			"checksumSHA1": "koBNYQryxAG8hyHBlpn8pcnSVdM=",
 			"path": "github.com/prometheus/common/log",
-			"revision": "0a3005bb37bc411040083a55372e77c405f6464c",
-			"revisionTime": "2016-01-04T14:47:38+01:00"
+			"revision": "c16e34897a744c32f6733ee720e60c4de13887fb",
+			"revisionTime": "2016-05-19T16:20:33Z"
 		},
 		{
+			"checksumSHA1": "Zgmg/aOfoCNTAMtrXqBJmt852t0=",
 			"path": "github.com/prometheus/common/model",
-			"revision": "167b27da48d058a9b46d84b834d67f68f0243f67",
-			"revisionTime": "2016-03-18T12:23:18Z"
+			"revision": "c16e34897a744c32f6733ee720e60c4de13887fb",
+			"revisionTime": "2016-05-19T16:20:33Z"
 		},
 		{
+			"checksumSHA1": "CKVJRc1NREmfoAWQLHxqWQlvxo0=",
 			"path": "github.com/prometheus/common/route",
-			"revision": "14ca1097bbe21584194c15e391a9dab95ad42a59",
-			"revisionTime": "2016-01-25T23:57:51+01:00"
+			"revision": "c16e34897a744c32f6733ee720e60c4de13887fb",
+			"revisionTime": "2016-05-19T16:20:33Z"
 		},
 		{
 			"checksumSHA1": "91KYK0SpvkaMJJA2+BcxbVnyRO0=",
 			"path": "github.com/prometheus/common/version",
-			"revision": "dd586c1c5abb0be59e60f942c22af711a2008cb4",
-			"revisionTime": "2016-05-03T22:05:32Z"
+			"revision": "c16e34897a744c32f6733ee720e60c4de13887fb",
+			"revisionTime": "2016-05-19T16:20:33Z"
 		},
 		{
 			"path": "github.com/prometheus/procfs",