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

x/telemetry/config: initial cmd/go and cmd/compile counters #67244

Closed
matloob opened this issue May 7, 2024 · 5 comments
Closed

x/telemetry/config: initial cmd/go and cmd/compile counters #67244

matloob opened this issue May 7, 2024 · 5 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. telemetry x/telemetry issues Telemetry-Accepted Telemetry-Proposal
Milestone

Comments

@matloob
Copy link
Contributor

matloob commented May 7, 2024

Summary

We propose to add counters for cmd/go and cmd/compile invocations (that would be the "denominatior" when trying to determine percentage usage), and for cmd/compile stacks when there's a fatal error, and for the Go command's buildmode flag.

Proposed Config Change

https://golang.org/cl/584015

@gopherbot gopherbot added the telemetry x/telemetry issues label May 7, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 7, 2024
@findleyr
Copy link
Contributor

findleyr commented May 8, 2024

This LGTM, pending resolution of certain technical details that don't affect the overall proposal (the 'version' string is likely meaningless for programs in cmd).

@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label May 8, 2024
@gopherbot
Copy link

Change https://go.dev/cl/585198 mentions this issue: internal/configgen: don't try to list versions for std programs

@findleyr
Copy link
Contributor

This does indeed require additional support in x/telemetry. I believe https://go.dev/cl/585198 should allow this CL to work, at which point you should probably also add a minimum version to each counter, by adding version: go1.23rc1 as to each chartconfig record in config.txt. Then, the generated upload configuration will not contain so many irrelevant versions for these counters.

gopherbot pushed a commit to golang/telemetry that referenced this issue May 14, 2024
Main module version information is not stamped in toolchain binaries,
and therefore we do not have a program version for toolchain programs.

Rather than add special handling for toolchain programs in many places,
to handle the missing version, instead enforce the convention that for
toolchain programs, program version == go version.

Specifically:
- Add internal/telemetry.{IsToolchainProgram,ProgramInfo}, to share the
  logic for interrogating program information.
- Update ProgramInfo to reuse the go version as program version for
  toolchain programs.
- Update config generation to reuse the go version list as program
  version list, for toolchain programs.
- Use the go/version package rather than copying it, now that it is
  available. This means that the config generator requires go1.22, which
  should be fine as it is only run by developers or RelUI.
- Update config generation and validation to use go/version package
  rather than semver package for working with toolchain program
  versions.
- Move chartconfig.Validate to configgen.ValidateChartConfig, since
  validation now requires the go/version package and requires go1.22
  (chartconfig is linked by more packages).

For golang/go#67244

Change-Id: I226b18d37e1ad973002b19afa8afdb8cf97b358c
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585198
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/585376 mentions this issue: internal/chartconfig: add missing newlines between buckets

gopherbot pushed a commit to golang/telemetry that referenced this issue May 14, 2024
A subsequent CL will fix the parsing/validation to ensure that this
would have been caught. For now, let's unbreak the config.

Updates golang/go#67244

Change-Id: I0a867cd7f4547085e6d027f486a069ff8a4e2c4f
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585376
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
@findleyr
Copy link
Contributor

This is now done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. telemetry x/telemetry issues Telemetry-Accepted Telemetry-Proposal
Projects
Status: No status
Development

No branches or pull requests

4 participants