upload: eliminate the x/telemetry/upload package

Now that all callers use telemetry.Start, the upload package can be removed. Move Run to internal/upload, and rewrite upload_test to call Run rather than NewUploader. At this point, the Uploader type (which encapsulates the upload operation) can be made an internal detail. Change-Id: Ib648017773909849a0534159a548f8c928db2268 Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585477 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
golang · May 15, 2024 · 74e3e9f · 74e3e9f
1 parent f89f2a3
commit 74e3e9f
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 97 deletions.
diff --git a/cmd/gotelemetry/main.go b/cmd/gotelemetry/main.go
@@ -19,7 +19,7 @@ import (
 	"golang.org/x/telemetry/cmd/gotelemetry/internal/view"
 	"golang.org/x/telemetry/internal/counter"
 	"golang.org/x/telemetry/internal/telemetry"
-	"golang.org/x/telemetry/upload"
+	"golang.org/x/telemetry/internal/upload"
 )
 
 type command struct {

diff --git a/internal/upload/date.go b/internal/upload/date.go
@@ -18,7 +18,7 @@ import (
 var distantPast = 21 * 24 * time.Hour
 
 // reports that are too old (21 days) are not uploaded
-func (u *Uploader) tooOld(date string, uploadStartTime time.Time) bool {
+func (u *uploader) tooOld(date string, uploadStartTime time.Time) bool {
 	t, err := time.Parse("2006-01-02", date)
 	if err != nil {
 		u.logger.Printf("tooOld: %v", err)
@@ -31,7 +31,7 @@ func (u *Uploader) tooOld(date string, uploadStartTime time.Time) bool {
 // counterDateSpan parses the counter file named fname and returns the (begin,
 // end) span recorded in its metadata, or an error if this data could not be
 // extracted.
-func (u *Uploader) counterDateSpan(fname string) (begin, end time.Time, _ error) {
+func (u *uploader) counterDateSpan(fname string) (begin, end time.Time, _ error) {
 	parsed, err := u.parseCountFile(fname)
 	if err != nil {
 		return time.Time{}, time.Time{}, err
@@ -61,7 +61,7 @@ type parsedCache struct {
 	m  map[string]*counter.File
 }
 
-func (u *Uploader) parseCountFile(fname string) (*counter.File, error) {
+func (u *uploader) parseCountFile(fname string) (*counter.File, error) {
 	u.cache.mu.Lock()
 	defer u.cache.mu.Unlock()
 	if u.cache.m == nil {

diff --git a/internal/upload/dates_test.go b/internal/upload/dates_test.go
@@ -204,7 +204,7 @@ func TestDates(t *testing.T) {
 			if err := os.MkdirAll(dbg, 0777); err != nil {
 				t.Fatal(err)
 			}
-			uploader, err := NewUploader(RunConfig{
+			uploader, err := newUploader(RunConfig{
 				TelemetryDir: telemetryDir,
 				UploadURL:    srv.URL,
 				Env:          env,
@@ -332,7 +332,7 @@ func readCountFileInfo(t *testing.T, localDir string) *countFileInfo {
 	return &ans
 }
 
-func doTest(t *testing.T, u *Uploader, test *Test, known *countFileInfo) int {
+func doTest(t *testing.T, u *uploader, test *Test, known *countFileInfo) int {
 	// set up directory contents
 	contents := bytes.Join([][]byte{
 		known.buf[:known.beginOffset],

diff --git a/internal/upload/findwork.go b/internal/upload/findwork.go
@@ -22,7 +22,7 @@ type work struct {
 // find all the files that look like counter files or reports
 // that need to be uploaded. (There may be unexpected leftover files
 // and uploading is supposed to be idempotent.)
-func (u *Uploader) findWork() work {
+func (u *uploader) findWork() work {
 	localdir, uploaddir := u.dir.LocalDir(), u.dir.UploadDir()
 	var ans work
 	fis, err := os.ReadDir(localdir)

diff --git a/internal/upload/reports.go b/internal/upload/reports.go
@@ -21,7 +21,7 @@ import (
 )
 
 // reports generates reports from inactive count files
-func (u *Uploader) reports(todo *work) ([]string, error) {
+func (u *uploader) reports(todo *work) ([]string, error) {
 	if mode, _ := u.dir.Mode(); mode == "off" {
 		return nil, nil // no reports
 	}
@@ -104,7 +104,7 @@ func notNeeded(date string, todo work) bool {
 	return false
 }
 
-func (u *Uploader) deleteFiles(files []string) {
+func (u *uploader) deleteFiles(files []string) {
 	for _, f := range files {
 		if err := os.Remove(f); err != nil {
 			// this could be a race condition.
@@ -117,7 +117,7 @@ func (u *Uploader) deleteFiles(files []string) {
 
 // createReport for all the count files for the same date.
 // returns the absolute path name of the file containing the report
-func (u *Uploader) createReport(start time.Time, expiryDate string, countFiles []string, lastWeek string) (string, error) {
+func (u *uploader) createReport(start time.Time, expiryDate string, countFiles []string, lastWeek string) (string, error) {
 	uploadOK := true
 	mode, asof := u.dir.Mode()
 	if mode != "on" {

diff --git a/internal/upload/run.go b/internal/upload/run.go
@@ -30,9 +30,24 @@ type RunConfig struct {
 	StartTime    time.Time // if set, overrides the upload start time
 }
 
-// Uploader encapsulates a single upload operation, carrying parameters and
+// Run generates and uploads reports, as allowed by the mode file.
+func Run(config RunConfig) error {
+	defer func() {
+		if err := recover(); err != nil {
+			log.Printf("upload recover: %v", err)
+		}
+	}()
+	uploader, err := newUploader(config)
+	if err != nil {
+		return err
+	}
+	defer uploader.Close()
+	return uploader.Run()
+}
+
+// uploader encapsulates a single upload operation, carrying parameters and
 // shared state.
-type Uploader struct {
+type uploader struct {
 	// config is used to select counters to upload.
 	config        *telemetry.UploadConfig //
 	configVersion string                  // version of the config
@@ -47,11 +62,11 @@ type Uploader struct {
 	logger  *log.Logger
 }
 
-// NewUploader creates a new uploader to use for running the upload for the
+// newUploader creates a new uploader to use for running the upload for the
 // given config.
 //
-// Uploaders should only be used for one call to [Run].
-func NewUploader(rcfg RunConfig) (*Uploader, error) {
+// Uploaders should only be used for one call to [uploader.Run].
+func newUploader(rcfg RunConfig) (*uploader, error) {
 	// Determine the upload directory.
 	var dir telemetry.Dir
 	if rcfg.TelemetryDir != "" {
@@ -108,7 +123,7 @@ func NewUploader(rcfg RunConfig) (*Uploader, error) {
 		startTime = rcfg.StartTime
 	}
 
-	return &Uploader{
+	return &uploader{
 		config:          config,
 		configVersion:   configVersion,
 		dir:             dir,
@@ -121,15 +136,15 @@ func NewUploader(rcfg RunConfig) (*Uploader, error) {
 }
 
 // Close cleans up any resources associated with the uploader.
-func (u *Uploader) Close() error {
+func (u *uploader) Close() error {
 	if u.logFile == nil {
 		return nil
 	}
 	return u.logFile.Close()
 }
 
 // Run generates and uploads reports
-func (u *Uploader) Run() error {
+func (u *uploader) Run() error {
 	if telemetry.DisabledOnPlatform {
 		return nil
 	}

diff --git a/internal/upload/run_test.go b/internal/upload/run_test.go
@@ -26,18 +26,18 @@ import (
 	"golang.org/x/telemetry/internal/upload"
 )
 
-// createUploader sets up an upload environment for the provided test, with a
+// runConfig sets up an upload environment for the provided test, with a
 // fake proxy allowing the given counters, and a fake upload server.
 //
-// The returned Uploader is ready to upload the given directory.
-// The second return is a function to fetch all uploaded reports.
+// The returned RunConfig is ready to pass to Run to upload the given
+// directory. The second return is a function to fetch all uploaded reports.
 //
-// For convenience, createUploader also sets the mode in telemetryDir to "on",
+// For convenience, runConfig also sets the mode in telemetryDir to "on",
 // back-dated to a time in the past. Callers that want to run the upload with a
 // different mode can reset as necessary.
 //
 // All associated resources are cleaned up with t.Clean.
-func createUploader(t *testing.T, telemetryDir string, counters, stackCounters []string) (*upload.Uploader, func() [][]byte) {
+func runConfig(t *testing.T, telemetryDir string, counters, stackCounters []string) (upload.RunConfig, func() [][]byte) {
 	t.Helper()
 
 	if err := telemetry.NewDir(telemetryDir).SetModeAsOf("on", time.Now().Add(-365*24*time.Hour)); err != nil {
@@ -48,17 +48,12 @@ func createUploader(t *testing.T, telemetryDir string, counters, stackCounters [
 	uc := upload.CreateTestUploadConfig(t, counters, stackCounters)
 	env := configtest.LocalProxyEnv(t, uc, "v1.2.3")
 
-	uploader, err := upload.NewUploader(upload.RunConfig{
+	return upload.RunConfig{
 		TelemetryDir: telemetryDir,
 		UploadURL:    srv.URL,
 		LogWriter:    testWriter{t},
 		Env:          env,
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	t.Cleanup(func() { uploader.Close() })
-	return uploader, uploaded
+	}, uploaded
 }
 
 // testWriter is an io.Writer wrapping t.Log.
@@ -71,7 +66,7 @@ func (w testWriter) Write(p []byte) (n int, err error) {
 	return len(p), nil
 }
 
-func TestUploader_Basic(t *testing.T) {
+func TestRun_Basic(t *testing.T) {
 	// Check the correctness of a single upload to the local server.
 
 	testenv.SkipIfUnsupportedPlatform(t)
@@ -96,14 +91,14 @@ func TestUploader_Basic(t *testing.T) {
 	// past where the "debug" directory could not be read.
 	// (there is no issue to reference for additional context, unfortunately)
 	logName := filepath.Join(telemetryDir, "debug")
-	err := os.WriteFile(logName, nil, 0666) // must be done before calling NewUploader
+	err := os.WriteFile(logName, nil, 0666) // must be done before calling Run
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	// Run the upload.
-	uploader, getUploads := createUploader(t, telemetryDir, []string{"knownCounter", "aStack"}, nil)
-	if err := uploader.Run(); err != nil {
+	cfg, getUploads := runConfig(t, telemetryDir, []string{"knownCounter", "aStack"}, nil)
+	if err := upload.Run(cfg); err != nil {
 		t.Fatal(err)
 	}
 
@@ -200,8 +195,8 @@ func checkTelemetryFiles(t *testing.T, telemetryDir string, want telemetryFiles)
 	}
 }
 
-func TestUploader_Retries(t *testing.T) {
-	// Check that the Uploader handles upload server status codes appropriately,
+func TestRun_Retries(t *testing.T) {
+	// Check that the Run handles upload server status codes appropriately,
 	// and that retries behave as expected.
 
 	testenv.SkipIfUnsupportedPlatform(t)
@@ -254,25 +249,21 @@ func TestUploader_Retries(t *testing.T) {
 			env := configtest.LocalProxyEnv(t, uc, "v1.2.3")
 
 			// Run the upload.
-			uploader, err := upload.NewUploader(upload.RunConfig{
+			badCfg := upload.RunConfig{
 				TelemetryDir: telemetryDir,
 				UploadURL:    srv.URL,
 				Env:          env,
-			})
-			if err != nil {
-				t.Fatal(err)
 			}
-			t.Cleanup(func() { uploader.Close() })
-			if err := uploader.Run(); err != nil {
+			if err := upload.Run(badCfg); err != nil {
 				t.Fatal(err)
 			}
 
 			// Check that the upload left the telemetry directory in the desired state.
 			checkTelemetryFiles(t, telemetryDir, test.initialFiles)
 
 			// Now re-run the upload with a succeeding upload server.
-			goodUploader, _ := createUploader(t, telemetryDir, []string{"counter"}, nil)
-			if err := goodUploader.Run(); err != nil {
+			goodCfg, _ := runConfig(t, telemetryDir, []string{"counter"}, nil)
+			if err := upload.Run(goodCfg); err != nil {
 				t.Fatal(err)
 			}
 
@@ -282,8 +273,8 @@ func TestUploader_Retries(t *testing.T) {
 	}
 }
 
-func TestUploader_MultipleUploads(t *testing.T) {
-	// This test checks that Uploader.Run produces multiple reports when counters
+func TestRun_MultipleUploads(t *testing.T) {
+	// This test checks that [upload.Run] produces multiple reports when counters
 	// span more than a week.
 
 	testenv.SkipIfUnsupportedPlatform(t)
@@ -302,8 +293,8 @@ func TestUploader_MultipleUploads(t *testing.T) {
 		t.Fatalf("failed to run program: %s", out)
 	}
 
-	uploader, getUploads := createUploader(t, telemetryDir, []string{"counter1", "counter2"}, nil)
-	if err := uploader.Run(); err != nil {
+	cfg, getUploads := runConfig(t, telemetryDir, []string{"counter1", "counter2"}, nil)
+	if err := upload.Run(cfg); err != nil {
 		t.Fatal(err)
 	}
 
@@ -319,7 +310,7 @@ func TestUploader_MultipleUploads(t *testing.T) {
 	}
 }
 
-func TestUploader_EmptyUpload(t *testing.T) {
+func TestRun_EmptyUpload(t *testing.T) {
 	// This test verifies that an empty counter file does not cause uploads of
 	// another week's reports to fail.
 
@@ -343,8 +334,8 @@ func TestUploader_EmptyUpload(t *testing.T) {
 		t.Fatalf("failed to run program: %s", out)
 	}
 
-	uploader, getUploads := createUploader(t, telemetryDir, []string{"week1", "week2"}, nil)
-	if err := uploader.Run(); err != nil {
+	cfg, getUploads := runConfig(t, telemetryDir, []string{"week1", "week2"}, nil)
+	if err := upload.Run(cfg); err != nil {
 		t.Fatal(err)
 	}
 
@@ -361,7 +352,7 @@ func TestUploader_EmptyUpload(t *testing.T) {
 	}
 }
 
-func TestUploader_MissingDate(t *testing.T) {
+func TestRun_MissingDate(t *testing.T) {
 	// This test verifies that a counter file with corrupt metadata does not
 	// prevent the uploader from uploading another week's reports.
 
@@ -412,8 +403,8 @@ func TestUploader_MissingDate(t *testing.T) {
 		t.Fatalf("failed to run program: %s", out)
 	}
 
-	uploader, getUploads := createUploader(t, telemetryDir, []string{"counter"}, nil)
-	if err := uploader.Run(); err != nil {
+	cfg, getUploads := runConfig(t, telemetryDir, []string{"counter"}, nil)
+	if err := upload.Run(cfg); err != nil {
 		t.Fatal(err)
 	}
 
@@ -428,7 +419,7 @@ func TestUploader_MissingDate(t *testing.T) {
 	}
 }
 
-func TestUploader_ModeHandling(t *testing.T) {
+func TestRun_ModeHandling(t *testing.T) {
 	// This test verifies that the uploader honors the telemetry mode, as well as
 	// its asof date.
 
@@ -458,15 +449,15 @@ func TestUploader_ModeHandling(t *testing.T) {
 				t.Fatalf("failed to run program: %s", out)
 			}
 
-			uploader, getUploads := createUploader(t, telemetryDir, []string{"counter"}, nil)
+			cfg, getUploads := runConfig(t, telemetryDir, []string{"counter"}, nil)
 
 			// Enable telemetry as of 10 days ago. This should prevent the first week
 			// from being uploaded, but not the second.
 			if err := telemetry.NewDir(telemetryDir).SetModeAsOf(test.mode, now.Add(-10*24*time.Hour)); err != nil {
 				t.Fatal(err)
 			}
 
-			if err := uploader.Run(); err != nil {
+			if err := upload.Run(cfg); err != nil {
 				t.Fatal(err)
 			}
 
@@ -477,7 +468,7 @@ func TestUploader_ModeHandling(t *testing.T) {
 		})
 	}
 }
-func TestUploader_DebugLog(t *testing.T) {
+func TestRun_DebugLog(t *testing.T) {
 	// This test verifies that the uploader honors the telemetry mode, as well as
 	// its asof date.
 
@@ -532,8 +523,8 @@ func TestUploader_DebugLog(t *testing.T) {
 				t.Fatalf("failed to run program: %s", out)
 			}
 
-			uploader, getUploads := createUploader(t, telemetryDir, []string{"counter"}, nil)
-			if err := uploader.Run(); err != nil {
+			cfg, getUploads := runConfig(t, telemetryDir, []string{"counter"}, nil)
+			if err := upload.Run(cfg); err != nil {
 				t.Fatal(err)
 			}