go icon indicating copy to clipboard operation
go copied to clipboard

x/telemetry: consolidate initialization into telemetry.Start

Open findleyr opened this issue 1 year ago • 2 comments

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.Start function 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, Start would be in charge of calling counter.Open, crashmonitor.Start, and upload.Run, per the provided configuration. By encapsulating, Start can 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.
  • Start should daemonize itself, so that the upload may outlast a short-running process. (we can borrow from the gopls daemon)
  • Start should implement rate limiting, to amortize its cost over many invocations. E.g. use lockfile acquisition to rate limit upload.Run to once a day. We do something like this for the gopls telemetry prompt.
  • If necessary, Start can wait a short amount of time in a goroutine (e.g. 1s) before doing anything expensive. This minimizes overhead extremely short-lived processes such as go version. This may or may not be necessary.

CC @matloob @adonovan @hyangah @pjweinb

findleyr avatar Feb 03 '24 16:02 findleyr

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.

adonovan avatar Feb 03 '24 18:02 adonovan

Change https://go.dev/cl/564615 mentions this issue: telemetry: add support for uploading

gopherbot avatar Feb 15 '24 22:02 gopherbot

Change https://go.dev/cl/564597 mentions this issue: telemetry: daemonize the child started by telemetry.Start

gopherbot avatar Feb 26 '24 19:02 gopherbot

Change https://go.dev/cl/567075 mentions this issue: telemetry: in Start, run upload.Run at most once a day

gopherbot avatar Feb 26 '24 19:02 gopherbot

@matloob I think this is done, right?

findleyr avatar May 20 '24 21:05 findleyr

Yes.

matloob avatar May 20 '24 21:05 matloob