Skip to content

cmd/dist: telemetry support #71826

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.

Sign up for GitHub

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

Jump to bottom
Closed
mauri870 opened this issue Feb 18, 2025 · 4 comments
Closed

cmd/dist: telemetry support #71826

mauri870 opened this issue Feb 18, 2025 · 4 comments
Labels
GoCommand cmd/go Telemetry-Proposal Issues proposing new telemetry counters.

Comments

@mauri870
Copy link
Member

mauri870 commented Feb 18, 2025 •
edited
Loading

CL 585235 added telemetry for commands in cmd, except for cmd/dist.

Since cmd/dist is built with the bootstrap toolchain, enabling telemetry requires Go 1.23 or later.

Go 1.24 requires Go 1.22 for bootstrapping, and Go 1.26 will require Go 1.24 or later, as described in golang/go#69315. If I understand correctly, we will need to wait until the Go 1.26 dev cycle to enable telemetry in cmd/dist.

I'm trying to prototype telemetry in cmd/dist and compile it using Go 1.24 as the bootstrap toolchain to validate this, but I'm encountering the following error:

Building Go cmd/dist using /usr/lib/go. (go1.24.0 linux/amd64)
package ./cmd/dist
        cmd/dist/util.go:21:2: use of internal package cmd/internal/telemetry/counter not allowed

I'm not sure whether this means cmd/dist should be allowed to import internal packages in 1.24 or if the issue is unrelated. Either way, we will need to address this at some point, so it's still worth including this issue in a milestone.

cc @golang/telemetry
@mauri870 mauri870 added the GoCommand cmd/go label Feb 18, 2025
@gabyhelp gabyhelp added the Telemetry-Proposal Issues proposing new telemetry counters. label Feb 19, 2025
@ianlancetaylor
Copy link
Member

The make.bash script is basically using the bootstrap Go toolchain (in this case 1.24) to run the command go build ./cmd/diste. So from the bootstrap toolchain's point of view it's just building some random program. And random programs aren't permitted to import internal packages. So currently cmd/dist can only import ordinary visible packages, which is all that it currently does import.

Note that cmd/dist already copies some standard library code into its own sources, such as copying part of go/build into cmd/dist/imports.go. Perhaps something along those lines could work here.

That said do we expect to learn much from enabling telemetry for cmd/dist?

@mauri870
Copy link
Member Author

Thanks, perhaps I'm focusing too much on whether I could rather than whether I should. A comment in the CL says it will eventually be enabled for cmd/dist, but I think the telemetry team would have a better understanding of the reasoning behind that.

@matloob
Copy link
Contributor

matloob commented Feb 19, 2025

@mauri870 My comment on CL 585235 was a mistake. I think we'd have to copy the code if we wanted to do telemetry in cmd/dist. But that would be pretty messy since we depend on vendored x/telemetry code, so I don't think we should.

@findleyr
Copy link
Member

Agreed, I think until there is a strong reason for telemetry in cmd/dist, let's not worry about it.

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go Telemetry-Proposal Issues proposing new telemetry counters.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ianlancetaylor @mauri870 @matloob @findleyr @gabyhelp