Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871733002:
Add histograms for observing UMA throttling effect. (Closed)
|
Created:
4 years, 8 months ago by gayane -on leave until 09-2017 Modified:
4 years, 8 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms for observing UMA throttling effect.
Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs.
Also, delete a histogram recording a connection type and actualy upload interval used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular.
BUG=599586
Committed: https://crrev.com/0417f978064cbeda802b99b21a88d126c66ff4da
Cr-Commit-Position: refs/heads/master@{#386440}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 20 (8 generated)
Patchset #1 (id:1) has been deleted
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Could you have a look? I have added histograms for counting unsent UMA logs but not the size.
Description was changed from ========== Add histograms for observing UMA throttling effect. BUG=599586 ========== to ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 ==========
https://codereview.chromium.org/1871733002/diff/20001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/20001/components/metrics/metr... components/metrics/metrics_service.cc:889: UMA_HISTOGRAM_COUNTS("UMA.LogUpload.Canceled.CellularConstraint", 1); UMA_HISTOGRAM_COUNTS() allocated 50 buckets. You can use UMA_HISTOGRAM_BOOLEAN(..., true); to log the same data more efficiently. I suggest logging both the true and false cases, so we can get a ratio. https://codereview.chromium.org/1871733002/diff/20001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/1871733002/diff/20001/components/metrics/pers... components/metrics/persisted_logs.cc:141: UMA_HISTOGRAM_COUNTS("UMA.UnSentLog.Dropped", 1); Can you just log the total at the end of the function? Otherwise, it's strange to log a value of 1 here and some other value separately. https://codereview.chromium.org/1871733002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1871733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54873: + </obsolete> Isn't the obsolete tag supposed to be first?
Description was changed from ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 ========== to ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type and actualy upload interval used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 ==========
https://codereview.chromium.org/1871733002/diff/20001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/20001/components/metrics/metr... components/metrics/metrics_service.cc:889: UMA_HISTOGRAM_COUNTS("UMA.LogUpload.Canceled.CellularConstraint", 1); On 2016/04/08 15:08:32, Alexei Svitkine wrote: > UMA_HISTOGRAM_COUNTS() allocated 50 buckets. You can use > UMA_HISTOGRAM_BOOLEAN(..., true); to log the same data more efficiently. > > I suggest logging both the true and false cases, so we can get a ratio. I have added the false case here, but it wouldn't mean that the log will certainly get uploaded. https://codereview.chromium.org/1871733002/diff/20001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/1871733002/diff/20001/components/metrics/pers... components/metrics/persisted_logs.cc:141: UMA_HISTOGRAM_COUNTS("UMA.UnSentLog.Dropped", 1); On 2016/04/08 15:08:32, Alexei Svitkine wrote: > Can you just log the total at the end of the function? Otherwise, it's strange > to log a value of 1 here and some other value separately. Done. https://codereview.chromium.org/1871733002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1871733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54873: + </obsolete> On 2016/04/08 15:08:32, Alexei Svitkine wrote: > Isn't the obsolete tag supposed to be first? Done.
https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... File components/metrics/metrics_reporting_scheduler.cc (left): https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... components/metrics/metrics_reporting_scheduler.cc:61: UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.ActualLogUploadInterval", I'm not convinced we should stop logging this yet. Let's keep it for now. https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:889: UMA_HISTOGRAM_BOOLEAN("UMA.LogUpload.Canceled.CellularConstraint", true); Can we log this only if Cellular upload logic is enabled? https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:892: UMA_HISTOGRAM_BOOLEAN("UMA.LogUpload.Canceled.CellularConstraint", false); UMA macros expand to a bunch of code. Please refactor to invoke the macro only once. https://codereview.chromium.org/1871733002/diff/40001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/1871733002/diff/40001/components/metrics/pers... components/metrics/persisted_logs.cc:147: UMA_HISTOGRAM_COUNTS("UMA.UnSentLog.Dropped", dropped_logs_num); Nit: UMA.UnsentLogs.Dropped (Unsent is one word and there's multiple that can be dropped.) https://codereview.chromium.org/1871733002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1871733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54866: + Simple counter of the log upload cancel events due to throttling on I would make the description more clear. What's a "log upload cancel event"? A more clear description could be: Logs whether a log was not uploaded due to cellular log throttling logic. Android only. https://codereview.chromium.org/1871733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55075: + <summary>Counter for number of UMA unsent logs dropped.</summary> Please explain what "dropped" means.
https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... File components/metrics/metrics_reporting_scheduler.cc (left): https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... components/metrics/metrics_reporting_scheduler.cc:61: UMA_HISTOGRAM_CUSTOM_COUNTS("UMA.ActualLogUploadInterval", On 2016/04/08 20:07:12, Alexei Svitkine wrote: > I'm not convinced we should stop logging this yet. Let's keep it for now. Done. https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:889: UMA_HISTOGRAM_BOOLEAN("UMA.LogUpload.Canceled.CellularConstraint", true); On 2016/04/08 20:07:12, Alexei Svitkine wrote: > Can we log this only if Cellular upload logic is enabled? I am not sure I follow. It is the the IF with client_->IsUMACellularUploadLogicEnabled() condition https://codereview.chromium.org/1871733002/diff/40001/components/metrics/metr... components/metrics/metrics_service.cc:892: UMA_HISTOGRAM_BOOLEAN("UMA.LogUpload.Canceled.CellularConstraint", false); On 2016/04/08 20:07:12, Alexei Svitkine wrote: > UMA macros expand to a bunch of code. Please refactor to invoke the macro only > once. Done. https://codereview.chromium.org/1871733002/diff/40001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/1871733002/diff/40001/components/metrics/pers... components/metrics/persisted_logs.cc:147: UMA_HISTOGRAM_COUNTS("UMA.UnSentLog.Dropped", dropped_logs_num); On 2016/04/08 20:07:12, Alexei Svitkine wrote: > Nit: UMA.UnsentLogs.Dropped > > (Unsent is one word and there's multiple that can be dropped.) Done. https://codereview.chromium.org/1871733002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1871733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54866: + Simple counter of the log upload cancel events due to throttling on On 2016/04/08 20:07:13, Alexei Svitkine wrote: > I would make the description more clear. What's a "log upload cancel event"? > > A more clear description could be: > > Logs whether a log was not uploaded due to cellular log throttling logic. > Android only. Done. https://codereview.chromium.org/1871733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55075: + <summary>Counter for number of UMA unsent logs dropped.</summary> On 2016/04/08 20:07:13, Alexei Svitkine wrote: > Please explain what "dropped" means. Done.
https://codereview.chromium.org/1871733002/diff/60001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/60001/components/metrics/metr... components/metrics/metrics_service.cc:894: UMA_HISTOGRAM_BOOLEAN("UMA.LogUpload.Canceled.CellularConstraint", Let's not log this unless IsUMACellularUploadLogicEnabled() is true, so that we're not logging this when it's not relevant. https://codereview.chromium.org/1871733002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1871733002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55068: +<histogram name="UMA.UnSentLogs.Dropped"> Nit: UnSentLogs -> UnsentLogs
https://codereview.chromium.org/1871733002/diff/60001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/60001/components/metrics/metr... components/metrics/metrics_service.cc:894: UMA_HISTOGRAM_BOOLEAN("UMA.LogUpload.Canceled.CellularConstraint", On 2016/04/11 15:09:08, Alexei Svitkine wrote: > Let's not log this unless IsUMACellularUploadLogicEnabled() is true, so that > we're not logging this when it's not relevant. Done. https://codereview.chromium.org/1871733002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1871733002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55068: +<histogram name="UMA.UnSentLogs.Dropped"> On 2016/04/11 15:09:08, Alexei Svitkine wrote: > Nit: UnSentLogs -> UnsentLogs Done.
lgtm https://codereview.chromium.org/1871733002/diff/80001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/80001/components/metrics/metr... components/metrics/metrics_service.cc:895: if (is_cellular_logic) Nit: {}
https://codereview.chromium.org/1871733002/diff/80001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1871733002/diff/80001/components/metrics/metr... components/metrics/metrics_service.cc:895: if (is_cellular_logic) On 2016/04/11 16:04:49, Alexei Svitkine wrote: > Nit: {} Done.
The CQ bit was checked by gayane@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/1871733002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871733002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871733002/100001
Message was sent while issue was closed.
Description was changed from ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type and actualy upload interval used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 ========== to ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type and actualy upload interval used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type and actualy upload interval used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 ========== to ========== Add histograms for observing UMA throttling effect. Added a histogram for counting number of events when UMA log was not uploaded due to throttling and one more for counting number of dropped UMA logs. Also, delete a histogram recording a connection type and actualy upload interval used to upload the previous UMA log which is not needed as we are done with the analysis of UMA logs on cellular. BUG=599586 Committed: https://crrev.com/0417f978064cbeda802b99b21a88d126c66ff4da Cr-Commit-Position: refs/heads/master@{#386440} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0417f978064cbeda802b99b21a88d126c66ff4da Cr-Commit-Position: refs/heads/master@{#386440} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
