Skip to content

x/telemetry: simplify state across counters, reports, mode #63218

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

Closed
findleyr opened this issue Sep 25, 2023 · 3 comments
Closed

x/telemetry: simplify state across counters, reports, mode #63218

findleyr opened this issue Sep 25, 2023 · 3 comments
Assignees
Labels
FrozenDueToAge telemetry x/telemetry issues
Milestone

Comments

@findleyr
Copy link
Member

While working on implementing gotelemetry clean (#63142), I started to look into the consequences of deleting state from the telemetry config dir. Here are a few observations:

  • Each counter file, when it is created, looks for the presence of a report file (.json), to detect whether the user is 'new'. If no report file is found, the counter expiry is increased by a week.
  • When creating reports to upload, they are collected by expiry, and a local report is created for any non-empty expiry before today. An upload report is created at the same time, if the telemetry mode is "on".
  • When the upload pass is run, it collects any upload reports that haven't been uploaded, and uploads them.

I see several problems with this:

  • If a user runs gotelemetry clean it will essentially reset their state to that of a new user, and new counters will skip an upload week (their expiry will be set far into the future).
  • If a user runs gotelemetry off I don't see anything preventing old upload reports from being uploaded, though I may have missed something. That's a problem: we should never upload anything if the Mode is off.
  • If a user runs gotelemetry on after a local report has been created (e.g. a few weeks after local collection started), we can potentially find local counters that expire the next day, not at least 7 days into the future as we suggest.
  • If a user runs gotelemetry off and then subsequently runs gotelemetry on, we'll potentially upload new counter files sooner than expected, due to the presence of historical reports.

If I'm not misunderstanding, I think these are all bugs that need to be fixed, and I don't think we can fix them with the current data model, due to the complicated state interactions:

  • counter expiry reflects the state of the local dir at the time the counter was created.
  • upload reports reflect the state of the mode at the time the report was created.

I think the simplest and best fix is to associate an explicit date with the mode file, recording when the mode was set to "on". Then we can easily guarantee that we won't upload data before that date, and won't submit the first upload before T+7.

Originally posted by @findleyr in #63142 (comment)

@findleyr findleyr self-assigned this Sep 25, 2023
@gopherbot gopherbot added the telemetry x/telemetry issues label Sep 25, 2023
@gopherbot gopherbot added this to the Unreleased milestone Sep 25, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/530995 mentions this issue: all: store an asof date in mode, and use it to guard uploads

gopherbot pushed a commit to golang/telemetry that referenced this issue Sep 28, 2023
As described in golang/go#63218, there were various modes where we
could upload data from before gotelemetry was enabled, or before the
week-long grace period had expired.

Fix this rather explicitly by recording the date that telemetry was
enabled in the mode file, and using this to guard uploads.

There are several places where the logic could be tightened, but I opted
for defensiveness given the late nature of this change.

For golang/go#63218

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

Change https://go.dev/cl/532175 mentions this issue: all: remove 7 day offset for new uploads

gopherbot pushed a commit to golang/telemetry that referenced this issue Oct 3, 2023
With opt-in telemetry we don't need to insert an arbitrary delay before
the first upload. Remove special handling for this delay, as well as the
mention of it in the prompt.

Updates golang/go#63218

Change-Id: I7bd1477bfa3d6639dbc84464235be75b6cde974f
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/532175
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
@findleyr
Copy link
Member Author

findleyr commented Oct 5, 2023

This is done.

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

No branches or pull requests

2 participants