Skip to content
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

proposal: cmd/go: make GOTELEMETRY settable like other env vars #69113

Closed
willfaught opened this issue Aug 28, 2024 · 7 comments
Closed

proposal: cmd/go: make GOTELEMETRY settable like other env vars #69113

willfaught opened this issue Aug 28, 2024 · 7 comments
Milestone

Comments

@willfaught
Copy link
Contributor

Proposal Details

go env, go env -w, go env -u, and go env -changed should work with GOTELEMETRY like other settable env vars.

GOTELEMETRY should be inherited from the process env, like other env vars.

To do this, GOTELEMETRY=on data should be put in a separate place than GOTELEMETRY=local data, so that processes with GOTELEMETRY=on in their env know that it's always safe to upload GOTELEMETRY=on data, without needing an extra timestamp stored somewhere to separate the two types of data.

Alternatively, if GOTELEMETRY=on and GOTELEMETRY=local data aren't separated, then a separate GOTELEMETRYSTART env var should be added to specify the cut-off timestamp that separates the two types of data. If GOTELEMETRY="on", but GOTELEMETRYSTART="", then telemetry is disabled. When go env -w GOTELEMETRY=on is run, then go env -w GOTELEMETRYSTART=$now is run under the hood for the user.

Filing per #68960 (comment).

@gopherbot gopherbot added this to the Proposal milestone Aug 28, 2024
@ianlancetaylor
Copy link
Contributor

CC @findleyr @matloob

@ianlancetaylor ianlancetaylor added the GoCommand cmd/go label Aug 28, 2024
@findleyr
Copy link
Contributor

Thanks for filing this proposal following our discussion in #68960.

Regarding the two implementation options in your proposal, I think we should focus on the first (separating "local" data vs "on" data). The fact that we keep track of the time that telemetry was enabled is really an artifact of the current implementation, where the telemetry mode is stored in a user-global configuration file that is independent of process environment. Preserving some notion of a start time in the per-process environment seems unnecessarily complex.

The current design went through a lengthy internal review, public discussion, and public proposal process (#58894), as did the go telemetry subcommand (#67111). Your proposal seems like a reasonable variation of that design, but what benefit does this change offer that would justify revising the current implementation?

To be clear, I don't think we'd have to revisit everything that has been discussed, but want to emphasize that the cost of implementing this proposal would not be technical: any change to the way telemetry data is stored or the way that telemetry is enabled would have to be scrutinized, especially if the change makes it even marginally easier to enable telemetry uploading, which I think this proposal does.

@willfaught
Copy link
Contributor Author

what benefit does this change offer that would justify revising the current implementation?

Improved understandability, consistency, simplicity, and security.

When working with the telemetry configuration for the first time, I was confused by how it worked. I wrote an email to golang-nuts that described that confusion:

go env doesn't seem to work well with GOTELEMETRY

Despite showing up an an env var, it's not treated consistently as one:

❯ go telemetry off
# nothing printed
# as expected

❯ go env | grep GOTELEMETRY=
GOTELEMETRY='off'
# as expected

❯ go env GOTELEMETRY
off
# as expected

❯ go env -changed
# nothing printed
# as expected

❯ go env -u GOTELEMETRY
go: unknown go command variable GOTELEMETRY
# expected nothing printed, success

❯ go env -w GOTELEMETRY=on
go: unknown go command variable GOTELEMETRY
# expected nothing printed, success

❯ go telemetry on
# snip
# as expected

❯ go env | grep GOTELEMETRY=
GOTELEMETRY='on'
# as expected

❯ go env GOTELEMETRY
on
# as expected

❯ go env -changed
# nothing printed
# expected to be printed: GOTELEMETRY='on'

❯ go env -u GOTELEMETRY
go: unknown go command variable GOTELEMETRY
# expected nothing printed, success

❯ go env -w GOTELEMETRY=off
go: unknown go command variable GOTELEMETRY
# expected nothing printed, success

According to the error messages, GOTELEMETRY wasn't even a known go command variable.

Users shouldn't have to explore with trial and error to discover for themselves how configuration works. GOTELEMETRY is listed as an environment variable, and the user is supposed to be able to set it to on/off/local as they please, yet you can't set it with a process environment variable or go env -w. This is confusing.

The Go 1 compatibility promise doesn't cover tooling, IIRC. Making GOTELEMETRY a normal env var wouldn't break compatibility anyway; all it would do is render the go telemetry subcommand redundant for setting GOTELEMETRY, and unfortunate inelegance isn't a priority.

especially if the change makes it even marginally easier to enable telemetry uploading

It seems like a security footgun that a user can ignorantly or accidentally do env GOTELEMETRY=off go build, and telemetry will still be enabled (because go telemetry on was done previously). What's the argument that the security risk of how it works now is less than the security risk of accidentally doing env GOTELEMETRY=on go build (where that enables telemetry where it would otherwise be disabled)? In the latter case, the user sees what is happening, it is completely their fault, no footgun is involved, and ignorance (in the sense of the former case) is impossible.

In #68960 (comment), I wrote:

People aren't going to accidentally set GOTELEMETRY=on (or at least it's no more likely than accidentally setting GODEBUG=execerrdot=0)

So I would ask for the argument for why the severity of accidentally GOTELEMETRY=on is greater than that of accidentally setting GODEBUG=execerrdot=0. I don't see how the severity is greater. If the severity is the same or less, then I don't see a problem.

@willfaught
Copy link
Contributor Author

Also, as I mentioned in #68960 (comment):

Having a cut-off timestamp means that GOTELEMETRY=on data is lost if you set GOTELEMETRY=on, then later GOTELEMETRY=local, then later GOTELEMETRY=on again. By placing the two data types separately, no data is lost when you switch modes.

@rsc
Copy link
Contributor

rsc commented Sep 4, 2024

The time to have this discussion was before Go 1.23. This ship has sailed, and it sailed to go telemetry island.

@rsc
Copy link
Contributor

rsc commented Sep 4, 2024

This proposal has been declined as infeasible.
— rsc for the proposal review group

@rsc rsc closed this as completed Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Declined
Development

No branches or pull requests

6 participants