golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.44k stars 17.59k forks source link

x/telemetry/counter: simplify counter file release logic by always rotate it daily #63692

Open hyangah opened 11 months ago

hyangah commented 11 months ago

The current implementation computes the counter file end time during the file creation time. The end time is the next upload date (that's determined by the day of week written in the weekends file). An exception also applies for the new users - instead of the next upload date, the counter packages picks 7+ days later. This exception was originally designed before the telemetry was changed to the opt-in model, to ensure to give at least a week before uploading telemetry data. However, this requirement is no longer critical in the current telemetry model.

The start time is on the other hand determined mostly by the instrumented process's start time (unless the process is a long running process and the counter file rotation happens to occur during the process lifetime). For example, if a user starts gopls on oct 21, 22 and 23, and the week ends on oct 24 according to the user's weekends file, there are three counter files (created on 2023-10-21, 2023-10-22, and 2023-10-23) and they all have the end date 2023-10-24. In theory the 2023-10-21 file can be still active and certain processes are still holding the reference.

This end time computation adds extra complexity: the counter package computes the end time deep in the call chain of rotate -> rotate1 -> filename -> fileValidity -> read weekends. The upload package also cannot safely determine when to upload a file without reading the file's content and its metadata.

If we change the counter file to always rotate daily, we may simplify both counter and uploader.

Additional benefits include:

cc @golang/tools-team @pjweinb @findleyr

pjweinb commented 11 months ago

I think the 7 days is worth preserving. It is worth giving users some guaranteed time to look at the counters they are generated.

hyangah commented 11 months ago

@pjweinb Now we are asking users to opt in after data is already generated and certain time is passed. So users are given plenty of time to look into counters before they opt into telemetry. And, UX-wise, I think it is more confusing for users if the time between the user changes configuration and the change takes effect gets longer.

pjweinb commented 9 months ago

Rotating daily moves the complexity to the upload package, so this does not seem like something worth doing.

findleyr commented 3 months ago

@rsc recently pointed out the asymmetry between creating a new file daily, and yet rotating only at the week end.

I'd like to resurrect this issue. It seems simpler to have slightly more complicated logic in the uploader (grouping counter files into week-long partitions), than to distribute the information already contained in the weekends file across every counter file.

In other words, we could drop/ignore TimeEnd for each counter file, and just rotate the file daily.

This also means that counter.Open would not depend on the weekends file, and therefore that the weekends file would be wholly owned by the uploader, which also seems simpler.

This is not going to change for 1.23, but I think we should consider it for 1.24.