go
go copied to clipboard
x/telemetry: consolidate initialization into telemetry.Start
This issue summarizes a team discussion about integrating telemetry with cmd/go, or generally any tool that may be short-lived. Currently the only tool with telemetry integration is gopls, which is a long-running server and therefore doesn't need to worry about rate limiting or overhead when calling upload.Run once when it starts serving.
The plan was for cmd/go to occasionally start a subprocess to run upload.Run, but as we discussed the details of this, it became clear that this is something x/telemetry should manage. cmd/go should be able to start telemetry without worrying about overhead.
Here's an outline of how this could work:
- Add a new
telemetry.Startfunction to encapsulate the telemetry entrypoint:
package telemetry
// Config controls the behavior of [Start].
type Config struct {
Upload bool
WatchCrashes bool
Logger io.Writer
}
// Start initializes telemetry, and may start a subprocess to monitor crashes
// and process expired counter files.
//
// Specifically, start opens a counter file if local collection is enabled,
// and starts a subprocess if [Config.WatchCrashes] is set or if the local
// telemetry directory is due for processing. 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.
//
// Start may re-execute the current executable as a child process, in a special
// mode. In that mode, the call to Start will never return. Therefore, Start should
// be called near the top of the main function of the application, and the
// application should avoid doing expensive work in init functions as they will
// be executed twice.
func Start(cfg Config)
(some phrasing borrowed from https://go.dev/cl/559503, which added the crashmonitor.Start function that telemetry.Start would supersede).
- As documented,
Startwould be in charge of callingcounter.Open,crashmonitor.Start, andupload.Run, per the provided configuration. By encapsulating,Startcan guarantee that the order of these calls is appropriate, and can share the subprocess for crash monitoring and uploading. Without encapsulation, we'd have to start two subprocesses: one for crash monitoring and one for uploading, and they may interact poorly. Startshould daemonize itself, so that the upload may outlast a short-running process. (we can borrow from the gopls daemon)Startshould implement rate limiting, to amortize its cost over many invocations. E.g. use lockfile acquisition to rate limitupload.Runto once a day. We do something like this for the gopls telemetry prompt.- If necessary,
Startcan wait a short amount of time in a goroutine (e.g. 1s) before doing anything expensive. This minimizes overhead extremely short-lived processes such asgo version. This may or may not be necessary.
CC @matloob @adonovan @hyangah @pjweinb
Sounds good. The current crashmonitor never lives much longer than the parent (application) process because the termination of the parent--for any reason--closes the pipe, causing the ReadAll in the child to unblock and the crashmonitor to finish up quickly thereafter and exit. If you want to consolidate the two subprocesses, you'll need to change the crashmonitor logic to return (not Fatal) and decrement a semaphore; the upload logic would also decrement a semaphore when it is finished; and the Start function (in the child process) would need to wait for both events before exiting.
Change https://go.dev/cl/564615 mentions this issue: telemetry: add support for uploading
Change https://go.dev/cl/564597 mentions this issue: telemetry: daemonize the child started by telemetry.Start
Change https://go.dev/cl/567075 mentions this issue: telemetry: in Start, run upload.Run at most once a day
@matloob I think this is done, right?
Yes.