internal/upload: only download the upload config if necessary
Refactor the upload pass so that the upload config is downloaded lazily,
and only if an upload is actually going to be performed. More
refactoring is desirable, but this change is intentionally minimal as it
will be cherry-picked into Go 1.23.1 to address golang/go#68946. TODOs
are left for future CLs.
For golang/go#68946
Change-Id: I4c565b333d92361a8380dd790b1a4f2d7223a37c
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/607796
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
diff --git a/internal/config/config.go b/internal/config/config.go
index 7d73dcd..0ab6aab 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -16,6 +16,9 @@
// Config is a wrapper around telemetry.UploadConfig that provides some
// convenience methods for checking the contents of a report.
+//
+// TODO(rfindley): remove the embedded UploadConfig and just call this
+// 'ConfigInfo'. There's no need for this to be a wrapper.
type Config struct {
*telemetry.UploadConfig
program map[string]bool
diff --git a/internal/configstore/download.go b/internal/configstore/download.go
index a38f371..e60ab7e 100644
--- a/internal/configstore/download.go
+++ b/internal/configstore/download.go
@@ -16,6 +16,7 @@
"os"
"os/exec"
"path/filepath"
+ "sync/atomic"
"golang.org/x/telemetry/internal/telemetry"
)
@@ -29,12 +30,22 @@
// creation flag.
var needNoConsole = func(cmd *exec.Cmd) {}
+var downloads int64
+
+// Downloads reports, for testing purposes, the number of times [Download] has
+// been called.
+func Downloads() int64 {
+ return atomic.LoadInt64(&downloads)
+}
+
// Download fetches the requested telemetry UploadConfig using "go mod
// download". If envOverlay is provided, it is appended to the environment used
// for invoking the go command.
//
// The second result is the canonical version of the requested configuration.
func Download(version string, envOverlay []string) (*telemetry.UploadConfig, string, error) {
+ atomic.AddInt64(&downloads, 1)
+
if version == "" {
version = "latest"
}
diff --git a/internal/upload/reports.go b/internal/upload/reports.go
index 8b17e32..5c89e29 100644
--- a/internal/upload/reports.go
+++ b/internal/upload/reports.go
@@ -120,34 +120,20 @@
// returns the upload report file's path.
// It may delete the count files once local and upload report
// files are successfully created.
+//
+// TODO(rfindley): refactor this function to simplify control flow,
+// particularly the resolution of 'uploadOK', and to separate the intermingling
+// of local and upload reports. The latter can be produced as a post-processing
+// pass.
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" {
- u.logger.Printf("No upload config or mode %q is not 'on'", mode)
- uploadOK = false // no config, nothing to upload
- }
- if u.tooOld(expiryDate, u.startTime) {
- u.logger.Printf("Expiry date %s is too old", expiryDate)
- uploadOK = false
- }
- // 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) {
- u.logger.Printf("As-of date %s is not before start %s", asof, start)
- uploadOK = false
- }
// TODO(rfindley): check that all the x.Meta are consistent for GOOS, GOARCH, etc.
report := &telemetry.Report{
- Config: u.configVersion,
+ // Note: Config is unset for local reports, to avoid the config download if
+ // uploading is not enabled.
X: computeRandom(), // json encodes all the bits
Week: expiryDate,
LastWeek: lastWeek,
}
- if report.X > u.config.SampleRate && u.config.SampleRate > 0 {
- u.logger.Printf("X: %f > SampleRate:%f, not uploadable", report.X, u.config.SampleRate)
- uploadOK = false
- }
var succeeded bool
for _, f := range countFiles {
fok := false
@@ -187,15 +173,39 @@
return "", fmt.Errorf("failed to unmarshal local report for %s: %v", expiryDate, err)
}
+ // 2. Create the uploadable version, if applicable.
+ uploadOK := true
var uploadContents []byte
+ mode, asof := u.dir.Mode() // TODO(rfindley): read the mode once, at the start of the upload pass.
+ if mode != "on" {
+ u.logger.Printf("Mode %q is not 'on'", mode)
+ uploadOK = false
+ }
+ if u.tooOld(expiryDate, u.startTime) {
+ u.logger.Printf("Expiry date %s is too old for upload", expiryDate)
+ uploadOK = false
+ }
+ // 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) {
+ u.logger.Printf("As-of date %s is not before start %s", asof, start)
+ uploadOK = false
+ }
if uploadOK {
- // 2. create the uploadable version
- cfg := config.NewConfig(u.config)
+ uploadConfig, configVersion, err := u.getUploadConfig()
+ if err != nil {
+ return "", fmt.Errorf("config download failed: %v", err)
+ }
+ if report.X > uploadConfig.SampleRate && uploadConfig.SampleRate > 0 {
+ u.logger.Printf("X: %f > SampleRate:%f, not uploadable", report.X, uploadConfig.SampleRate)
+ uploadOK = false
+ }
+ cfg := config.NewConfig(uploadConfig)
upload := &telemetry.Report{
Week: report.Week,
LastWeek: report.LastWeek,
X: report.X,
- Config: report.Config,
+ Config: configVersion,
}
for _, p := range report.Programs {
// does the uploadConfig want this program?
@@ -251,6 +261,9 @@
// write the uploadable file
var errUpload, errLocal error
if uploadOK {
+ // Note: moving this write to the 'uploadOK' block above would subtly
+ // change control flow in the presence of racing uploads, because of the
+ // stats above. (see TODO above regarding refactoring)
_, errUpload = exclusiveWrite(uploadFileName, uploadContents)
}
// write the local file
diff --git a/internal/upload/run.go b/internal/upload/run.go
index eba13b1..e772b56 100644
--- a/internal/upload/run.go
+++ b/internal/upload/run.go
@@ -13,6 +13,7 @@
"path/filepath"
"runtime/debug"
"strings"
+ "sync"
"time"
"golang.org/x/telemetry/internal/configstore"
@@ -48,10 +49,7 @@
// uploader encapsulates a single upload operation, carrying parameters and
// shared state.
type uploader struct {
- // config is used to select counters to upload.
- config *telemetry.UploadConfig //
- configVersion string // version of the config
- dir telemetry.Dir // the telemetry dir to process
+ dir telemetry.Dir // the telemetry dir to process
uploadServerURL string
startTime time.Time
@@ -60,6 +58,22 @@
logFile *os.File
logger *log.Logger
+
+ // golang/go#68946: the telemetry upload configuration is lazily downloaded
+ // in order to avoid polluting the module cache with the telemetry config
+ // module when no such config is necessary.
+ //
+ // config, configVersion, and configErr store the result of
+ // [configstore.Download], guarded by configOnce. Access the downloaded
+ // result with [uploader.getUploadConfig].
+ //
+ // (as of writing, the config download need not be concurrency safe, but the
+ // use of a sync.Once simplifies control flow).
+ configEnv []string
+ configOnce sync.Once
+ config *telemetry.UploadConfig
+ configVersion string
+ configErr error
}
// newUploader creates a new uploader to use for running the upload for the
@@ -111,12 +125,6 @@
}
logger := log.New(logWriter, "", log.Ltime|log.Lmicroseconds|log.Lshortfile)
- // Fetch the upload config, if it is not provided.
- config, configVersion, err := configstore.Download("latest", rcfg.Env)
- if err != nil {
- return nil, err
- }
-
// Set the start time, if it is not provided.
startTime := time.Now().UTC()
if !rcfg.StartTime.IsZero() {
@@ -124,8 +132,8 @@
}
return &uploader{
- config: config,
- configVersion: configVersion,
+ configEnv: rcfg.Env,
+
dir: dir,
uploadServerURL: uploadURL,
startTime: startTime,
@@ -135,6 +143,16 @@
}, nil
}
+// getUploadConfig returns the upload configuration and config version to use
+// for this upload pass. These values are evaluated lazily, calling
+// [configstore.Download] the first time getUploadConfig is called.
+func (u *uploader) getUploadConfig() (*telemetry.UploadConfig, string, error) {
+ u.configOnce.Do(func() {
+ u.config, u.configVersion, u.configErr = configstore.Download("latest", u.configEnv)
+ })
+ return u.config, u.configVersion, u.configErr
+}
+
// Close cleans up any resources associated with the uploader.
func (u *uploader) Close() error {
if u.logFile == nil {
diff --git a/internal/upload/run_test.go b/internal/upload/run_test.go
index c2cb08e..4231985 100644
--- a/internal/upload/run_test.go
+++ b/internal/upload/run_test.go
@@ -20,6 +20,7 @@
"time"
"golang.org/x/telemetry/counter"
+ "golang.org/x/telemetry/internal/configstore"
"golang.org/x/telemetry/internal/configtest"
"golang.org/x/telemetry/internal/regtest"
"golang.org/x/telemetry/internal/telemetry"
@@ -430,12 +431,13 @@
prog := regtest.NewIncProgram(t, "prog1", "counter")
tests := []struct {
- mode string
- wantUploads int
+ mode string
+ wantConfigDownloads int64
+ wantUploads int
}{
- {"off", 0},
- {"local", 0},
- {"on", 1}, // only the second week is uploadable
+ {"off", 0, 0},
+ {"local", 0, 0},
+ {"on", 1, 1}, // only the second week is uploadable
}
for _, test := range tests {
t.Run(test.mode, func(t *testing.T) {
@@ -459,10 +461,15 @@
t.Fatal(err)
}
+ downloadsBefore := configstore.Downloads()
if err := upload.Run(cfg); err != nil {
t.Fatal(err)
}
+ if got := configstore.Downloads() - downloadsBefore; got != test.wantConfigDownloads {
+ t.Errorf("configstore.Download called: %v, want %v", got, test.wantConfigDownloads)
+ }
+
uploads := getUploads()
if gotUploads := len(uploads); gotUploads != test.wantUploads {
t.Fatalf("got %d uploads, want %d", gotUploads, test.wantUploads)
