Skip to content

x/telemetry: improve the testability of Upload #66003

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
findleyr opened this issue Feb 28, 2024 · 4 comments
Closed

x/telemetry: improve the testability of Upload #66003

findleyr opened this issue Feb 28, 2024 · 4 comments
Assignees
@findleyr
Labels
FrozenDueToAge telemetry x/telemetry issues
Milestone
Unreleased

Comments

@findleyr
Copy link
Member

Integrating telemetry with cmd/go may result in potentially many more calls to x/telemetry/upload.Run, which could interact in complicated ways. While we're guarding against this in #65970 and #65500, we also want to have improved integration testing with the upload process.

This is currently challenging, because the behavior of uploading interacts with a fair bit of global or system state:

  • the system time
  • the location of the "local" and "upload" directories
  • the global logger
  • the upload URL
  • the telemetry mode (which is contained in neither the local nor upload directories)

We should revisit this to make it easier to integration test uploading, perhaps by threading through a per-operation logger, clock, upload URL, and telemetry directory. Then cmd/go could assert on certain invariants, such as that expired counts are correctly uploaded following one (or many) calls to Upload.
@gopherbot gopherbot added the telemetry x/telemetry issues label Feb 28, 2024
@gopherbot gopherbot added this to the Unreleased milestone Feb 28, 2024
@findleyr findleyr self-assigned this Feb 28, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/575059 mentions this issue: internal/telemetry: encapsulate the telemetry directory

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/575060 mentions this issue: internal/upload: close over state when uploading, for easier testing

gopherbot pushed a commit to golang/telemetry that referenced this issue Apr 5, 2024
@findleyr
internal/telemetry: encapsulate the telemetry directory
5e7a336 
In several places throughout tests, we set all of telemetry.{LocalDir,
UploadDir, Mode} according to the standard schema ("local/", "upload/",
and "mode").

In the interest of reducing or eliminating global state to improve
testability, introduce a telemetry.Dir type which encapsulates the
telemetry directory layout. The default layout is accessed through
telemetry.Default. This makes it easier for future components to close
over or pass around a single piece of state (the Dir). In subsequent
CLs, this will be used to make uploading more testable.

For golang/go#66003

Change-Id: I31db8df20f133d834219ff17c2fe2d4f9e446b5d
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/575059
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Peter Weinberger <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/581735 mentions this issue: internal/upload: use a real proxy during tests

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/telemetry that referenced this issue May 7, 2024
@findleyr
internal/upload: close over state when uploading, for easier testing
79576a6 
As described in golang/go#66003 (and illustrated in a recent bug in go
command integration), it is important that downstream tools are able to
write end-to-end tests for uploading. Yet the upload process accessed a
fair amount of shared global state, presenting a challenge for
testability.

Fix this by closing over all state in the Uploader, and expose
configuration hooks in a new upload.RunConfig type. Specifically:

- Rename upload.Control to upload.RunConfig (since it is config for
  Run), and move it into the internal/upload package (with an alias in
  upload), so that it can be used for internal/telemetry tests
- Add fields necessary for testing to RunConfig: TelemetryDir,
  UploadURL, LogWriter, UploadConfig, and StartTime.
- Unexport all fields from Uploader, since hooks are all exposed via
  RunConfig.
- Remove the global upload.logger, since logging is now scoped to
  individual log operations.
- Move initialization of the Uploader into NewUploader, rather than
  delaying the resolution of the UploadConfig or having a separate
  LogIfDebug call. This way, the constructed Uploader is ready to use
  and immutable.
- Let upload.Run (both variants) return an error if the upload failed
  completely.
- Update configstore.Download to return a pointer, for convenience.
- Add the Debug dir to the internal/telemetry.Dir mapper.

Together, these changes expose the seams necessary to test the Upload
(possibly concurrently) from external integrations such as the Go
command. In the future, we may want to reduce the surface area of this
configuration, for example by setting GOPROXY rather than passing in an
upload configuration.

Also:
 - Fix a bug where uploads proceeded even if the config download failed.
   Now, the config is downloaded during NewUploader, which can fail with
   an error.
 - Change the debug directory from "<telemetryDir>/local/debug" to
   "<telemetryDir>/debug", since the nesting felt unnecessary (I don't
   feel strongly about this)

For golang/go#66003

Change-Id: I4125bfe682d11122c5bb2a605717a366108a862d
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/575060
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
gopherbot pushed a commit to golang/telemetry that referenced this issue May 7, 2024
@findleyr
internal/upload: use a real proxy during tests
0f8345c 
Rather than inject the UploadConfig in tests, and bypass downloading,
add a test helper that creates a test environment that can be used for
downloading the config from a local file-based go proxy.

This allows tests to exercise Go command integration.

For golang/go#66003

Change-Id: Ia51c0b83419a53561983ad4e8c24340be97a1c20
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/581735
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@golang golang locked and limited conversation to collaborators May 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@findleyr findleyr

Labels
FrozenDueToAge telemetry x/telemetry issues
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
2 participants
@gopherbot @findleyr