-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: consider making GOTELEMETRY=off settable #68960
Comments
Related Issues and Documentation
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.) |
I'm happy to work on this, since it would benefit the Please build system, as discussed in #68946. Ideally, where would you want it implemented? I can see at least two possibilities in |
If the bug #68946 is fixed, what is the remaining use case & motivation of |
Collecting telemetry data has no use if the intention is for it never to be uploaded. Most of the build rules in Please's go-rules plugin invoke |
If that bug is fixed by using I'd say that |
@peterebden I think if |
I think the decision in this issue comes down to the following question: is the convenience of being able to disable telemetry with an environment variable worth the potential confusion caused by (1) there being two ways to disable telemetry, and (2) the asymmetry of @peterebden or @chrisnovakovic, could you elaborate on why setting |
Mostly because it's easier for us to control the ephemeral build sandbox via its environment than via the commands that run inside it. We'd have to make sure that any build action that runs |
My use-case (running an integration test on GitHub Actions) similarly is much easier to configure via a GOTELEMETRY=off environment variable (which is inherited by processes) than to arrange for commands to be run in the various sub-processes that are spawned as part of the test. |
Just a note that if we permit |
The fix we hope to implement is "not to download" at all unless the telemetry is explicitly on. So, no more unnecessary downloads of the telemetry module.
And a note: Making |
(@ianlancetaylor suggested I repost here. Happy to move this to a new issue if needed.) go env doesn't seem to work well with GOTELEMETRY:
It seems incorrect for It seems to me that |
It's only sticky within one current 'user session'. We have ephemeral working directories for each invocation, The same is presumably likely to be true for any other usage of remote build execution (REAPI is a fairly widely known example), where presumably any sensible executor implementation would at least attempt to avoid allowing actions to write global state.
Well not quite, setting We've mentioned a bit before, but I also think it's relevant how easy it is to make this happen:
How do I do the same for One might also consider it bad form for certain tools to muck with a user's global config state, if they only want to make sure that their invocations of go don't have telemetry enabled. |
@willfaught I don't think we can eliminate |
I think |
@findleyr It could also be printed when GOTELEMETRY=on is set with go env -w. IMO, it doesn't need to be printed. People aren't going to accidentally set GOTELEMETRY=on (or at least it's no more likely than accidentally setting GODEBUG=execerrdot=0), and I understand wanting to be careful after the opt-out telemetry backlash, but this seems to go too far in the other direction, to the point of clunkiness.
I think we should have crossed that bridge only when we came to it, not before. We might not ever need to. Even if we do, that doesn't preclude treating GOTELEMETRY as a normal env var. |
Those are fair points, but #67111 was accepted and implemented and shipped, and it is very unlikely that we'd remove a subcommand, especially for superficial reasons. If the handling of the go telemetry mode could be more elegant, that's perhaps unfortunate, but there are other areas of the language and toolchain that could be more elegant. For now, I think we should focus on making telemetry as unobtrusive as possible, and this issue is about investigating whether supporting an environment variable would be helpful. I suspect that the major problem caused by the current telemetry default is #68946, and perhaps if we fix this then there is no need for supporting |
@hyangah points out a technical consideration that may make GOTELEMETRY=on impossible: currently, when the user runs |
@findleyr
Sorry for my ignorance, but I don't follow why a stored timestamp is required. Aren't all telemetry data guaranteed to be safe for upload, since it was only collected when GOTELEMETRY=on? That property can be preserved by putting GOTELEMETRY=local data in a separate place, which is never uploaded. Assuming that a stored timestamp is required for some reason, ISTM that there could be a separate GOTELEMETRYSTART env var to mark the cut-off time. There's already a second telemetry env var, GOTELEMETRYDIR, so why not a third? When you do
My underlying concern is the go tool moving away from the common, simple config model of env vars that can be set per process or in a config file. There's a handy env subcommand to edit the config file. That's it. It's easy to understand, and to use. Now, there's a writeable "env var" that's listed with the rest of the env vars, but it can't be set with the env subcommand or command-line env vars, and it's not listed in the env var changes. IIRC, this is unprecedented in the go tool. |
Telemetry data is written to the local file system by default, when the go telemetry mode is "local". Such data is only for local debugging. When the user enables telemetry, we don't want to upload any data that was collected before the mode was "on". I think this is crucial to explaining why GOTELEMETRY=on can't be settable, because "on" means that telemetry data is uploadable, and we don't distinguish in the stored data whether that data was recorded when GOTELEMETRY=local or GOTELEMETRY=on. If we were to consider supporting GOTELEMETRY=on as a settable environment variable, I think we'd also need to redesign the telemetry data model to separate counters that were recorded with GOTELEMETRY=local from GOTELEMETRY=on. That's an interesting idea, but beyond the scope of this issue. I also am not sure it would be worth the engineering effort.
If we were to support
I'm not sure what you mean when you say that GOTELEMETRY is writable. It is currently a non-settable env var that is derived from the telemetry mode file. There are other env vars, such as GOMOD, that aren't settable. The fact that GOTELEMETRY isn't well documented This discussion raises many good points about how GOTELEMETRY is confusing. I worry that making GOTELEMETRY=off settable would only add to that confusion. As I said in #68960 (comment), I think we should first fix #68946 and then see if there is a strong need for GOTELEMETRY=off to be settable. |
Change https://go.dev/cl/609115 mentions this issue: |
For #68960 Change-Id: I5495b3d45ad3817d7edf6336a5e24a331d5cf8d0 Reviewed-on: https://go-review.googlesource.com/c/go/+/609115 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
I'll file a separate issue. Thanks.
It would be defined with the GOTELEMETRYSTART env var:
I mean that it's changeable:
Are they changeable through the CLI? |
Thanks for filing that issue. We can continue discussion there.
Hmm. I think I'd prefer a model where the "local" and "on" data is separate, as it seems a bit hard to wrap one's head around GOTELEMETRYSTART as an environment variable. I'll say this on the other issue.
I guess that depends on what you mean by changeable. GOVERSION is perhaps the most complicated, affected by
I think your point is that Would you feel the same way if we didn't expose GOTELEMETRY as an environment variable, and just exposed GOTELEMETRYDIR? |
That would be my preference too. 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.
Part of my point, yes, agreed.
I agree that that would remove the rough edges in go env, go env -w, go env -u, and go env -changed. However, it wouldn't address the inconsistency of how telemetry is configured ( |
Beyond the confusion about an explicit setting and a separate environment variable setting, the other problem with go env -w GOTELEMETRY=... is that every program with telemetry now has to query the go env somehow, which means either they all exec the go command, or they all contain copies of the env-computing code that can go stale. |
This proposal has been declined as infeasible. |
it seems like a strange coincidence that the anti-feature telemetry is the only aspect that cannot be configured like any other path or aspect (now I need a little tool that creates a little file in a directory that cannot be changed?). this fact could easily be interpreted as creating extra obstacles. |
@codespotx Perhaps it is a comfort that since the default is for telemetry reporting to be off, the extra obstacle you mention is making it harder for people to turn telemetry on. |
if the default would be "off" there would be no discussion. |
The default is "local" so that people can have a clear idea of exactly what kinds of data will be uploaded if they turn on telemetry. This avoids the two step process of "turn on local mode", then run a bunch of stuff for a while, and then, if it looks OK, turn telemetry on. |
This will set the default value for go telemetry to 'off' rather than 'local'. Go 1.23 introduced a telemetry feature that collects local audit data about the Go toolchain, storing it by default in $HOME/.config/go/telemetry. While this data is not sent externally by default, the local storage path can trigger security alerts in tools like Falco, as it writes to a sensitive location under /root. The behavior can be disabled with 'go telemetry off', which writes to the config file above, but that means the user needs to do so before calling 'go' in any other manner. Doing so for a container is non-obvious. We could build /root/.config/go/telemetry into a 'go' image, but that would still provide a problem for any user other than uid 0. There is no mechanism to change the behavior "system wide" or an environment variable that can set the value. See golang/go#68960 and golang/go#69113. The second one requests that env GOTELEMETRY=off would disable telemetry. That would be easy for us to utilize but it was rejected upstream. Instead, we just change the default value returned if there is no .config/go/telemetry file present.
As discussed in #68928, while
go env GOTELEMETRY
reports the current go telemetry mode value,GOTELEMETRY=off
has no effect as GOTELEMETRY is not a settable environment variable. Apart from improving the documentation, we should consider supportingGOTELEMETRY=off
as a settable value.We can never support
GOTELEMETRY=on
being settable, because we want users to explicitly rungo telemetry on
to enable telemetry, and because the telemetry mode is a global property that controls whether telemetry data is uploaded. By default, telemetry data is stored in the local file system, and we don't distinguish in that data whether it was recorded while the telemetry mode waslocal
oron
. When the telemetry mode is set toon
, we don't want to upload data that was recorded while the telemetry mode waslocal
.However, apart from the asymmetry I don't think there's a reason we couldn't support GOTELEMETRY=off.
It might be convenient to support disabling telemetry with GOTELEMETRY=off. We should consider it.
EDIT (2024-08-28): updated to explain a bit more why GOTELEMETRY=on can't be settable with the current data model.
The text was updated successfully, but these errors were encountered: