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

cmd/go: add go telemetry subcommand #67111

Closed
matloob opened this issue Apr 29, 2024 · 14 comments
Closed

cmd/go: add go telemetry subcommand #67111

matloob opened this issue Apr 29, 2024 · 14 comments

Comments

@matloob
Copy link
Contributor

matloob commented Apr 29, 2024

Update: See #67111 (comment) for the current version of the proposal.

Original version of the proposal:

Proposal Details

This is a proposal to add a new go telemetry subcommand, which would have implement the behavior of the on, local, off, andclean subcommands of the golang.org/x/telemetry/cmd/gotelemetry command, as well as a new config command which is a clearer version of gotelemetry env:

  • go telemetry on: same behavior as gotelemetry on: enable telemetry collection and uploading
  • go telemetry local: same behavior as gotelemetry local: enable telemetry collection but disable uploading
  • go telemetry off: same behavior as gotelemetry off: disable telemetry collection and uploading
  • go telemetry config [-json]: print the go telemetry config: including the mode, start time and directory optionally in JSON format
  • go telemetry clean: same behavior as gotelemetry clean: remove all local telemetry data

If implemented, we would refactor the gotelemetry command so that go telemetry could call into the same code.

The rationale for this proposal is that we want to add this to make it easy for users of the go command to modify their telemetry collection settings. This is particularly useful because we're adding opt-in telemetry to the Go toolchain (#58894). With this change, users wouldn't need to install the gotelemetry command on their system to modify their telemetry collection settings.

This proposal wouldn't add the functionality of the gotelemetry view command: that command has JavaScript dependencies that we do not want to add to the main Go repo.

@gopherbot gopherbot added this to the Proposal milestone Apr 29, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Apr 29, 2024
@mauri870 mauri870 added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go labels May 1, 2024
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 1, 2024
@rsc
Copy link
Contributor

rsc commented May 8, 2024

This proposal is only about adding the on/off/local functionality, not telemetry itself. The effect is that users dont have to go install golang.org/x/telemetry/cmd/gotelemetry@latest to configure telemetry settings. That's clearly a good thing. I think this is unobjectionable enough that we can move this to likely accept.

@rsc rsc moved this from Incoming to Likely Accept in Proposals May 8, 2024
@rsc
Copy link
Contributor

rsc commented May 8, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a “go telemetry” sub-command with these sub-sub-commands: on, off, local, clean, and config.

On, off, and local set the telemetry mode. Clean deletes all telemetry data. These match golang.org/x/telemetry/cmd/gotelemetry.

Config prints the telemetry config, like this:

mode: on
start: 2024-05-08T15:10:51-04:00
dir: /Users/matloob/Library/Application Support/go/telemetry

Config -json prints:

{
    "Mode": "on",
    "Start": "2024-05-08T15:10:51-04:00",
    "Dir": "/Users/matloob/Library/Application Support/go/telemetry"
}

Config is a simplification of “gotelemetry env”, which will probably be deleted in favor of this config command.

@TheCoreMan
Copy link

TheCoreMan commented May 10, 2024

One question - isn't there a tradeoff worth consider in supporting go telemetry view -json that doesn't show the charts with JS, but does give access to the raw data?

Seems like from an impl standpoint it'll be cheap, and from usability standpoint it will give people a great sense of transparency about what the telemetry actually is.

@matloob
Copy link
Contributor Author

matloob commented May 14, 2024

@TheCoreMan I think it would be pretty hard for a user to see what's going on by looking at the raw data, especially if we included all counters being written to disk, not just those being uploaded. If we included only those being uploaded it could be more reasonable, but to me the html view is more user friendly and it's a good display of both what's collected locally and what's uploaded, so we should encourage users to look there.

@matloob
Copy link
Contributor Author

matloob commented May 14, 2024

I'd like to update the proposal to make it a bit simpler by adopting a suggestion from @hyangah:

go telemetry prints the mode: either on, off or local
go telemetry on sets the mode to on
go telemetry local sets the mode to local
go telemetry off sets the mode to off

New unsettable GOTELEMETRY and GOTELEMETRYDIR variables are displayed go env containing the telemetry mode (on, off, or local) and the telemetry directory.

go clean -telemetry cleans the telemetry directory.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584535 mentions this issue: cmd/go: add go telemetry command and support in env and clean

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 15, 2024
@rsc
Copy link
Contributor

rsc commented May 15, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a “go telemetry” sub-command with these sub-sub-commands: on, off, local, clean, and config.

On, off, and local set the telemetry mode. Clean deletes all telemetry data. These match golang.org/x/telemetry/cmd/gotelemetry.

Config prints the telemetry config, like this:

mode: on
start: 2024-05-08T15:10:51-04:00
dir: /Users/matloob/Library/Application Support/go/telemetry

Config -json prints:

{
    "Mode": "on",
    "Start": "2024-05-08T15:10:51-04:00",
    "Dir": "/Users/matloob/Library/Application Support/go/telemetry"
}

Config is a simplification of “gotelemetry env”, which will probably be deleted in favor of this config command.

@rsc rsc changed the title proposal: cmd/go: add go telemetry subcommand cmd/go: add go telemetry subcommand May 15, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 15, 2024
gopherbot pushed a commit that referenced this issue May 16, 2024
Add the go telemetry command to support setting and viewing the
telemetry mode. Also add the non-settable GOTELEMETRY and GOTELEMETRYDIR
variables to go env, which contain the mode and telemetry dir.

For #67111

Change-Id: Id7e89cefe30acfe3d865fa467315fe7cda975de9
Reviewed-on: https://go-review.googlesource.com/c/go/+/584535
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 May 23, 2024
@matloob
Copy link
Contributor Author

matloob commented May 23, 2024

We've decided to simplify the proposal to that in #67111 (comment) :

I'd like to update the proposal to make it a bit simpler by adopting a suggestion from @hyangah:

go telemetry prints the mode: either on, off or local go telemetry on sets the mode to on go telemetry local sets the mode to local go telemetry off sets the mode to off

New unsettable GOTELEMETRY and GOTELEMETRYDIR variables are displayed go env containing the telemetry mode (on, off, or local) and the telemetry directory.

go clean -telemetry cleans the telemetry directory.

This has been submitted.

@matloob matloob closed this as completed May 23, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587923 mentions this issue: cmd/go: add release notes for go telemetry, and telemetry env values

gopherbot pushed a commit that referenced this issue May 28, 2024
This change fills in the release notes for the go telemetry command as
well as the unsettable GOTELEMETRY and GOTELEMETRYDIR go env values.

For #67111

Change-Id: Id6943f79f7ab2457787e1639d8d5fb1c1e2649dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/587923
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@zhsj
Copy link
Contributor

zhsj commented Aug 16, 2024

Looks like the clean support is missing.

@matloob
Copy link
Contributor Author

matloob commented Aug 16, 2024

@zhsj Apologies for not updating this issue. It was decided to leave clean in the golang.org/x/telemetry/cmd/gotelemetry command for now. Please use gotelemetry clean to remove all telemetry data.

@tianon
Copy link
Contributor

tianon commented Aug 16, 2024

New unsettable GOTELEMETRY and GOTELEMETRYDIR variables are displayed go env containing the telemetry mode (on, off, or local) and the telemetry directory.

Can you elaborate on why it was decided these would be unsettable? (Apologies if this was discussed somewhere already, I'm not seeing it here. 😭 ❤️)

(Asking because if they were settable via environment variables, I'd be much more likely to enable them in more places because it would be trivial for me to do so. 😞)

@matloob
Copy link
Contributor Author

matloob commented Aug 20, 2024

@tianon Please take a look at #68960 for an issue about being able to setting GOTELEMETRY to off. Even with that proposal, though, GOTELEMETRY=on would not be settable because we want users to explicitly enable telemetry using go telemetry on.

@willfaught
Copy link
Contributor

willfaught commented Aug 21, 2024

Even with that proposal, though, GOTELEMETRY=on would not be settable because we want users to explicitly enable telemetry using go telemetry on.

@matloob Why? go env -w GOTELEMETRY=on or GOTELEMETRY=on go build are just as explicit and clear, and they are consistent with the rest of the env vars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

9 participants