all: remove 7 day offset for new uploads

With opt-in telemetry we don't need to insert an arbitrary delay before
the first upload. Remove special handling for this delay, as well as the
mention of it in the prompt.

Updates golang/go#63218

Change-Id: I7bd1477bfa3d6639dbc84464235be75b6cde974f
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/532175
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>

Author: findleyr

cmd/gotelemetry/main.go

@@ -14,7 +14,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"time"
 
 	"golang.org/x/telemetry/cmd/gotelemetry/internal/csv"
 	"golang.org/x/telemetry/cmd/gotelemetry/internal/view"
@@ -189,13 +188,12 @@ func runOn(_ []string) {
 }
 
 func telemetryOnMessage() string {
-	reportDate := time.Now().AddDate(0, 0, 7).Format("2006-01-02")
-	return fmt.Sprintf(`Telemetry uploading is now enabled and may be sent to https://telemetry.go.dev/ starting %s. Uploaded data is used to help improve the Go toolchain and related tools, and it will be published as part of a public dataset.
+	return fmt.Sprintf(`Telemetry uploading is now enabled and data will be periodically sent to https://telemetry.go.dev/. Uploaded data is used to help improve the Go toolchain and related tools, and it will be published as part of a public dataset.
 
 For more details, see https://telemetry.go.dev/privacy.
 This data is collected in accordance with the Google Privacy Policy (https://policies.google.com/privacy).
 
-To disable telemetry uploading, run “gotelemetry off”`, reportDate)
+To disable telemetry uploading, run “gotelemetry off”.`)
 }
 
 func runOff(_ []string) {

internal/counter/counter_test.go

@@ -294,7 +294,7 @@ func TestNewFile(t *testing.T) {
 			t.Fatal(err)
 		}
 		days := (timeEnd.Sub(testStartTime)) / (24 * time.Hour)
-		if days <= 7 || days > 14 {
+		if days <= 0 || days > 7 {
 			timeBegin, _ := time.Parse(time.RFC3339, cf.Meta["TimeBegin"])
 			t.Logf("testStartTime: %v file: %v TimeBegin: %v TimeEnd: %v", testStartTime, fi[0].Name(), timeBegin, timeEnd)
 			t.Errorf("%d: days = %d, want 7 < days <= 14", i, days)
@@ -306,27 +306,14 @@ func TestNewFile(t *testing.T) {
 	}
 }
 
-func TestWeekendsNewUser(t *testing.T) {
+func TestWeekends(t *testing.T) {
 	testenv.SkipIfUnsupportedPlatform(t)
-	commonWeekends(t, false)
-}
-
-func TestWeekendsOldUser(t *testing.T) {
-	testenv.SkipIfUnsupportedPlatform(t)
-	commonWeekends(t, true)
-}
 
-func commonWeekends(t *testing.T, old bool) {
-	t.Helper()
 	setup(t)
 	// get all the 49 combinations of today and when the week ends
 	for i := 0; i < 7; i++ {
 		counterTime = future(i)
 		for index := range "0123456" {
-			if old {
-				// make it look like a report has been generated
-				os.WriteFile(filepath.Join(telemetry.LocalDir, "2000-00-00.json"), []byte("{}\n"), 0666)
-			}
 			os.WriteFile(filepath.Join(telemetry.LocalDir, "weekends"), []byte{byte(index + '0')}, 0666)
 			var f file
 			c := f.New("gophers")
@@ -361,9 +348,6 @@ func commonWeekends(t *testing.T, old bool) {
 			// if we're an old user, we should have a <=7 day report
 			// if we're a new user, we should have a <=7+7 day report
 			more := 0
-			if !old {
-				more = 7
-			}
 			if delta <= 0+more || delta > 7+more {
 				t.Errorf("delta %d, expected %d<delta<=%d",
 					delta, more, more+7)

internal/counter/file.go

@@ -227,20 +227,7 @@ func fileValidity(now time.Time) (int, error) {
 	if incr <= 0 {
 		incr += 7
 	}
-	// as long as there are no reports this is a new user
-	fi, err := os.ReadDir(telemetry.LocalDir)
-	if err != nil {
-		return 0, err
-	}
-	// TODO(rfindley): once we can rely on telemetry users setting a mode date,
-	// we don't need to implement this heuristic about the existing of json
-	// files.
-	for _, f := range fi {
-		if strings.HasSuffix(f.Name(), ".json") {
-			return int(incr), nil
-		}
-	}
-	return int(incr) + 7, nil
+	return int(incr), nil
 }
 
 // rotate checks to see whether the file f needs to be rotated,

internal/upload/date.go

@@ -35,40 +35,37 @@ func tooOld(date string) bool {
 	return age > distantPast
 }
 
-// return the expiry date of a countfile in YYYY-MM-DD format
-func expiryDate(fname string) string {
-	t := expiry(fname)
-	if t.IsZero() {
-		return ""
-	}
-	// PJW: is this sometimes off by a day?
-	year, month, day := t.Date()
-	return fmt.Sprintf("%04d-%02d-%02d", year, month, day)
-}
-
 // a time in the far future for the expiry time with errors
 var farFuture = time.UnixMilli(1 << 62)
 
-// expiry returns the expiry time of a countfile. For errors
-// it returns a time far in the future, so that erroneous files
-// don't look like they should be used.
-func expiry(fname string) time.Time {
+// counterDateSpan parses the counter file named fname and returns the (begin, end) span
+// recorded in its metadata.
+// On any error, it returns (0, farFuture), so that invalid files don't look
+// like they can be used.
+//
+// TODO(rfindley): just return an error to make this explicit.
+func counterDateSpan(fname string) (begin, end time.Time) {
 	parsed, err := parse(fname)
 	if err != nil {
 		logger.Printf("expiry Parse: %v for %s", err, fname)
-		return farFuture // don't process it, whatever it is
+		return time.Time{}, farFuture
+	}
+	begin, err = time.Parse(time.RFC3339, parsed.Meta["TimeBegin"])
+	if err != nil {
+		logger.Printf("time.Parse(%s[TimeBegin]) failed: %v", fname, err)
+		return time.Time{}, farFuture
 	}
-	expiry, err := time.Parse(time.RFC3339, parsed.Meta["TimeEnd"])
+	end, err = time.Parse(time.RFC3339, parsed.Meta["TimeEnd"])
 	if err != nil {
-		logger.Printf("time.Parse: %v for %s", err, fname)
-		return farFuture // don't process it, whatever it is
+		logger.Printf("time.Parse(%s[TimeEnd]) failed: %v", fname, err)
+		return time.Time{}, farFuture
 	}
-	return expiry
+	return begin, end
 }
 
 // stillOpen returns true if the counter file might still be active
 func stillOpen(fname string) bool {
-	expiry := expiry(fname)
+	_, expiry := counterDateSpan(fname)
 	return expiry.After(thisInstant)
 }
 

internal/upload/dates_test.go

@@ -48,36 +48,34 @@ func TestDates(t *testing.T) {
 	const today = "2020-01-24"
 	const yesterday = "2020-01-23"
 	tests := []Test{ // each date must be different to subvert the parse cache
-		{ // test that existing counters and ready files are not uploaded if telemetry was recently enabled.
-			name:      "beforefirstupload",
-			today:     "2019-12-04",
-			date:      "2019-12-03",
-			begins:    "2019-12-01",
-			ends:      "2019-12-03",
-			readys:    []string{"2019-12-02"},
+		{ // test that existing counters and ready files are not uploaded if they span data before telemetry was enabled
+			name:   "beforefirstupload",
+			today:  "2019-12-04",
+			date:   "2019-12-03",
+			begins: "2019-12-01",
+			ends:   "2019-12-03",
+			readys: []string{"2019-12-01", "2019-12-02"},
+			// We get one local report: the newly created report.
+			// It is not ready as it begins on the same day that telemetry was
+			// enabled, and we err on the side of assuming it holds data from before
+			// the user turned on uploading.
 			wantLocal: 1,
+			// The report for 2019-12-01 is still ready, because it was not uploaded.
+			// This could happen in practice if the user disabled and then reenabled
+			// telmetry.
 			wantReady: 1,
+			// The report for 2019-12-02 was uploaded.
+			wantUploadeds: 1,
 		},
-		{ // test that existing counters and ready files are not uploaded if telemetry was recently enabled.
+		{ // test that existing counters and ready files are uploaded they only contain data after telemetry was enabled
 			name:          "oktoupload",
 			today:         "2019-12-10",
 			date:          "2019-12-09",
 			begins:        "2019-12-02",
 			ends:          "2019-12-09",
 			readys:        []string{"2019-12-07"},
 			wantLocal:     1,
-			wantReady:     1, // ready report is before the first upload time
-			wantUploadeds: 1, // ...but the new report is uploaded
-		},
-		{ // test that existing counters and ready files are not uploaded if telemetry was recently enabled.
-			name:          "uploadnewreport",
-			today:         "2019-12-14",
-			date:          "2019-12-12",
-			begins:        "2019-12-04",
-			ends:          "2019-12-12",
-			readys:        []string{"2019-12-13"},
-			wantLocal:     1,
-			wantUploadeds: 2, // ready report was uploaded
+			wantUploadeds: 2, // Both new report and existing report are uploaded.
 		},
 		{ // test that an old countfile is removed and no reports generated
 			name:   "oldcountfile",

internal/upload/findwork.go

@@ -54,7 +54,14 @@ func findWork(localdir, uploaddir string) work {
 				// right thing...
 				//
 				// (see https://github.com/golang/go/issues/63142#issuecomment-1734025130)
-				if asof.AddDate(0, 0, 7).Before(reportDate) {
+				if asof.Before(reportDate) {
+					// Note: since this report was created after telemetry was enabled,
+					// we can only assume that the process that created it checked that
+					// the counter data contained therein was all from after the asof
+					// date.
+					//
+					// TODO(rfindley): store the begin date in reports, so that we can
+					// verify this assumption.
 					ans.readyfiles = append(ans.readyfiles, filepath.Join(localdir, fi.Name()))
 				}
 			} else {

internal/upload/reports.go

@@ -33,21 +33,27 @@ func reports(todo *work) ([]string, error) {
 	if lastWeek >= today { //should never happen
 		lastWeek = ""
 	}
-	countFiles := make(map[string][]string) // date->filename
+	countFiles := make(map[string][]string) // expiry date string->filenames
+	earliest := make(map[string]time.Time)  // earliest begin time for any counter
 	for _, f := range todo.countfiles {
-		exp := expiryDate(f)
-		if exp < today {
-			countFiles[exp] = append(countFiles[exp], f)
+		begin, end := counterDateSpan(f)
+
+		if end.Before(thisInstant) {
+			expiry := end.Format(dateFormat)
+			countFiles[expiry] = append(countFiles[expiry], f)
+			if earliest[expiry].IsZero() || earliest[expiry].After(begin) {
+				earliest[expiry] = begin
+			}
 		}
 	}
-	for k, v := range countFiles {
-		if notNeeded(k, *todo) {
+	for expiry, files := range countFiles {
+		if notNeeded(expiry, *todo) {
 			// The report already exists.
 			// There's another check in createReport.
-			deleteFiles(v)
+			deleteFiles(files)
 			continue
 		}
-		fname, err := createReport(k, v, lastWeek)
+		fname, err := createReport(earliest[expiry], expiry, files, lastWeek)
 		if err != nil {
 			return nil, err
 		}
@@ -101,7 +107,7 @@ func deleteFiles(files []string) {
 
 // createReports for all the count files for the same date.
 // returns the absolute path name of the file containing the report
-func createReport(date string, files []string, lastWeek string) (string, error) {
+func createReport(start time.Time, expiryDate string, files []string, lastWeek string) (string, error) {
 	if uploadConfig == nil {
 		a, v, err := configstore.Download("latest", nil)
 		if err != nil {
@@ -115,27 +121,19 @@ func createReport(date string, files []string, lastWeek string) (string, error)
 	if uploadConfig == nil || mode != "on" {
 		uploadOK = false // no config, nothing to upload
 	}
-	if tooOld(date) {
+	if tooOld(expiryDate) {
 		uploadOK = false
 	}
-	if uploadOK && !asof.IsZero() {
-		// If the mode is recorded with an asof date, don't upload if the asof date
-		// is too recent.
+	// If the mode is recorded with an asof date, don't upload if the report
+	// includes any data on or before the asof date.
+	if !asof.IsZero() && !asof.Before(start) {
 		uploadOK = false
-		t, err := time.Parse("2006-01-02", date)
-		if err == nil {
-			if asof.AddDate(0, 0, 7).Before(t) {
-				uploadOK = true
-			}
-		} else {
-			logger.Printf("parsing report date: %v", err)
-		}
 	}
 	// should we check that all the x.Meta are consistent for GOOS, GOARCH, etc?
 	report := &telemetry.Report{
 		Config:   configVersion,
 		X:        computeRandom(), // json encodes all the bits
-		Week:     date,
+		Week:     expiryDate,
 		LastWeek: lastWeek,
 	}
 	// X is not being used as a probability to decide which counters to upload,
@@ -217,8 +215,8 @@ func createReport(date string, files []string, lastWeek string) (string, error)
 	if err != nil {
 		return "", fmt.Errorf("failed to marshal upload report (%v)", err)
 	}
-	localFileName := filepath.Join(it.LocalDir, "local."+date+".json")
-	uploadFileName := filepath.Join(it.LocalDir, date+".json")
+	localFileName := filepath.Join(it.LocalDir, "local."+expiryDate+".json")
+	uploadFileName := filepath.Join(it.LocalDir, expiryDate+".json")
 	// if either file exists, someone has been here ahead of us
 	// (there is still a race, but this check shortens the open window)
 	if _, err := os.Stat(localFileName); err == nil {