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: bug in telemetry.Start during config download delegation #67211

Closed
findleyr opened this issue May 6, 2024 · 3 comments
Closed
Assignees
Labels
telemetry x/telemetry issues
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented May 6, 2024

The telemetry.Start command uses a fork+exec environment variable trick to run the upload and crash monitor in a subprocess.

However, the upload itself uses the Go command to download the configstore: if the magic environment variable is set, this Go command will also think it is a telemetry.Start subprocess. We must unset the environment variable to avoid this broken delegation.

Filing this issue to track both the fix and vendoring the fix into Go 1.23.

@findleyr findleyr self-assigned this May 6, 2024
@gopherbot gopherbot added the telemetry x/telemetry issues label May 6, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 6, 2024
@gopherbot
Copy link

Change https://go.dev/cl/583495 mentions this issue: telemetry: add a test for Start

@gopherbot
Copy link

Change https://go.dev/cl/583575 mentions this issue: telemetry: in Start, unset X_TELEMETRY_CHILD before running the child

gopherbot pushed a commit to golang/telemetry that referenced this issue May 7, 2024
Add an end-to-end test for Start, which exacerbates the integration bug
described in golang/go#67211.

To achieve this, make the following API changes:
- Add telemetry.Config.{UploadStartTime, UploadURL}, to pass through to
  the upload configuration.
- Add a StartResult return for Start, with a single Wait() method.

Also fix a bug that regtest.RunProg appeared to accidentally lose the
os.Environ when invoking its subprocess.

Updates golang/go#67211
Fixes golang/go#66003

Change-Id: Ic9e53742cd96991452a37601fb5454ef2bdc3d1a
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/583495
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopherbot pushed a commit to golang/telemetry that referenced this issue May 7, 2024
As described in the code, failing to modify X_TELEMETRY_CHILD can lead to
go subcommands acting as the telemetry child themselves, rather than as
the go command. On the other hand, we don't want the delegated go
commands to *also* fork themselves. Therefore, we establish a convention
that X_TELEMETRY_CHILD=1 implies that the process is a telemetry child
process, and X_TELEMETRY_CHILD=2 implies that the process is a child of
a telemetry child process.

For golang/go#67211

Change-Id: Ia6924cc1671f9500f21cc9102cc7d5da33251a83
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/583575
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
@findleyr findleyr modified the milestones: Unreleased, Go1.23 May 7, 2024
@gopherbot
Copy link

Change https://go.dev/cl/584796 mentions this issue: telemetry: re-enable TestStart at go 1.23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
telemetry x/telemetry issues
Projects
None yet
Development

No branches or pull requests

2 participants