atom / telemetry

sends usage metrics to GitHub's internal analytics pipeline
MIT License
11 stars 12 forks source link

Reduce the maximum delay time to send metrics to 10 minutes #30

Closed rafeca closed 5 years ago

rafeca commented 5 years ago

Problem

Currently, due to the setInterval() call happening on the getTimer() function, events are sent 4 hours after the telemetry package has been initialized (this is because the reportingFrequency is 24 hours by default so 6 times less it's the 4 hours).

This means that if a user only creates short-lived sessions (which last less than 4 hours), telemetry will never report stats for her.

Solution

This PR changes the timeout of the setInterval() function to have a maximum value of 10 minutes (instead of the 4 hours) in case the calculation of 6 times less than the reportingFrequency is greater than that value.

On other cases, the previous calculation logic is kept (this is to allow customizing the reportingFrequency to values like 1 minutes for debugging purposes).

I've chosen the 10 minutes ballpark value since it seems long enough to capture some initial activity for that session (so we don't send almost-empty events when opening Atom for the first time) but short enough to include quite short sessions from users (note that missing a few sessions from a user is not an issue, since now the events are persisted to disk, the issue can happen when we consistently miss all the sessions from a user).

rafeca commented 5 years ago

I am wondering if reducing the interval will still cause us to miss a certain percentage of stats for users that only use Atom briefly (e.g., to author commit messages, alter .bash_profile, etc.). Do you think that's negligible? If not, could we send unreported stats on startup instead? 💭

I thought about this, but if we want to keep the current constrain of sending the events only every 24h, this means that the events that happen just after startup won't be sent after the next day for all users.

We could have some smarter logic that checks the number of unsent events on startup, and send them during the startup if that number is above certain threshold.

rafeca commented 5 years ago

After discussing offline about whether we should send the metrics on startup with @as-cii, we've decided to go ahead with this PR and revisit adding some kind of smarter logic to send the metrics.