[ENG-947] Window rounding, remove prom duration with offset logic (#2768)

* [ENG-947] Window rounding, remove prom duration with offset logic

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* pin protoc version

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* syntax fix

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* justfile fixes

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* fix test

Signed-off-by: Alex Meijer <ameijer@kubecost.com>
(cherry picked from commit b1496ae0f3a5b53fec773e7738f035e351b0210f)

* fix test

Signed-off-by: Alex Meijer <ameijer@kubecost.com>
(cherry picked from commit e2da4e455f49b2f4e67f53bd9dc202d10f73d8d4)

* addl test fixes

Signed-off-by: Alex Meijer <ameijer@kubecost.com>
(cherry picked from commit d77a239a8d606bfcfa45b949ffd067e58479be13)

* test fixes

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* addl test fixes

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* test fixes

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* test fixes

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

* update weekly logic

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

---------

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

.github/workflows/build-test.yaml

@@ -34,7 +34,7 @@ jobs:
       - name: install protobuf-go
         run: |
           go install github.com/golang/protobuf/protoc-gen-go@latest
-          go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest
+          go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.3.0
           which protoc-gen-go-grpc
       -
         name: Validate

core/pkg/opencost/window.go

@@ -298,13 +298,18 @@ func parseWindow(window string, now time.Time) (Window, error) {
 		end := now
 		start := end.Add(-time.Duration(num) * dur)
 
-		// when using windows such as "7d" and "1w", we have to have a definition for what "the past X days" means.
+		// when using windows such as "7d", "1w", and "2h" we have to have a definition for what "the past X days/hours" means.
 		// let "the past X days" be defined as the entirety of today plus the entirety of the past X-1 days, where
 		// "entirety" is defined as midnight to midnight, UTC. given this definition, we round forward the calculated
 		// start and end times to the nearest day to align with midnight boundaries
-		if match[2] == "d" || match[2] == "w" {
+		// an analogous definition applies to "the past X weeks" and "the past X hours"
+		if match[2] == "w" {
+			// special case - with a week, we say the week ends today
 			end = end.Truncate(timeutil.Day).Add(timeutil.Day)
 			start = start.Truncate(timeutil.Day).Add(timeutil.Day)
+		} else {
+			end = now.Truncate(dur).Add(dur)
+			start = end.Add(-time.Duration(num) * dur)
 		}
 
 		return NewWindow(&start, &end), nil
@@ -743,53 +748,6 @@ func (w Window) DurationOffset() (time.Duration, time.Duration, error) {
 	return duration, offset, nil
 }
 
-// DurationOffsetForPrometheus returns strings representing durations for the
-// duration and offset of the given window, factoring in the Thanos offset if
-// necessary. Whereas duration is a simple duration string (e.g. "1d"), the
-// offset includes the word "offset" (e.g. " offset 2d") so that the values
-// returned can be used directly in the formatting string "some_metric[%s]%s"
-// to generate the query "some_metric[1d] offset 2d".
-func (w Window) DurationOffsetForPrometheus() (string, string, error) {
-	duration, offset, err := w.DurationOffset()
-	if err != nil {
-		return "", "", err
-	}
-
-	// If using Thanos, increase offset to 3 hours, reducing the duration by
-	// equal measure to maintain the same starting point.
-	// TODO: This logic should technically be decoupled from this type, but
-	// TODO: current use cases are unclear. To ensure we do not break existing
-	// TODO: (or legacy) use-cases, temporarily support this one-off logic.
-	thanosDur := thanosOffset()
-	if offset < thanosDur && isThanosEnabled() {
-		diff := thanosDur - offset
-		offset += diff
-		duration -= diff
-	}
-
-	// If duration < 0, return an error
-	if duration < 0 {
-		return "", "", fmt.Errorf("negative duration: %s", duration)
-	}
-
-	// Negative offset means that the end time is in the future. Prometheus
-	// fails for non-positive offset values, so shrink the duration and
-	// remove the offset altogether.
-	if offset < 0 {
-		duration = duration + offset
-		offset = 0
-	}
-
-	durStr, offStr := timeutil.DurationOffsetStrings(duration, offset)
-	if offset < time.Minute {
-		offStr = ""
-	} else {
-		offStr = " offset " + offStr
-	}
-
-	return durStr, offStr, nil
-}
-
 // DurationOffsetStrings returns formatted, Prometheus-compatible strings representing
 // the duration and offset of the window in terms of days, hours, minutes, or seconds;
 // e.g. ("7d", "1441m", "30m", "1s", "")

core/pkg/opencost/window_test.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"strings"
 	"testing"
 	"time"
 
@@ -653,7 +652,39 @@ func TestWindow_DurationOffsetStrings(t *testing.T) {
 	}
 }
 
-func TestWindow_DurationOffsetForPrometheus(t *testing.T) {
+func TestParse_Window(t *testing.T) {
+	now := time.Date(2024, time.May, 3, 8, 1, 4, 6, time.UTC)
+	win, err := parseWindow("2h", now)
+	if err != nil {
+		t.Fatalf(`unexpected error parsing "2h": %s`, err)
+	}
+
+	expectedStart := time.Date(2024, time.May, 3, 7, 0, 0, 0, time.UTC)
+	expectedEnd := time.Date(2024, time.May, 3, 9, 0, 0, 0, time.UTC)
+
+	if !win.start.Equal(expectedStart) {
+		t.Fatalf(`expect: window start to be %s; actual: %s`, expectedStart, win.start)
+	}
+	if !win.end.Equal(expectedEnd) {
+		t.Fatalf(`expect: window end to be %s; actual: %s`, expectedEnd, win.end)
+	}
+
+	win, err = parseWindow("3d", now)
+	if err != nil {
+		t.Fatalf(`unexpected error parsing "3d": %s`, err)
+	}
+
+	expectedStart = time.Date(2024, time.May, 1, 0, 0, 0, 0, time.UTC)
+	expectedEnd = time.Date(2024, time.May, 4, 0, 0, 0, 0, time.UTC)
+
+	if !win.start.Equal(expectedStart) {
+		t.Fatalf(`expect: window start to be %s; actual: %s`, expectedStart, win.start)
+	}
+	if !win.end.Equal(expectedEnd) {
+		t.Fatalf(`expect: window end to be %s; actual: %s`, expectedEnd, win.end)
+	}
+}
+func TestWindow_Duration(t *testing.T) {
 	// Set-up and tear-down
 	thanosEnabled := env.GetBool(ThanosEnabledEnvVarName, false)
 	defer env.SetBool(ThanosEnabledEnvVarName, thanosEnabled)
@@ -665,144 +696,47 @@ func TestWindow_DurationOffsetForPrometheus(t *testing.T) {
 	}
 
 	now := time.Now().UTC()
-	startOfToday := now.Truncate(timeutil.Day)
 	w, err := parseWindow("1d", now)
 	if err != nil {
 		t.Fatalf(`unexpected error parsing "1d": %s`, err)
 	}
-
-	dur, off, err := w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	// We can get a response in seconds OR minutes. Check seconds first as it
-	// is higher resolution.
-	expDurSec := int(now.Sub(startOfToday).Seconds())
-	expDurSecStr := fmt.Sprintf("%ds", expDurSec)
-	expDurMin := int(now.Sub(startOfToday).Minutes())
-	expDurMinStr := fmt.Sprintf("%dm", expDurMin)
-	if dur != expDurSecStr && dur != expDurMinStr {
-		t.Fatalf(`expect: window to be "%s" (or "%s"); actual: "%s"`, expDurSecStr, expDurMinStr, dur)
-	}
-	if off != "" {
-		t.Fatalf(`expect: offset to be ""; actual: "%s"`, off)
+	if w.Duration() != 24*time.Hour {
+		t.Fatalf(`expect: window to be 24 hours; actual: %s`, w.Duration())
 	}
 
 	w, err = ParseWindowUTC("2h")
 	if err != nil {
 		t.Fatalf(`unexpected error parsing "2h": %s`, err)
 	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	if dur != "2h" {
-		t.Fatalf(`expect: window to be "2h"; actual: "%s"`, dur)
-	}
-	if off != "" {
-		t.Fatalf(`expect: offset to be ""; actual: "%s"`, off)
-	}
 
+	if w.Duration().String() != "2h0m0s" {
+		t.Fatalf(`expect: window to be "2h"; actual: "%s"`, w.Duration().String())
+	}
 	w, err = ParseWindowUTC("10m")
 	if err != nil {
 		t.Fatalf(`unexpected error parsing "10m": %s`, err)
 	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	if dur != "10m" {
-		t.Fatalf(`expect: window to be "10m"; actual: "%s"`, dur)
-	}
-	if off != "" {
-		t.Fatalf(`expect: offset to be ""; actual: "%s"`, off)
+
+	if w.Duration().String() != "10m0s" {
+		t.Fatalf(`expect: window to be "10m"; actual: "%s"`, w.Duration().String())
 	}
 
 	w, err = ParseWindowUTC("1589448338,1589534798")
 	if err != nil {
 		t.Fatalf(`unexpected error parsing "1589448338,1589534798": %s`, err)
 	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	if dur != "1441m" {
-		t.Fatalf(`expect: window to be "1441m"; actual: "%s"`, dur)
-	}
-	if !strings.HasPrefix(off, " offset ") {
-		t.Fatalf(`expect: offset to start with " offset "; actual: "%s"`, off)
+	if w.Duration().String() != "24h1m0s" {
+		t.Fatalf(`expect: window to be "24h1m0s"; actual: "%s"`, w.Duration().String())
 	}
 
 	w, err = ParseWindowUTC("yesterday")
 	if err != nil {
 		t.Fatalf(`unexpected error parsing "yesterday": %s`, err)
 	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	if dur != "1d" {
-		t.Fatalf(`expect: window to be "1d"; actual: "%s"`, dur)
-	}
-	if !strings.HasPrefix(off, " offset ") {
-		t.Fatalf(`expect: offset to start with " offset "; actual: "%s"`, off)
-	}
-
-	// Test for Thanos (env.IsThanosEnabled() == true)
-	env.SetBool(ThanosEnabledEnvVarName, true)
-	if !env.GetBool(ThanosEnabledEnvVarName, false) {
-		t.Fatalf("expected env.IsThanosEnabled() == true")
-	}
-
-	// Note - with the updated logic of 1d, 1w, etc. rounding the start and end times forward to the nearest midnight,
-	// DurationOffsetForPrometheus may fail if not using a window using "Xh" as the string to parse
-	w, err = ParseWindowUTC("24h")
-	if err != nil {
-		t.Fatalf(`unexpected error parsing "24h": %s`, err)
-	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	if dur != "21h" {
-		t.Fatalf(`expect: window to be "21d"; actual: "%s"`, dur)
-	}
-	if off != " offset 3h" {
-		t.Fatalf(`expect: offset to be " offset 3h"; actual: "%s"`, off)
+	if w.Duration().String() != "24h0m0s" {
+		t.Fatalf(`expect: window to be "24h0m0s"; actual: "%s"`, w.Duration().String())
 	}
 
-	w, err = ParseWindowUTC("2h")
-	if err != nil {
-		t.Fatalf(`unexpected error parsing "2h": %s`, err)
-	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err == nil {
-		t.Fatalf(`expected error (negative duration); got ("%s", "%s")`, dur, off)
-	}
-
-	w, err = ParseWindowUTC("10m")
-	if err != nil {
-		t.Fatalf(`unexpected error parsing "1d": %s`, err)
-	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err == nil {
-		t.Fatalf(`expected error (negative duration); got ("%s", "%s")`, dur, off)
-	}
-
-	w, err = ParseWindowUTC("1589448338,1589534798")
-	if err != nil {
-		t.Fatalf(`unexpected error parsing "1589448338,1589534798": %s`, err)
-	}
-	dur, off, err = w.DurationOffsetForPrometheus()
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-	if dur != "1441m" {
-		t.Fatalf(`expect: window to be "1441m"; actual: "%s"`, dur)
-	}
-	if !strings.HasPrefix(off, " offset ") {
-		t.Fatalf(`expect: offset to start with " offset "; actual: "%s"`, off)
-	}
 }
 
 // TODO

core/pkg/util/timeutil/timeutil_test.go

@@ -1,7 +1,6 @@
 package timeutil
 
 import (
-	"fmt"
 	"testing"
 	"time"
 )
@@ -393,27 +392,27 @@ func Test_FormatDurationStringDaysToHours(t *testing.T) {
 func TestRoundToStartOfWeek(t *testing.T) {
 	sunday := time.Date(2023, 03, 26, 12, 12, 12, 12, time.UTC)
 	roundedFromSunday := RoundToStartOfWeek(sunday)
-	if roundedFromSunday.Day() != 26 || roundedFromSunday.Weekday() == time.Sunday {
-		fmt.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromSunday.Day(), roundedFromSunday.Weekday().String())
+	if roundedFromSunday.Day() != 26 || roundedFromSunday.Weekday() != time.Sunday {
+		t.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromSunday.Day(), roundedFromSunday.Weekday().String())
 	}
 
 	tuesday := time.Date(2023, 03, 28, 12, 12, 12, 12, time.UTC)
 	roundedFromTuesday := RoundToStartOfWeek(tuesday)
-	if roundedFromTuesday.Day() != 26 || roundedFromTuesday.Weekday() == time.Sunday {
-		fmt.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromTuesday.Day(), roundedFromTuesday.Weekday().String())
+	if roundedFromTuesday.Day() != 26 || roundedFromTuesday.Weekday() != time.Sunday {
+		t.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromTuesday.Day(), roundedFromTuesday.Weekday().String())
 	}
 }
 
 func TestRoundToStartOfFollowingWeek(t *testing.T) {
 	sunday := time.Date(2023, 03, 26, 12, 12, 12, 12, time.UTC)
 	roundedFromSunday := RoundToStartOfFollowingWeek(sunday)
-	if roundedFromSunday.Month() != 4 || roundedFromSunday.Day() != 2 || roundedFromSunday.Weekday() == time.Sunday {
-		fmt.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromSunday.Day(), roundedFromSunday.Weekday().String())
+	if roundedFromSunday.Month() != 4 || roundedFromSunday.Day() != 2 || roundedFromSunday.Weekday() != time.Sunday {
+		t.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromSunday.Day(), roundedFromSunday.Weekday().String())
 	}
 
 	tuesday := time.Date(2023, 03, 28, 12, 12, 12, 12, time.UTC)
 	roundedFromTuesday := RoundToStartOfFollowingWeek(tuesday)
-	if roundedFromTuesday.Month() != 4 || roundedFromTuesday.Day() != 2 || roundedFromTuesday.Weekday() == time.Sunday {
-		fmt.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromTuesday.Day(), roundedFromTuesday.Weekday().String())
+	if roundedFromTuesday.Month() != 4 || roundedFromTuesday.Day() != 2 || roundedFromTuesday.Weekday() != time.Sunday {
+		t.Errorf("expected date to be rounded to the same sunday, got: %d, %s", roundedFromTuesday.Day(), roundedFromTuesday.Weekday().String())
 	}
 }

justfile

@@ -6,8 +6,14 @@ commit := `git rev-parse --short HEAD`
 default:
     just --list
 
+# run core unit tests
+test-core: 
+    {{commonenv}} cd ./core && go test ./... -coverprofile=coverage.out
+    {{commonenv}} cd ./core && go vet ./...
+
+
 # Run unit tests
-test:
+test: test-core
     {{commonenv}} go test ./... -coverprofile=coverage.out
     {{commonenv}} go vet ./...