Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812083002:
Skip collection and upload of metrics if offline.
|
Created:
3 years, 8 months ago by bcwhite Modified:
3 years, 8 months ago Reviewers:
Steven Holte CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkip collection and upload of metrics if offline.
BUG=
Patch Set 1 #
Total comments: 4
Messages
Total messages: 8 (1 generated)
holte@chromium.org changed reviewers: + holte@chromium.org
https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:734: if (net::NetworkChangeNotifier::IsOffline()) You should call (or schedule a call to) rotation_scheduler_->RotationFinished() before exiting this function. Collection of logs while offline is already somewhat suppressed. The next if condition checks has_unsent_logs, and if so, avoids generating a log. So this change will suppress generation of at most one log.
https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:160: #include "net/base/network_change_notifier.h" I don't think net/ is allowed here, so this probably would need to be wrapped in MetricsServiceClient or similar. https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:734: if (net::NetworkChangeNotifier::IsOffline()) On 2017/04/12 23:11:37, Steven Holte wrote: > You should call (or schedule a call to) rotation_scheduler_->RotationFinished() > before exiting this function. > > Collection of logs while offline is already somewhat suppressed. The next if > condition checks has_unsent_logs, and if so, avoids generating a log. So this > change will suppress generation of at most one log. Actually after rereading the code, this will also suppress generation of "initial" logs if we happen to be in Offline mode at startup (state != SENDING_LOGS), so we'd be generating one log per session instead of 2, which seems more significant, though that also means that we could generate a large "initial" log.
https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... components/metrics/metrics_service.cc:734: if (net::NetworkChangeNotifier::IsOffline()) On 2017/04/12 23:11:37, Steven Holte wrote: > You should call (or schedule a call to) rotation_scheduler_->RotationFinished() > before exiting this function. > > Collection of logs while offline is already somewhat suppressed. The next if > condition checks has_unsent_logs, and if so, avoids generating a log. So this > change will suppress generation of at most one log. Sounds like it's not worth it, then. One (or even two) pending logs should be fine; it's if it grows without bound that could be a problem for devices that are off-line for hours or even days at a time but still viewing offline content during that interval.
On 2017/04/18 14:56:45, bcwhite wrote: > https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... > File components/metrics/metrics_service.cc (right): > > https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... > components/metrics/metrics_service.cc:734: if > (net::NetworkChangeNotifier::IsOffline()) > On 2017/04/12 23:11:37, Steven Holte wrote: > > You should call (or schedule a call to) > rotation_scheduler_->RotationFinished() > > before exiting this function. > > > > Collection of logs while offline is already somewhat suppressed. The next if > > condition checks has_unsent_logs, and if so, avoids generating a log. So this > > change will suppress generation of at most one log. > > Sounds like it's not worth it, then. One (or even two) pending logs should be > fine; it's if it grows without bound that could be a problem for devices that > are off-line for hours or even days at a time but still viewing offline content > during that interval. It might be worth considering changing how the limits on how many logs we store work. Right now they are a weird in that we don't discard logs unless we have at least 8 and at least 300k of persisted logs (IIRC those numbers correctly) and we don't persist logs greater than 100k. Which means the max is 800k for "ongoing" logs. This could probably be simplified a bit, e.g. we could drop the "at least 8" and just limit by size per log and total. Stability logs don't have those limits, so I think they theoretically could grow unbounded, if an offline client is stuck in a crash loop. It might be best to add some.
On 2017/04/18 19:10:23, Steven Holte wrote: > On 2017/04/18 14:56:45, bcwhite wrote: > > > https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... > > File components/metrics/metrics_service.cc (right): > > > > > https://codereview.chromium.org/2812083002/diff/1/components/metrics/metrics_... > > components/metrics/metrics_service.cc:734: if > > (net::NetworkChangeNotifier::IsOffline()) > > On 2017/04/12 23:11:37, Steven Holte wrote: > > > You should call (or schedule a call to) > > rotation_scheduler_->RotationFinished() > > > before exiting this function. > > > > > > Collection of logs while offline is already somewhat suppressed. The next > if > > > condition checks has_unsent_logs, and if so, avoids generating a log. So > this > > > change will suppress generation of at most one log. > > > > Sounds like it's not worth it, then. One (or even two) pending logs should be > > fine; it's if it grows without bound that could be a problem for devices that > > are off-line for hours or even days at a time but still viewing offline > content > > during that interval. > > It might be worth considering changing how the limits on how many logs we store > work. Right now they are a weird in that we don't discard logs unless we have > at least 8 and at least 300k of persisted logs (IIRC those numbers correctly) > and we don't persist logs greater than 100k. Which means the max is 800k for > "ongoing" logs. This could probably be simplified a bit, e.g. we could drop the > "at least 8" and just limit by size per log and total. > > Stability logs don't have those limits, so I think they theoretically could grow > unbounded, if an offline client is stuck in a crash loop. It might be best to > add some. If we don't collect a new log if it has_unsent_logs, how do we reach the limit of 8?
On 2017/04/18 20:10:23, bcwhite wrote: There are a few ways that check is skipped: 1. For stability logs, which go into a different list we don't have that limit for. 2. For initial metrics logs (state != SENDING_LOGS here), shortly after startup. 3. Logs generated on android/ios during OnAppEnterBackground. 4. Logs generated by DisableRecording (called by ~MetricsService() during shutdown) |
||||||||||||||||||||||||||||||||||||||||||||||||||
