Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175743002:
Add MetricsProvider::OnAppEnterBackground callback. (Closed)
|
Created:
4 years, 5 months ago by Bryan McQuade Modified:
4 years, 4 months ago Reviewers:
Alexei Svitkine (slow) CC:
asvitkine+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd MetricsProvider::OnAppEnterBackground callback.
This allows MetricsProviders to log any histograms for data that's
buffered. For example, The page load metrics subsystem buffers some
metrics data until the end of the page load. These metrics should
also be logged when the app enters the background, in order to avoid
data loss.
https://codereview.chromium.org/2189543002 is a follow up change that
makes use of this new callback when tracking page load metrics.
See https://groups.google.com/a/google.com/d/topic/uma-users/BcJRPxNlJ4E/discussion
for additional context on this change.
BUG=608360
Committed: https://crrev.com/6a49bedce62c4dff0031bd3cfc135440dc219903
Cr-Commit-Position: refs/heads/master@{#408212}
Patch Set 1 #Patch Set 2 : fix comment #Patch Set 3 : fix comment #
Total comments: 5
Patch Set 4 : address comments #
Dependent Patchsets: Messages
Total messages: 28 (22 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add MetricsProvider::OnAppEnterBackground callback. BUG= ========== to ========== Add MetricsProvider::OnAppEnterBackground callback. BUG=608360 ==========
Description was changed from ========== Add MetricsProvider::OnAppEnterBackground callback. BUG=608360 ========== to ========== Add MetricsProvider::OnAppEnterBackground callback. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. BUG=608360 ==========
Description was changed from ========== Add MetricsProvider::OnAppEnterBackground callback. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. BUG=608360 ========== to ========== Add MetricsProvider::OnAppEnterBackground callback. This allows MetricsProviders to log any histograms for data that's buffered. For example, The page load metrics subsystem buffers some metrics data until the end of the page load. These metrics should also be logged when the app enters the background, in order to avoid data loss. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. BUG=608360 ==========
Description was changed from ========== Add MetricsProvider::OnAppEnterBackground callback. This allows MetricsProviders to log any histograms for data that's buffered. For example, The page load metrics subsystem buffers some metrics data until the end of the page load. These metrics should also be logged when the app enters the background, in order to avoid data loss. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. BUG=608360 ========== to ========== Add MetricsProvider::OnAppEnterBackground callback. This allows MetricsProviders to log any histograms for data that's buffered. For example, The page load metrics subsystem buffers some metrics data until the end of the page load. These metrics should also be logged when the app enters the background, in order to avoid data loss. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. See https://groups.google.com/a/google.com/d/topic/uma-users/BcJRPxNlJ4E/discussion for additional context on this change. BUG=608360 ==========
bmcquade@chromium.org changed reviewers: + asvitkine@chromium.org
PTAL. See https://groups.google.com/a/google.com/d/msg/uma-users/BcJRPxNlJ4E/ovdTxK-pBwAJ for some discussion we had about this a few months ago. Thanks! https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:479: for (MetricsProvider* provider : metrics_providers_) I wasnt't sure if we should be informing providers here, inside the test to see if recording is active, or before this if test, even if recording is not active. My sense it it's only useful to implement this callback if histograms are also going to be persisted, and my understanding is they'll only be persisted if we enter the 'if' case above, so it makes sense to invoke the callback here. But I don't understand the code as well, so that may not be the case. Please take a look and let me know which you think is correct.
lgtm % comments https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... components/metrics/metrics_provider.h:43: // application may be killed without further notification after this callback. This seems OK for now, but note that we do have plans to persist metrics even in the case of a crash with bcwhite@'s persistent metrics work. https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:479: for (MetricsProvider* provider : metrics_providers_) On 2016/07/27 13:18:34, Bryan McQuade wrote: > I wasnt't sure if we should be informing providers here, inside the test to see > if recording is active, or before this if test, even if recording is not active. > My sense it it's only useful to implement this callback if histograms are also > going to be persisted, and my understanding is they'll only be persisted if we > enter the 'if' case above, so it makes sense to invoke the callback here. But I > don't understand the code as well, so that may not be the case. Please take a > look and let me know which you think is correct. It's debatable, but I somewhat prefer doing it outside the if. For example, if the state later changes, it would be good to have those metrics have been logged.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Addressed comments. https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... components/metrics/metrics_provider.h:43: // application may be killed without further notification after this callback. On 2016/07/27 at 17:41:11, Alexei Svitkine (slow) wrote: > This seems OK for now, but note that we do have plans to persist metrics even in the case of a crash with bcwhite@'s persistent metrics work. Yeah - I think any subsystem that does buffering is going to continue to have to accept data loss due to crashes. That's an acceptable tradeoff for us - there are certain metrics that we only want to log once we've collected enough information, and we accept that a low loss rate due to browser process crashes is acceptable. But we can at least flush those metrics when the app goes into the background, using this callback. So I think this will continue to be beneficial for us even with bcwhite's persistent metrics. https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2175743002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:479: for (MetricsProvider* provider : metrics_providers_) On 2016/07/27 at 17:41:11, Alexei Svitkine (slow) wrote: > On 2016/07/27 13:18:34, Bryan McQuade wrote: > > I wasnt't sure if we should be informing providers here, inside the test to see > > if recording is active, or before this if test, even if recording is not active. > > My sense it it's only useful to implement this callback if histograms are also > > going to be persisted, and my understanding is they'll only be persisted if we > > enter the 'if' case above, so it makes sense to invoke the callback here. But I > > don't understand the code as well, so that may not be the case. Please take a > > look and let me know which you think is correct. > > It's debatable, but I somewhat prefer doing it outside the if. For example, if the state later changes, it would be good to have those metrics have been logged. Good point, I agree. I moved it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2175743002/#ps60001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add MetricsProvider::OnAppEnterBackground callback. This allows MetricsProviders to log any histograms for data that's buffered. For example, The page load metrics subsystem buffers some metrics data until the end of the page load. These metrics should also be logged when the app enters the background, in order to avoid data loss. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. See https://groups.google.com/a/google.com/d/topic/uma-users/BcJRPxNlJ4E/discussion for additional context on this change. BUG=608360 ========== to ========== Add MetricsProvider::OnAppEnterBackground callback. This allows MetricsProviders to log any histograms for data that's buffered. For example, The page load metrics subsystem buffers some metrics data until the end of the page load. These metrics should also be logged when the app enters the background, in order to avoid data loss. https://codereview.chromium.org/2189543002 is a follow up change that makes use of this new callback when tracking page load metrics. See https://groups.google.com/a/google.com/d/topic/uma-users/BcJRPxNlJ4E/discussion for additional context on this change. BUG=608360 Committed: https://crrev.com/6a49bedce62c4dff0031bd3cfc135440dc219903 Cr-Commit-Position: refs/heads/master@{#408212} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a49bedce62c4dff0031bd3cfc135440dc219903 Cr-Commit-Position: refs/heads/master@{#408212} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
