From b2ef4dc52d8166b4aea3348cc8fc5a7844a6e070 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 19 May 2016 18:32:47 +0200 Subject: [PATCH 1/3] Correctly identify no-op appends if the value is NaN This requires an updating of the vendored commen.model package, which I will do once https://github.com/prometheus/common/pull/40 is merged. --- storage/local/storage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/local/storage.go b/storage/local/storage.go index 20a73748b..85c50d089 100644 --- a/storage/local/storage.go +++ b/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() From 8032763db1ae6d5df8bc795f68b85d52f02552c2 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Fri, 20 May 2016 13:44:37 +0200 Subject: [PATCH 2/3] Update vendoring of github.com/prometheus/common/... --- .../prometheus/common/expfmt/fuzz.go | 4 +-- .../prometheus/common/model/value.go | 18 ++++++++---- vendor/vendor.json | 29 +++++++++++-------- 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/vendor/github.com/prometheus/common/expfmt/fuzz.go b/vendor/github.com/prometheus/common/expfmt/fuzz.go index 14f920146..dc2eedeef 100644 --- a/vendor/github.com/prometheus/common/expfmt/fuzz.go +++ b/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 { diff --git a/vendor/github.com/prometheus/common/model/value.go b/vendor/github.com/prometheus/common/model/value.go index 10ffb0bd6..dbf5d10e4 100644 --- a/vendor/github.com/prometheus/common/model/value.go +++ b/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 } diff --git a/vendor/vendor.json b/vendor/vendor.json index 286f74f22..94a84800f 100644 --- a/vendor/vendor.json +++ b/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", From a308c762920eb625d6886cbc6b801a6e11ac6905 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Fri, 20 May 2016 13:46:33 +0200 Subject: [PATCH 3/3] Improve TestAppendOutOfOrder It did not test the returned error so far. Also, add tests for the NaN case broken before https://github.com/prometheus/common/pull/40 --- storage/local/storage_test.go | 96 ++++++++++++++++++++++++++++++++--- 1 file changed, 88 insertions(+), 8 deletions(-) diff --git a/storage/local/storage_test.go b/storage/local/storage_test.go index c3c0948a2..476fe60c8 100644 --- a/storage/local/storage_test.go +++ b/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) + } } }