Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491793002:
Add histograms for ExternalDataUseObserver (Closed)
DescriptionAdd histograms for ExternalDataUseObserver
Also, submit another data use report if previous report has
been pending for more than 2 minutes. This prevents
ExternalDataUseObserver from getting blocked in case the
call back fails.
BUG=540061
Committed: https://crrev.com/2238dfbf0f1ae3bac2004ea2303c0d9417ae51a3
Cr-Commit-Position: refs/heads/master@{#364436}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Addressed sclittle comments and some cleanup #
Total comments: 14
Patch Set 3 : Addressed sclittle comments #
Total comments: 14
Patch Set 4 : Addressed sclittle comments #
Total comments: 12
Patch Set 5 : Addressed comments #Patch Set 6 : Rebased #
Total comments: 3
Patch Set 7 : Addressed comments #Patch Set 8 : Rebased #
Messages
Total messages: 31 (13 generated)
Description was changed from ========== w commit before rebase tests passing pass not passing p c BUG= ========== to ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 ==========
tbansal@chromium.org changed reviewers: + sclittle@chromium.org
sclittle: PTAL. Thanks.
Description was changed from ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 ========== to ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous report has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 ==========
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:51: bytes); Histogram samples are casted to int32_t when they're recorded: https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... You could cap the recorded |bytes| at kSampleType_MAX - 1, which is the maximum recordable sample value. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:74: const int kDefaultDataUseReportSubmitTimeoutSeconds = 60 * 2; // 2 minutes. nit: Since you're controlling this with a field trial, could you change the units to milliseconds just in case the extra precision is necessary? https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:283: // |last_data_report_submitted_| is null if no data use report is currently The NULL timeticks is a valid, real time; it shouldn't be used as a sentinel value like this. You'll probably want to keep the bool you had before. see https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:285: base::TimeTicks last_data_report_submitted_; nit: Name this so that it's obvious it's a point in time, e.g. last_data_report_submitted_ticks_ or something. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:288: // is currently pending. |pending_report_bytes_| is 0 if no data use report is FYI: Be careful here - the DataUseAggregator could pass you DataUse objects with both tx_bytes and rx_bytes of 0. It's fine to avoid submitting reports that are 0 bytes, but those will need to be filtered out right before reporting them. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc (right): https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:349: ExternalDataUseObserver::kMaxBufferSize - 1); nit: If these are all the same sample (e.g. the same number of bytes), you could just use ExpectUniqueSample(histogram_name, bytes_sample, kMaxBufferSize - 1) https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:355: ExternalDataUseObserver::kMaxBufferSize - 1); Can you also test the samples in this histogram? https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:657: "DataUsage.ReportSubmission.Successful.Bytes", 1); nit: Can you check the sample for this histogram as well? You can just replace this with a call to ExpectUniqueSample. Same thing everywhere else that you ExpectTotalCount(name, 1) in tests. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:756: histogram_tester.ExpectTotalCount("DataUsage.ReportSubmissionResult", 1); nit: you could get rid of the ExpectTotalCount here and just use ExpectUniqueSample below. https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6441: + the external data use observer timed out. Recorded everytime a report is nit: confusing wording here, what does this actually record? You mention timeouts here, but how do timeouts affect this histogram? https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6467: +<histogram name="DataUsage.ReportSubmissionResult" nit: Is this histogram necessary? Can't someone just compare the count of samples from the bytes histograms here? https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6472: + Result of data usage report submission. Recorded everytime a report is nit: s/everytime/every time/, here and elsewhere
Patchset #2 (id:40001) has been deleted
ptal. thanks!! https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:51: bytes); On 2015/12/02 19:07:54, sclittle wrote: > Histogram samples are casted to int32_t when they're recorded: > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > You could cap the recorded |bytes| at kSampleType_MAX - 1, which is the maximum > recordable sample value. Good point. Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:74: const int kDefaultDataUseReportSubmitTimeoutSeconds = 60 * 2; // 2 minutes. On 2015/12/02 19:07:54, sclittle wrote: > nit: Since you're controlling this with a field trial, could you change the > units to milliseconds just in case the extra precision is necessary? Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:283: // |last_data_report_submitted_| is null if no data use report is currently On 2015/12/02 19:07:54, sclittle wrote: > The NULL timeticks is a valid, real time; it shouldn't be used as a sentinel > value like this. You'll probably want to keep the bool you had before. > > see > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... That applies only when doing Maths with TimeTicks. e.g., TimeTicks1 + TimeDelta = TimeTicks2. It is possible that TimeTicks2 is 0. But it is guaranteed that if TimeTicks has been initialized (without maths operations), then it would have non null value. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:285: base::TimeTicks last_data_report_submitted_; On 2015/12/02 19:07:54, sclittle wrote: > nit: Name this so that it's obvious it's a point in time, e.g. > last_data_report_submitted_ticks_ or something. Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:288: // is currently pending. |pending_report_bytes_| is 0 if no data use report is On 2015/12/02 19:07:54, sclittle wrote: > FYI: Be careful here - the DataUseAggregator could pass you DataUse objects with > both tx_bytes and rx_bytes of 0. It's fine to avoid submitting reports that are > 0 bytes, but those will need to be filtered out right before reporting them. Added an early return statement to filter out reports that have 0 bytes. Also, changed the comment. Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc (right): https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:349: ExternalDataUseObserver::kMaxBufferSize - 1); On 2015/12/02 19:07:55, sclittle wrote: > nit: If these are all the same sample (e.g. the same number of bytes), you could > just use ExpectUniqueSample(histogram_name, bytes_sample, kMaxBufferSize - 1) Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:355: ExternalDataUseObserver::kMaxBufferSize - 1); On 2015/12/02 19:07:54, sclittle wrote: > Can you also test the samples in this histogram? Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:657: "DataUsage.ReportSubmission.Successful.Bytes", 1); On 2015/12/02 19:07:55, sclittle wrote: > nit: Can you check the sample for this histogram as well? You can just replace > this with a call to ExpectUniqueSample. Same thing everywhere else that you > ExpectTotalCount(name, 1) in tests. Done. https://codereview.chromium.org/1491793002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:756: histogram_tester.ExpectTotalCount("DataUsage.ReportSubmissionResult", 1); On 2015/12/02 19:07:55, sclittle wrote: > nit: you could get rid of the ExpectTotalCount here and just use > ExpectUniqueSample below. Done. https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6441: + the external data use observer timed out. Recorded everytime a report is On 2015/12/02 19:07:55, sclittle wrote: > nit: confusing wording here, what does this actually record? You mention > timeouts here, but how do timeouts affect this histogram? Rephrased. https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6467: +<histogram name="DataUsage.ReportSubmissionResult" On 2015/12/02 19:07:55, sclittle wrote: > nit: Is this histogram necessary? Can't someone just compare the count of > samples from the bytes histograms here? This makes it much easier to analyze. https://codereview.chromium.org/1491793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6472: + Result of data usage report submission. Recorded everytime a report is On 2015/12/02 19:07:55, sclittle wrote: > nit: s/everytime/every time/, here and elsewhere Done.
ping!
https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:46: int64_t bytes) { nit: just for clarity, could you add a DCHECK_LE(0, bytes)? https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:50: const int32_t bytes_capped = bytes <= base::HistogramBase::kSampleType_MAX - 1 optional nit: You could use std::min here. https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:335: if (data_use.rx_bytes < 0 || data_use.tx_bytes < 0 || nit: The < 0 checks here aren't necessary, and disallowed by the style guide since you already DCHECK these conditions above. See https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:393: if (!last_data_report_submitted_ticks_.is_null()) { nit: add a comment like "Cancel a pending DataUsage report if it has timed out" or something https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc (right): https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:630: histogram_tester.ExpectUniqueSample("DataUsage.ReportSubmissionResult", 0, 1); Could you make the SubmissionResult enum public in ExternalDataUseObserver, and use the enum value here instead of just a number? https://codereview.chromium.org/1491793002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6450: + Count of bytes in the data use reports that whose submission to the external nit: Does this include timeouts? Please be more specific here about what "failed" means. What part of the path failed? https://codereview.chromium.org/1491793002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6460: + Count of bytes in the data use reports that were lost before an attempt is nit: Could you be more specific here, e.g. that this means that the buffer of data use reports filled up to max?
PTAL. Thanks. https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:46: int64_t bytes) { On 2015/12/07 21:44:08, sclittle wrote: > nit: just for clarity, could you add a DCHECK_LE(0, bytes)? Done. https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:50: const int32_t bytes_capped = bytes <= base::HistogramBase::kSampleType_MAX - 1 On 2015/12/07 21:44:08, sclittle wrote: > optional nit: You could use std::min here. I tried that before but it does not work because one of them is 32-bit, other is 64-bit. I can use static_cast with std::min but that does not look much better either. https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:335: if (data_use.rx_bytes < 0 || data_use.tx_bytes < 0 || On 2015/12/07 21:44:08, sclittle wrote: > nit: The < 0 checks here aren't necessary, and disallowed by the style guide > since you already DCHECK these conditions above. > > See > https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... Right, removed. https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:393: if (!last_data_report_submitted_ticks_.is_null()) { On 2015/12/07 21:44:08, sclittle wrote: > nit: add a comment like "Cancel a pending DataUsage report if it has timed out" > or something Done. https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc (right): https://codereview.chromium.org/1491793002/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:630: histogram_tester.ExpectUniqueSample("DataUsage.ReportSubmissionResult", 0, 1); On 2015/12/07 21:44:08, sclittle wrote: > Could you make the SubmissionResult enum public in ExternalDataUseObserver, and > use the enum value here instead of just a number? Done. https://codereview.chromium.org/1491793002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6450: + Count of bytes in the data use reports that whose submission to the external On 2015/12/07 21:44:08, sclittle wrote: > nit: Does this include timeouts? Please be more specific here about what > "failed" means. What part of the path failed? Done. https://codereview.chromium.org/1491793002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6460: + Count of bytes in the data use reports that were lost before an attempt is On 2015/12/07 21:44:08, sclittle wrote: > nit: Could you be more specific here, e.g. that this means that the buffer of > data use reports filled up to max? Done.
https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:330: if (data_use.rx_bytes == 0 && data_use.tx_bytes == 0) nit: add a comment like "don't submit empty reports" or something https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:387: // Mark the pending DataUsage report as timed out. Can you make it clear somewhere, maybe also in the Java ExternalDataUseObserver or something, that submitting a new report while another is already in progress should cancel the previous submission? https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:57: // Result of data usage report submission. nit: mention that this should be kept in sync with the histograms.xml enum https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:67: DATAUSAGE_REPORT_SUBMISSION_BOUNDARY = 4 nit: isn't the typical naming for this "..._MAX"? https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6474: + report is lost before an attempt is made to submit it to the external data nit: What is the "external data use observer" here? The C++ one? The Java one? https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6485: + the external data use observer. Recorded every time a report is successfully nit: This "Recorded every time..." doesn't actually seem accurate. Isn't this actually recorded when the Java external data use observer notifies that the submission was a success? Otherwise this would include the timeouts and failed reports as well. https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6506: + submitted to the external data use observer. nit: What does "submitted" mean here? Maybe you could just leave out the "Recorded every time..." sentence.
Patchset #4 (id:100001) has been deleted
tbansal@chromium.org changed reviewers: + asvitkine@chromium.org
PTAL: asvitkine: histograms.xml. Thanks. https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:330: if (data_use.rx_bytes == 0 && data_use.tx_bytes == 0) On 2015/12/08 00:36:21, sclittle wrote: > nit: add a comment like "don't submit empty reports" or something Done. https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:387: // Mark the pending DataUsage report as timed out. On 2015/12/08 00:36:21, sclittle wrote: > Can you make it clear somewhere, maybe also in the Java ExternalDataUseObserver > or something, that submitting a new report while another is already in progress > should cancel the previous submission? Added more comments in Java code. https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:57: // Result of data usage report submission. On 2015/12/08 00:36:21, sclittle wrote: > nit: mention that this should be kept in sync with the histograms.xml enum Done. https://codereview.chromium.org/1491793002/diff/80001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:67: DATAUSAGE_REPORT_SUBMISSION_BOUNDARY = 4 On 2015/12/08 00:36:21, sclittle wrote: > nit: isn't the typical naming for this "..._MAX"? Done. Not sure, I have seen both. https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6474: + report is lost before an attempt is made to submit it to the external data On 2015/12/08 00:36:21, sclittle wrote: > nit: What is the "external data use observer" here? The C++ one? The Java one? Done. https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6485: + the external data use observer. Recorded every time a report is successfully On 2015/12/08 00:36:21, sclittle wrote: > nit: This "Recorded every time..." doesn't actually seem accurate. Isn't this > actually recorded when the Java external data use observer notifies that the > submission was a success? > > Otherwise this would include the timeouts and failed reports as well. Done. https://codereview.chromium.org/1491793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6506: + submitted to the external data use observer. On 2015/12/08 00:36:21, sclittle wrote: > nit: What does "submitted" mean here? Maybe you could just leave out the > "Recorded every time..." sentence. Done.
https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:92: * use report while the previous is pending may cause the previous report to be lost. This new sentence contradicts the previous sentence. You should be specific here, e.g. that the previous report should be cancelled and that nativeOnReportDataUseDone will not be called for that cancelled report. https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6462: + notified that the submission failed. Recorded every time platform external nit: add "the" before "platform external..." https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6475: + use observer. A submission may be lost if the data use report could not be nit: fix double space between words https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6506: + either submitted to the platform external data use observer, and when an nit: this "Recorded every time..." sentence is still unclear - isn't success only recorded after the platform EDUObserver notifies that success happened, not when the report is initially submitted to it? I'd just remove this sentence altogether - it doesn't seem to me that it adds any information. https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58396: + <int value="3" label="Lost"/> FYI: If the buffer ever gets too big, there'll be a huge amount of "Lost" values recorded here for each tiny DataUse report that gets lost. Is there any value in recording the "Lost" case together with the other cases here?
ptal. thanks. https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:92: * use report while the previous is pending may cause the previous report to be lost. On 2015/12/08 02:25:41, sclittle wrote: > This new sentence contradicts the previous sentence. You should be specific > here, e.g. that the previous report should be cancelled and that > nativeOnReportDataUseDone will not be called for that cancelled report. There is no concept of canceling a report. Actually, there is no way o cancel it because of there is a timeout, it is impossible to know where the execution got stuck. May be the external observer (e.g., Google Play Service received it, accepted it but the confirmation back got lost somewhere). Again, it is not guaranteed that nativeOnReportDataUseDone will not be called. It may still get called depending on where the timeout happened. Also, e.g., nativeOnReportDataUseDone may be called on a different thread. So, it may get called at 5 minute 0.01 second. Only guarantee is that if two reports are submitted at the same time, then the first one *may* be lost. This is not the best solution but right now the goal is to (i) know how frequently reports are lost in the wild; (ii) Not block forever in case the callback never comes. Hopefully, the number is low enough. https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6462: + notified that the submission failed. Recorded every time platform external On 2015/12/08 02:25:41, sclittle wrote: > nit: add "the" before "platform external..." Done. https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6475: + use observer. A submission may be lost if the data use report could not be On 2015/12/08 02:25:41, sclittle wrote: > nit: fix double space between words Done. https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6506: + either submitted to the platform external data use observer, and when an On 2015/12/08 02:25:41, sclittle wrote: > nit: this "Recorded every time..." sentence is still unclear - isn't success > only recorded after the platform EDUObserver notifies that success happened, not > when the report is initially submitted to it? > > I'd just remove this sentence altogether - it doesn't seem to me that it adds > any information. Done. https://codereview.chromium.org/1491793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58396: + <int value="3" label="Lost"/> On 2015/12/08 02:25:41, sclittle wrote: > FYI: If the buffer ever gets too big, there'll be a huge amount of "Lost" values > recorded here for each tiny DataUse report that gets lost. Is there any value in > recording the "Lost" case together with the other cases here? That's possible if there are many matching labels (more than 300?). I think it is still useful to have the count in a separate dashboard (partly because I am hoping all errors to have very low count).
rebased.
https://codereview.chromium.org/1491793002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.cc:23: namespace { Nit: Move this anon namespace to be inside of chrome::android namespace that you have below. This way, you don't need to prefix things in that namespace. https://codereview.chromium.org/1491793002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1491793002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6480: +<histogram name="DataUsage.ReportSubmission.TimedOut.Bytes" units="bytes"> I suggest switching the order of the suffixes - i.e to "DataUsage.ReportSubmission.Bytes.TimedOut". Then, you can use the <histogram_suffixes> feature in the XML file to reduce the verbosity of these definitions. See comment at top of file on how to use it.
LGTM with nit https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:92: * use report while the previous is pending may cause the previous report to be lost. On 2015/12/08 04:04:32, tbansal1 wrote: > On 2015/12/08 02:25:41, sclittle wrote: > > This new sentence contradicts the previous sentence. You should be specific > > here, e.g. that the previous report should be cancelled and that > > nativeOnReportDataUseDone will not be called for that cancelled report. > > There is no concept of canceling a report. Actually, there is no way o cancel it > because of there is a timeout, it is impossible to know where the execution got > stuck. May be the external observer (e.g., Google Play Service received it, > accepted it but the confirmation back got lost somewhere). > > Again, it is not guaranteed that nativeOnReportDataUseDone will not be called. > It may still get called depending on where the timeout happened. Also, e.g., > nativeOnReportDataUseDone may be called on a different thread. So, it may get > called at 5 minute 0.01 second. > > Only guarantee is that if two reports are submitted at the same time, then the > first one *may* be lost. > > This is not the best solution but right now the goal is to (i) know how > frequently reports are lost in the wild; (ii) Not block forever in case the > callback never comes. Hopefully, the number is low enough. OK, could you reword/remove the previous sentence, about how new reports shouldn't be submitted while another is in progress, since that's not true - e.g., timeouts are a case where the new report *should* be submitted. You can just keep the new sentence you've added here.
Patchset #7 (id:180001) has been deleted
asvitkine: PTAL. Thanks. https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1491793002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:92: * use report while the previous is pending may cause the previous report to be lost. On 2015/12/09 19:42:59, sclittle wrote: > On 2015/12/08 04:04:32, tbansal1 wrote: > > On 2015/12/08 02:25:41, sclittle wrote: > > > This new sentence contradicts the previous sentence. You should be specific > > > here, e.g. that the previous report should be cancelled and that > > > nativeOnReportDataUseDone will not be called for that cancelled report. > > > > There is no concept of canceling a report. Actually, there is no way o cancel > it > > because of there is a timeout, it is impossible to know where the execution > got > > stuck. May be the external observer (e.g., Google Play Service received it, > > accepted it but the confirmation back got lost somewhere). > > > > Again, it is not guaranteed that nativeOnReportDataUseDone will not be called. > > It may still get called depending on where the timeout happened. Also, e.g., > > nativeOnReportDataUseDone may be called on a different thread. So, it may get > > called at 5 minute 0.01 second. > > > > Only guarantee is that if two reports are submitted at the same time, then the > > first one *may* be lost. > > > > This is not the best solution but right now the goal is to (i) know how > > frequently reports are lost in the wild; (ii) Not block forever in case the > > callback never comes. Hopefully, the number is low enough. > > OK, could you reword/remove the previous sentence, about how new reports > shouldn't be submitted while another is in progress, since that's not true - > e.g., timeouts are a case where the new report *should* be submitted. > > You can just keep the new sentence you've added here. Done. https://codereview.chromium.org/1491793002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1491793002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.cc:23: namespace { On 2015/12/09 18:15:47, Alexei Svitkine (slow) wrote: > Nit: Move this anon namespace to be inside of chrome::android namespace that you > have below. This way, you don't need to prefix things in that namespace. Done.
lgtm
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1491793002/#ps240001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491793002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491793002/240001
Message was sent while issue was closed.
Description was changed from ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous report has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 ========== to ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous report has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous report has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 ========== to ========== Add histograms for ExternalDataUseObserver Also, submit another data use report if previous report has been pending for more than 2 minutes. This prevents ExternalDataUseObserver from getting blocked in case the call back fails. BUG=540061 Committed: https://crrev.com/2238dfbf0f1ae3bac2004ea2303c0d9417ae51a3 Cr-Commit-Position: refs/heads/master@{#364436} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2238dfbf0f1ae3bac2004ea2303c0d9417ae51a3 Cr-Commit-Position: refs/heads/master@{#364436} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
