-
Notifications
You must be signed in to change notification settings - Fork 18k
x/telemetry: consolidate initialization into telemetry.Start #65500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
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: |
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 <[email protected]> Reviewed-by: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
Change https://go.dev/cl/564597 mentions this issue: |
Change https://go.dev/cl/567075 mentions this issue: |
This follows the implementation of the daemonize function in x/tools/gopls/internal/lsprpc. On Windows, we set the DETACHED_PROCESS creation flag. For golang/go#65500 Change-Id: Ib7f052e88999444c4166bc7711346d26801b8f0f Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/564597 Reviewed-by: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
This change will create a token file to acquire a token to limit uploading to at most once a day: before upload is run, we'll check to see if an upload token file already exists. If it doesn't, or it's been a day since the existing file has been created, it's been a day since the last upload.Run was attempted so we can run it again. If it doesn't, we'll skip running upload.Run. We don't delete the token files when we're done with the upload, but instead only delete them when we want to acquire a new lock so that the existence of the file and its modification time is a signal of the last time uploading was run. The token acquiring code is based on acquireLockFile in x/tools/gopls/internal/server/prompt.go. For golang/go#65500 Change-Id: Ie31c1f351e2d31a016fd2ad79b29784e6631c564 Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/567075 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Robert Findley <[email protected]>
@matloob I think this is done, right? |
Yes. |
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 somethingx/telemetry
should manage. cmd/go should be able to start telemetry without worrying about overhead.Here's an outline of how this could work:
telemetry.Start
function to encapsulate the telemetry entrypoint:(some phrasing borrowed from https://go.dev/cl/559503, which added the
crashmonitor.Start
function thattelemetry.Start
would supersede).Start
would be in charge of callingcounter.Open
,crashmonitor.Start
, andupload.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 limitupload.Run
to once a day. We do something like this for the gopls telemetry prompt.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 asgo version
. This may or may not be necessary.CC @matloob @adonovan @hyangah @pjweinb
The text was updated successfully, but these errors were encountered: