telemetry: add support for uploading No support for rate limiting yet but want to keep things small here. This change adds a dependency on x/sync for errgroup. I've also had to run go mod tidy in the godev module because it has a replacement instead of depending on a version of this module. For golang/go#65500 Change-Id: Icbcd8d3d58fb443c14cf21e339baa3edb76fbca1 Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/564615 Reviewed-by: Robert Findley <rfindley@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/cmd/gotelemetry/main.go b/cmd/gotelemetry/main.go index 58d5def..6538d97 100644 --- a/cmd/gotelemetry/main.go +++ b/cmd/gotelemetry/main.go
@@ -341,7 +341,7 @@ func runUpload(_ []string) { upload.Run(&upload.Control{ - Logging: os.Stderr, + Logger: os.Stderr, }) }
diff --git a/go.mod b/go.mod index 74d5da9..3b9d932 100644 --- a/go.mod +++ b/go.mod
@@ -4,4 +4,7 @@ require golang.org/x/mod v0.15.0 -require golang.org/x/sys v0.17.0 +require ( + golang.org/x/sync v0.6.0 + golang.org/x/sys v0.17.0 +)
diff --git a/go.sum b/go.sum index b89e429..e33729a 100644 --- a/go.sum +++ b/go.sum
@@ -1,4 +1,6 @@ golang.org/x/mod v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= +golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
diff --git a/godev/go.mod b/godev/go.mod index 5d55415..cd93047 100644 --- a/godev/go.mod +++ b/godev/go.mod
@@ -30,7 +30,7 @@ golang.org/x/crypto v0.14.0 // indirect golang.org/x/net v0.17.0 // indirect golang.org/x/oauth2 v0.13.0 // indirect - golang.org/x/sync v0.4.0 // indirect + golang.org/x/sync v0.6.0 // indirect golang.org/x/text v0.13.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/appengine v1.6.7 // indirect
diff --git a/godev/go.sum b/godev/go.sum index 246be43..0ad42e0 100644 --- a/godev/go.sum +++ b/godev/go.sum
@@ -106,8 +106,8 @@ golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ= -golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= +golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= +golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
diff --git a/start.go b/start.go index 0a05933..01c0dfd 100644 --- a/start.go +++ b/start.go
@@ -10,8 +10,10 @@ "os" "os/exec" + "golang.org/x/sync/errgroup" "golang.org/x/telemetry/counter" "golang.org/x/telemetry/internal/crashmonitor" + "golang.org/x/telemetry/upload" ) // Config controls the behavior of [Start]. @@ -47,11 +49,15 @@ // // If [Config.Upload] is set, and the user has opted in to telemetry // uploading, this process may attempt to upload approved counters -// to telemetry.go.dev. [But: this option doesn't appear to exist yet.] +// to telemetry.go.dev. // -// If [Config.ReportCrashes] is set, Start re-executes the current -// executable as a child process, in a special mode in which it acts -// as a crash monitor for the parent process (the application). +// If [Config.ReportCrashes] is set, any fatal crash will be +// recorded by incrementing a counter named for the stack of the +// first running goroutine in the traceback. +// +// If either of these flags is set, Start re-executes the current +// executable as a child process, in a special mode in which it +// acts as a telemetry sidecar for the parent process (the application). // In that mode, the call to Start will never return, so Start must // be called immediately within main, even before such things as // inspecting the command line. The application should avoid expensive @@ -59,23 +65,21 @@ // be executed twice (parent and child). func Start(config Config) { counter.Open() - if !config.ReportCrashes || !crashmonitor.Supported() { - // TODO(matloob): Once support for uploading is added to start, - // we'll just start the uploader instead of returning here. - return - } - if os.Getenv(telemetryChildVar) != "" { - child(config.Logger) - panic("unreachable") - } - parent(config.Logger) + if (config.ReportCrashes && crashmonitor.Supported()) || config.Upload { + if os.Getenv(telemetryChildVar) != "" { + child(config) + panic("unreachable") + } + + parent(config) + } } const telemetryChildVar = "X_TELEMETRY_CHILD" -func parent(logw io.Writer) { - logger := log.New(logw, "", 0) +func parent(config Config) { + logger := log.New(config.Logger, "", 0) // This process is the application (parent). // Fork+exec the telemetry child. exe, err := os.Executable() @@ -86,19 +90,44 @@ cmd.Env = append(os.Environ(), telemetryChildVar+"=1") cmd.Stderr = os.Stderr cmd.Stdout = os.Stderr - pipe, err := cmd.StdinPipe() - if err != nil { - logger.Fatalf("StdinPipe: %v", err) - } - crashmonitor.Parent(pipe.(*os.File)) // (this conversion is safe) + if config.ReportCrashes { + pipe, err := cmd.StdinPipe() + if err != nil { + logger.Fatalf("StdinPipe: %v", err) + } + + crashmonitor.Parent(pipe.(*os.File)) // (this conversion is safe) + } if err := cmd.Start(); err != nil { logger.Fatalf("can't start telemetry child process: %v", err) } } -func child(logw io.Writer) { - // TODO(matloob): add support for uploading. - crashmonitor.Child(logw) +func child(config Config) { + // Start crashmonitoring and uploading depending on what's requested + // and wait for the longer running child to complete before exiting: + // if we collected a crash before the upload finished, wait for the + // upload to finish before exiting + var g errgroup.Group + + if config.Upload { + g.Go(func() error { + uploaderChild(config.Logger) + return nil + }) + } + if config.ReportCrashes { + g.Go(func() error { + crashmonitor.Child(config.Logger) + return nil + }) + } + g.Wait() +} + +func uploaderChild(logger io.Writer) { + // TODO(matloob): Do rate-limiting here. + upload.Run(&upload.Control{Logger: logger}) }
diff --git a/upload/upload.go b/upload/upload.go index 0a2b13a..122b725 100644 --- a/upload/upload.go +++ b/upload/upload.go
@@ -14,8 +14,8 @@ // Run generates and uploads reports, as allowed by the mode file. // A nil Control is legal. func Run(c *Control) { - if c != nil && c.Logging != nil { - upload.SetLogOutput(c.Logging) + if c != nil && c.Logger != nil { + upload.SetLogOutput(c.Logger) } // ignore error: failed logging should not block uploads upload.LogIfDebug("") @@ -32,7 +32,7 @@ // reporting and uploading choices. // Future versions may also allow the user to set the upload URL. type Control struct { - // Logging provides a io.Writer for error messages during uploading + // Logger provides a io.Writer for error messages during uploading // nil is legal and no log messages get generated - Logging io.Writer + Logger io.Writer }