Simplify how OutOfOrderTimeWindow works (#285)

* Simplify how OutOfOrderTimeWindow works

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

* Update test

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

cmd/prometheus/main.go

@@ -314,12 +314,6 @@ func main() {
 	serverOnlyFlag(a, "storage.tsdb.wal-compression", "Compress the tsdb WAL.").
 		Hidden().Default("true").BoolVar(&cfg.tsdb.WALCompression)
 
-	serverOnlyFlag(a, "storage.tsdb.out-of-order-cap-min", "Minimum capacity for out of order chunks (in samples. between 0 and 255.)").
-		Hidden().Default("4").IntVar(&cfg.tsdb.OutOfOrderCapMin)
-
-	serverOnlyFlag(a, "storage.tsdb.out-of-order-cap-max", "Maximum capacity for out of order chunks (in samples. between 1 and 255.)").
-		Hidden().Default("32").IntVar(&cfg.tsdb.OutOfOrderCapMax)
-
 	serverOnlyFlag(a, "storage.tsdb.head-chunks-write-queue-size", "Size of the queue through which head chunks are written to the disk to be m-mapped, 0 disables the queue completely. Experimental.").
 		Default("0").IntVar(&cfg.tsdb.HeadChunksWriteQueueSize)
 
@@ -1536,8 +1530,6 @@ type tsdbOptions struct {
 	MinBlockDuration               model.Duration
 	MaxBlockDuration               model.Duration
 	OutOfOrderTimeWindow           int64
-	OutOfOrderCapMin               int
-	OutOfOrderCapMax               int
 	EnableExemplarStorage          bool
 	MaxExemplars                   int64
 	EnableMemorySnapshotOnShutdown bool
@@ -1561,8 +1553,6 @@ func (opts tsdbOptions) ToTSDBOptions() tsdb.Options {
 		MaxExemplars:                   opts.MaxExemplars,
 		EnableMemorySnapshotOnShutdown: opts.EnableMemorySnapshotOnShutdown,
 		OutOfOrderTimeWindow:           opts.OutOfOrderTimeWindow,
-		OutOfOrderCapMin:               int64(opts.OutOfOrderCapMin),
-		OutOfOrderCapMax:               int64(opts.OutOfOrderCapMax),
 	}
 }
 

tsdb/db.go

@@ -671,6 +671,9 @@ func validateOpts(opts *Options, rngs []int64) (*Options, []int64) {
 	if opts.OutOfOrderCapMax <= 0 {
 		opts.OutOfOrderCapMax = DefaultOutOfOrderCapMax
 	}
+	if opts.OutOfOrderCapMin > opts.OutOfOrderCapMax {
+		opts.OutOfOrderCapMax = opts.OutOfOrderCapMin
+	}
 	if opts.OutOfOrderTimeWindow < 0 {
 		opts.OutOfOrderTimeWindow = 0
 	}

tsdb/db_test.go

@@ -4261,8 +4261,18 @@ func TestOOOAppendAndQuery(t *testing.T) {
 
 	// At the edge of time window, also it would be "out of bound" without the ooo support.
 	addSample(s1, 60, 65, false)
-	addSample(s2, 50, 55, false)
+	verifyOOOMinMaxTimes(60, 265)
-	verifyOOOMinMaxTimes(50, 265)
+	testQuery()
+
+	// This sample is not within the time window w.r.t. the head's maxt, but it is within the window
+	// w.r.t. the series' maxt. But we consider only head's maxt.
+	addSample(s2, 59, 59, true)
+	verifyOOOMinMaxTimes(60, 265)
+	testQuery()
+
+	// Now the sample is within time window w.r.t. the head's maxt.
+	addSample(s2, 60, 65, false)
+	verifyOOOMinMaxTimes(60, 265)
 	testQuery()
 
 	// Out of time window again.
@@ -4276,7 +4286,7 @@ func TestOOOAppendAndQuery(t *testing.T) {
 	require.Equal(t, float64(4), prom_testutil.ToFloat64(db.head.metrics.chunksCreated))
 	addSample(s1, 180, 249, false)
 	require.Equal(t, float64(6), prom_testutil.ToFloat64(db.head.metrics.chunksCreated))
-	verifyOOOMinMaxTimes(50, 265)
+	verifyOOOMinMaxTimes(60, 265)
 	testQuery()
 }
 

tsdb/head_append.go

@@ -326,50 +326,43 @@ func (a *headAppender) Append(ref storage.SeriesRef, lset labels.Labels, t int64
 // The sample belongs to the out of order chunk if we return true and no error.
 // An error signifies the sample cannot be handled.
 func (s *memSeries) appendable(t int64, v float64, headMaxt, minValidTime, oooTimeWindow int64) (isOutOfOrder bool, delta int64, err error) {
-	msMaxt := s.maxTime()
+	// Check if we can append in the in-order chunk.
-	if msMaxt == math.MinInt64 {
-		// The series has no sample and was freshly created.
 	if t >= minValidTime {
-			// We can append it in the in-order chunk.
+		if s.head() == nil {
+			// The series has no sample and was freshly created.
 			return false, 0, nil
 		}
-
+		msMaxt := s.maxTime()
-		// We cannot append it in the in-order head. So we check the oooTimeWindow
-		// w.r.t. the head's maxt.
-		// -1 because for the first sample in the Head, headMaxt will be equal to t.
-		msMaxt = headMaxt - 1
-	}
-
 		if t > msMaxt {
 			return false, 0, nil
 		}
-
+		if t == msMaxt {
-	if t < msMaxt-oooTimeWindow {
-		if oooTimeWindow > 0 {
-			return true, msMaxt - t, storage.ErrTooOldSample
-		}
-		if t < minValidTime {
-			return false, msMaxt - t, storage.ErrOutOfBounds
-		}
-		return false, msMaxt - t, storage.ErrOutOfOrderSample
-	}
-
-	if t != msMaxt || s.head() == nil {
-		// Sample is ooo and within time window OR series has no active chunk to check for duplicate sample.
-		return true, msMaxt - t, nil
-	}
-
 			// We are allowing exact duplicates as we can encounter them in valid cases
 			// like federation and erroring out at that time would be extremely noisy.
-	// this only checks against the latest in-order sample.
+			// This only checks against the latest in-order sample.
-	// the OOO headchunk has its own method to detect these duplicates
+			// The OOO headchunk has its own method to detect these duplicates
 			if math.Float64bits(s.sampleBuf[3].v) != math.Float64bits(v) {
 				return false, 0, storage.ErrDuplicateSampleForTimestamp
 			}
-
+			// Sample is identical (ts + value) with most current (highest ts) sample in sampleBuf.
-	// sample is identical (ts + value) with most current (highest ts) sample in sampleBuf
 			return false, 0, nil
 		}
+	}
+
+	// The sample cannot go in the in-order chunk. Check if it can go in the out-of-order chunk.
+	if oooTimeWindow > 0 && t >= headMaxt-oooTimeWindow {
+		return true, headMaxt - t, nil
+	}
+
+	// The sample cannot go in both in-order and out-of-order chunk.
+	if oooTimeWindow > 0 {
+		return true, headMaxt - t, storage.ErrTooOldSample
+	}
+	if t < minValidTime {
+		return false, headMaxt - t, storage.ErrOutOfBounds
+	}
+	return false, headMaxt - t, storage.ErrOutOfOrderSample
+}
 
 // AppendExemplar for headAppender assumes the series ref already exists, and so it doesn't
 // use getOrCreate or make any of the lset sanity checks that Append does.