From 74e3e9fbf3418c47cba7b80f76bbae19b5b32494 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 15 May 2024 16:50:22 +0000 Subject: [PATCH] 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 Reviewed-by: Hyang-Ah Hana Kim --- cmd/gotelemetry/main.go | 2 +- internal/upload/date.go | 6 +-- internal/upload/dates_test.go | 4 +- internal/upload/findwork.go | 2 +- internal/upload/reports.go | 6 +-- internal/upload/run.go | 31 +++++++++++---- internal/upload/run_test.go | 75 +++++++++++++++-------------------- internal/upload/upload.go | 6 +-- start.go | 2 +- upload/upload.go | 33 --------------- 10 files changed, 70 insertions(+), 97 deletions(-) delete mode 100644 upload/upload.go diff --git a/cmd/gotelemetry/main.go b/cmd/gotelemetry/main.go index 7f0e4bc..5e2d723 100644 --- 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 index 26e65bd..22e4e8a 100644 --- 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 index cd41c35..513843e 100644 --- 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 index 6bd559a..f1490be 100644 --- 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 index a335132..4175789 100644 --- 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 index 714617a..eba13b1 100644 --- 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,7 +136,7 @@ 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 } @@ -129,7 +144,7 @@ func (u *Uploader) Close() error { } // 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 index 84ab209..5898ce7 100644 --- 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,16 +249,12 @@ 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) } @@ -271,8 +262,8 @@ func TestUploader_Retries(t *testing.T) { 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,7 +449,7 @@ 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. @@ -466,7 +457,7 @@ func TestUploader_ModeHandling(t *testing.T) { 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) } diff --git a/internal/upload/upload.go b/internal/upload/upload.go index bec2230..0e75d09 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -22,7 +22,7 @@ var ( // uploadReportDate returns the date component of the upload file name, or "" if the // date was unmatched. -func (u *Uploader) uploadReportDate(fname string) time.Time { +func (u *uploader) uploadReportDate(fname string) time.Time { match := dateRE.FindStringSubmatch(fname) if match == nil || len(match) < 2 { u.logger.Printf("malformed report name: missing date: %q", filepath.Base(fname)) @@ -36,7 +36,7 @@ func (u *Uploader) uploadReportDate(fname string) time.Time { return d } -func (u *Uploader) uploadReport(fname string) { +func (u *uploader) uploadReport(fname string) { thisInstant := u.startTime // TODO(rfindley): use uploadReportDate here, once we've done a gopls release. @@ -60,7 +60,7 @@ func (u *Uploader) uploadReport(fname string) { } // try to upload the report, 'true' if successful -func (u *Uploader) uploadReportContents(fname string, buf []byte) bool { +func (u *uploader) uploadReportContents(fname string, buf []byte) bool { b := bytes.NewReader(buf) fdate := strings.TrimSuffix(filepath.Base(fname), ".json") fdate = fdate[len(fdate)-len("2006-01-02"):] diff --git a/start.go b/start.go index 2b6b15b..414c9fc 100644 --- a/start.go +++ b/start.go @@ -17,7 +17,7 @@ import ( "golang.org/x/telemetry/counter" "golang.org/x/telemetry/internal/crashmonitor" "golang.org/x/telemetry/internal/telemetry" - "golang.org/x/telemetry/upload" + "golang.org/x/telemetry/internal/upload" ) // Config controls the behavior of [Start]. diff --git a/upload/upload.go b/upload/upload.go deleted file mode 100644 index 0e2fb45..0000000 --- a/upload/upload.go +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2023 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package upload - -import ( - "log" - - "golang.org/x/telemetry/internal/upload" -) - -// TODO(rfindley): remove, in favor of all callers using Start. - -// A RunConfig controls the behavior of Run. -// The zero value RunConfig is the default behavior; fields may be set to -// override various reporting and uploading choices. -type RunConfig = upload.RunConfig - -// 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 := upload.NewUploader(config) - if err != nil { - return err - } - defer uploader.Close() - return uploader.Run() -}