Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
794493002:
Add UMA Histogram Manager to Cronet. (Closed)
|
Created:
6 years ago by ramant (doing other things) Modified:
6 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Hamilton Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA Histogram Manager to Cronet.
Part of change to collect, upload and expose histogram data
for Cronet.
BUG=441466
Committed: https://crrev.com/4eb0fd47de0736f3f4c970e8d7f8813ef2177886
Cr-Commit-Position: refs/heads/master@{#308239}
Committed: https://crrev.com/7254696a81e31cc34a509a692c145690c4e58755
Cr-Commit-Position: refs/heads/master@{#308427}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fixed comments in Patch Set 1 #
Total comments: 5
Patch Set 3 : fixed comments for Patch Set 2 #
Total comments: 10
Patch Set 4 : Added end to end unittest #
Total comments: 11
Patch Set 5 : moved histogram_manager_unittest to components/metrics #
Total comments: 8
Patch Set 6 : #
Total comments: 2
Patch Set 7 : Fix build errors #
Total comments: 14
Patch Set 8 : removed code that uses return value optimization #
Total comments: 4
Patch Set 9 : Fixed flaky histogram_manager_unittest #Patch Set 10 : fixed a bug in histogram_manager_unittest #Patch Set 11 : use lazy_instance to avoid pulling in AnnotateHappensAfter into cronet #Patch Set 12 : added dynamic_annotations.gyp #Patch Set 13 : disabled flaky HistogramBucketFields unittest #Messages
Total messages: 71 (23 generated)
rtenneti@chromium.org changed reviewers: + asvitkine@chromium.org, mef@chromium.org
Work in progress. Fixes comments in https://codereview.chromium.org/769873003/
Raman, thanks a lot for picking this up! Alexei, could you take a look, this CL addresses your comments for my original CL, https://codereview.chromium.org/769873003. What's a good way to test this? https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:72: HistogramManager* histogram_manager = HistogramManager::GetInstance(); I think having it as singleton for initial CL is Ok, I can convert it to hang off java object in the follow up CL.
Thanks, looks very good - a few more comments. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:17: #include "components/metrics/metrics_log.h" This is no longer needed. Please check the other includes too. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:22: using base::SampleCountIterator; This is no longer needed. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:26: static const char kCronetHistogramName[] = "cronet"; This is not used. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:72: HistogramManager* histogram_manager = HistogramManager::GetInstance(); On 2014/12/10 15:06:52, mef wrote: > I think having it as singleton for initial CL is Ok, I can convert it to hang > off java object in the follow up CL. I agree, this is fine for now. Maybe add a TODO about it. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:74: int data_size = histogram_manager->uma_proto().ByteSize(); |uma_proto| should be cleared between calls. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.h:21: class HistogramSnapshotManager; If these all come from the methods you're overriding from HistogramFlattener, you don't need to forward declare them, since the header for HistogramFlattener would already. But you do need to include the header for histogram flattener since you're inheriting from it... https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.h:28: class HistogramManager : public base::HistogramFlattener { Add a basic comment to the class. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... File components/metrics/histogram_encoder.cc (right): https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:7: #include <string> Nit: Add a line after this. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:12: #include "components/metrics/metrics_log.h" This include shouldn't be needed - I think you just need to include the proto for ChromeUserMetricsExtension. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:18: // extern Remove comment. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:19: void RecordHistogramDelta(ChromeUserMetricsExtension& uma_proto, Nit: Pass by pointer, also modifiable params should be last. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... File components/metrics/histogram_encoder.h (right): https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.h:22: class ChromeUserMetricsExtension; You have an include for this on line 14. You don't need both. Remove one or the other. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.h:25: extern void RecordHistogramDelta(ChromeUserMetricsExtension* uma_proto, Nit: No need for extern. https://codereview.chromium.org/794493002/diff/1/components/metrics/metrics_l... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/794493002/diff/1/components/metrics/metrics_l... components/metrics/metrics_log.cc:182: metrics::RecordHistogramDelta(&uma_proto_, histogram_name, snapshot); Nit: This is already in metrics namespace, so no need for metrics:: prefix here.
In terms of testing, I suggest having a unit test that does the following: Logs a histogram sample. Uses this end to end to get some bytes. Deserializes the bytes an checks if that histogram sample is there. Then logs another histogram sample and get the bytes again. Deserializing the new bytes and checks that only the new sample is there (i.e. that its the delta).
Looks great, just a few more things. https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... components/cronet/android/histogram_manager.cc:69: if (histogram_manager->uma_proto().SerializeToArray(&data[0], data_size)) { I suggest making a method of HistogramManager that can serialize to std::vector<uint8> which this function will call. Then, the unit test I suggested in a previous comment can just test the C++ class directly and the Java code will just be a wrapper over it. https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... components/cronet/android/histogram_manager.h:32: // HistogramFlattener methods: Nit: I think convention for new code is to have this comment just be: // base::HistogramFlattener: Also, these methods can all be private, I think. https://codereview.chromium.org/794493002/diff/20001/components/metrics/histo... File components/metrics/histogram_encoder_unittest.cc (right): https://codereview.chromium.org/794493002/diff/20001/components/metrics/histo... components/metrics/histogram_encoder_unittest.cc:52: MetricsLog log("totally bogus client ID", 137, MetricsLog::ONGOING_LOG, I think you should be able to omit the MetricsLog and the TestMetricsServiceClient and TestingPrefServiceSimple from this test. Just declare uma_proto on the stack: ChromeUserMetricsExtension uma_proto;
Hi Alexei, Implemented all the changes you have suggested. This code is still not ready. Will work with mef to test on cronet and write the end to end test for HistogramManager. thanks raman https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:17: #include "components/metrics/metrics_log.h" On 2014/12/10 15:19:31, Alexei Svitkine wrote: > This is no longer needed. Please check the other includes too. Done. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:22: using base::SampleCountIterator; On 2014/12/10 15:19:31, Alexei Svitkine wrote: > This is no longer needed. Done. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:26: static const char kCronetHistogramName[] = "cronet"; On 2014/12/10 15:19:31, Alexei Svitkine wrote: > This is not used. Done. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:72: HistogramManager* histogram_manager = HistogramManager::GetInstance(); On 2014/12/10 15:06:52, mef wrote: > I think having it as singleton for initial CL is Ok, I can convert it to hang > off java object in the follow up CL. Acknowledged. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:72: HistogramManager* histogram_manager = HistogramManager::GetInstance(); On 2014/12/10 15:19:31, Alexei Svitkine wrote: > On 2014/12/10 15:06:52, mef wrote: > > I think having it as singleton for initial CL is Ok, I can convert it to hang > > off java object in the follow up CL. > > I agree, this is fine for now. Maybe add a TODO about it. Done. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.cc:74: int data_size = histogram_manager->uma_proto().ByteSize(); On 2014/12/10 15:19:31, Alexei Svitkine wrote: > |uma_proto| should be cleared between calls. Done. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.h:21: class HistogramSnapshotManager; On 2014/12/10 15:19:31, Alexei Svitkine wrote: > If these all come from the methods you're overriding from HistogramFlattener, > you don't need to forward declare them, since the header for HistogramFlattener > would already. But you do need to include the header for histogram flattener > since you're inheriting from it... Done. https://codereview.chromium.org/794493002/diff/1/components/cronet/android/hi... components/cronet/android/histogram_manager.h:28: class HistogramManager : public base::HistogramFlattener { On 2014/12/10 15:19:31, Alexei Svitkine wrote: > Add a basic comment to the class. Done. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... File components/metrics/histogram_encoder.cc (right): https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:7: #include <string> On 2014/12/10 15:19:31, Alexei Svitkine wrote: > Nit: Add a line after this. Done. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:12: #include "components/metrics/metrics_log.h" On 2014/12/10 15:19:31, Alexei Svitkine wrote: > This include shouldn't be needed - I think you just need to include the proto > for ChromeUserMetricsExtension. This code was calling MetricsLog::Hash. Added a TODO to rename it and move it to metrics_hashes.h. Will do in the next CL. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:18: // extern On 2014/12/10 15:19:31, Alexei Svitkine wrote: > Remove comment. Done. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.cc:19: void RecordHistogramDelta(ChromeUserMetricsExtension& uma_proto, On 2014/12/10 15:19:31, Alexei Svitkine wrote: > Nit: Pass by pointer, also modifiable params should be last. Done. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... File components/metrics/histogram_encoder.h (right): https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.h:22: class ChromeUserMetricsExtension; On 2014/12/10 15:19:31, Alexei Svitkine wrote: > You have an include for this on line 14. You don't need both. Remove one or the > other. Done. https://codereview.chromium.org/794493002/diff/1/components/metrics/histogram... components/metrics/histogram_encoder.h:25: extern void RecordHistogramDelta(ChromeUserMetricsExtension* uma_proto, On 2014/12/10 15:19:31, Alexei Svitkine wrote: > Nit: No need for extern. Done. https://codereview.chromium.org/794493002/diff/1/components/metrics/metrics_l... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/794493002/diff/1/components/metrics/metrics_l... components/metrics/metrics_log.cc:182: metrics::RecordHistogramDelta(&uma_proto_, histogram_name, snapshot); On 2014/12/10 15:19:31, Alexei Svitkine wrote: > Nit: This is already in metrics namespace, so no need for metrics:: prefix here. Done. https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... components/cronet/android/histogram_manager.cc:69: if (histogram_manager->uma_proto().SerializeToArray(&data[0], data_size)) { On 2014/12/10 21:50:17, Alexei Svitkine wrote: > I suggest making a method of HistogramManager that can serialize to > std::vector<uint8> which this function will call. Then, the unit test I > suggested in a previous comment can just test the C++ class directly and the > Java code will just be a wrapper over it. Done. Will work with mef to write the unit test. https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/20001/components/cronet/androi... components/cronet/android/histogram_manager.h:32: // HistogramFlattener methods: On 2014/12/10 21:50:17, Alexei Svitkine wrote: > Nit: I think convention for new code is to have this comment just be: > > // base::HistogramFlattener: > > Also, these methods can all be private, I think. Done.
Will wait for the test, but a few more comments in the meanwhile. https://codereview.chromium.org/794493002/diff/40001/components/cronet/androi... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/40001/components/cronet/androi... components/cronet/android/histogram_manager.h:35: const metrics::ChromeUserMetricsExtension& uma_proto() const { I don't think you need to have this method anymore. https://codereview.chromium.org/794493002/diff/40001/components/metrics/histo... File components/metrics/histogram_encoder_unittest.cc (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/histo... components/metrics/histogram_encoder_unittest.cc:53: &client, &prefs); I think you missed a comment from earlier - you don't actually need a MetricsLog object or the TestingPrefServiceSimple and TestMetricsServiceClient objects. Just declare ChromeUserMetricsExtension on the stack: ChromeUserMetricsExtension uma_proto; https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... components/metrics/metrics_log.cc:182: metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); Nit: No need for metrics:: prefix here. https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... File components/metrics/metrics_log.h (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... components/metrics/metrics_log.h:144: ChromeUserMetricsExtension* uma_proto() { return &uma_proto_; } Revert this change, since it isn't needed per my other suggestion.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Added end to end unit test. Tested it my moving all the code into components/metrics area. Added new .Cronet histograms to histograms.xml https://codereview.chromium.org/794493002/diff/40001/components/cronet/androi... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/40001/components/cronet/androi... components/cronet/android/histogram_manager.h:35: const metrics::ChromeUserMetricsExtension& uma_proto() const { On 2014/12/10 22:36:37, Alexei Svitkine wrote: > I don't think you need to have this method anymore. Done. https://codereview.chromium.org/794493002/diff/40001/components/metrics/histo... File components/metrics/histogram_encoder_unittest.cc (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/histo... components/metrics/histogram_encoder_unittest.cc:53: &client, &prefs); On 2014/12/10 22:36:37, Alexei Svitkine wrote: > I think you missed a comment from earlier - you don't actually need a MetricsLog > object or the TestingPrefServiceSimple and TestMetricsServiceClient objects. > Just declare ChromeUserMetricsExtension on the stack: > > ChromeUserMetricsExtension uma_proto; Thanks much. Missed it. Done. https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... components/metrics/metrics_log.cc:182: metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); On 2014/12/10 22:36:37, Alexei Svitkine wrote: > Nit: No need for metrics:: prefix here. Compiler prefers to use class method and complains about wrong arguments. Should we rename the extern method? https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... File components/metrics/metrics_log.h (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... components/metrics/metrics_log.h:144: ChromeUserMetricsExtension* uma_proto() { return &uma_proto_; } On 2014/12/10 22:36:37, Alexei Svitkine wrote: > Revert this change, since it isn't needed per my other suggestion. Done. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... File components/cronet/android/histogram_manager_unittest.cc (right): https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:31: TEST_F(HistogramManagerTest, HistogramBucketFields) { Tested this code by moving all HistogramManager.* into components/metrics area and running unit tests there. Wondering if we should most of the histogram_managet.* code into components/metrics and just leave Java wrapper code in cronet/android area. Alexei/Misha: wdyt?
https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... File components/cronet/android/histogram_manager_unittest.cc (right): https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:31: TEST_F(HistogramManagerTest, HistogramBucketFields) { On 2014/12/11 02:21:53, ramant wrote: > Tested this code by moving all HistogramManager.* into components/metrics area > and running unit tests there. > > Wondering if we should most of the histogram_managet.* code > into components/metrics and just leave Java wrapper code in > cronet/android area. > > Alexei/Misha: wdyt? sgtm. I like making Java wrapper as thin as possible.
https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... components/metrics/metrics_log.cc:182: metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); On 2014/12/11 02:21:53, ramant wrote: > On 2014/12/10 22:36:37, Alexei Svitkine wrote: > > Nit: No need for metrics:: prefix here. > > Compiler prefers to use class method and complains about wrong arguments. Should > we rename the extern method? Yeah, let's rename it. How about EncodeHistogramDelta() for the standalone method? https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... File components/cronet/android/histogram_manager_unittest.cc (right): https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:18: class HistogramManagerTest : public testing::Test { If this class doesn't have anything in it, you can actually just remove it entirely and change TEST_F() to TEST() below. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:29: using metrics::HistogramEventProto; Nit: I prefer not having using statements and just prefix these in the code. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:31: TEST_F(HistogramManagerTest, HistogramBucketFields) { On 2014/12/11 02:21:53, ramant wrote: > Tested this code by moving all HistogramManager.* into components/metrics area > and running unit tests there. > > Wondering if we should most of the histogram_managet.* code > into components/metrics and just leave Java wrapper code in > cronet/android area. > > Alexei/Misha: wdyt? My preference is to keep this in the cronet component. But if it's really infeasible to set up the tests here, I am ok with this being inside the metrics component for now with a TODO to fix later. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:39: EXPECT_LT(0u, data.size()); Shouldn't this expect be before ParseFromArray()? Also, if you're just testing that data is not empty, I'd just check .empty() is false. Also, please check the return value of ParseFromArray.
On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... > File components/metrics/metrics_log.cc (right): > > https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... > components/metrics/metrics_log.cc:182: > metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); > On 2014/12/11 02:21:53, ramant wrote: > > On 2014/12/10 22:36:37, Alexei Svitkine wrote: > > > Nit: No need for metrics:: prefix here. > > > > Compiler prefers to use class method and complains about wrong arguments. > Should > > we rename the extern method? > > Yeah, let's rename it. How about EncodeHistogramDelta() for the standalone > method? > > https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... > File components/cronet/android/histogram_manager_unittest.cc (right): > > https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... > components/cronet/android/histogram_manager_unittest.cc:18: class > HistogramManagerTest : public testing::Test { > If this class doesn't have anything in it, you can actually just remove it > entirely and change TEST_F() to TEST() below. > > https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... > components/cronet/android/histogram_manager_unittest.cc:29: using > metrics::HistogramEventProto; > Nit: I prefer not having using statements and just prefix these in the code. > > https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... > components/cronet/android/histogram_manager_unittest.cc:31: > TEST_F(HistogramManagerTest, HistogramBucketFields) { > On 2014/12/11 02:21:53, ramant wrote: > > Tested this code by moving all HistogramManager.* into components/metrics area > > and running unit tests there. > > > > Wondering if we should most of the histogram_managet.* code > > into components/metrics and just leave Java wrapper code in > > cronet/android area. > > > > Alexei/Misha: wdyt? > > My preference is to keep this in the cronet component. But if it's really > infeasible to set up the tests here, I am ok with this being inside the metrics > component for now with a TODO to fix later. > > https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... > components/cronet/android/histogram_manager_unittest.cc:39: EXPECT_LT(0u, > data.size()); > Shouldn't this expect be before ParseFromArray()? Also, if you're just testing > that data is not empty, I'd just check .empty() is false. > > Also, please check the return value of ParseFromArray. Currently all Cronet unit tests are in fact Android Instrumentation tests that exercise native library through its Java wrappers. I guess ideally the test should verify content of UMA protobuffs on Java side, but I've no idea how to set it up.
Can the test have JNI? If so, it could just call C++ test code... On Thu, Dec 11, 2014 at 10:48 AM, <mef@chromium.org> wrote: > On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > > https://codereview.chromium.org/794493002/diff/40001/ > components/metrics/metrics_log.cc > >> File components/metrics/metrics_log.cc (right): >> > > > https://codereview.chromium.org/794493002/diff/40001/ > components/metrics/metrics_log.cc#newcode182 > >> components/metrics/metrics_log.cc:182: >> metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); >> On 2014/12/11 02:21:53, ramant wrote: >> > On 2014/12/10 22:36:37, Alexei Svitkine wrote: >> > > Nit: No need for metrics:: prefix here. >> > >> > Compiler prefers to use class method and complains about wrong >> arguments. >> Should >> > we rename the extern method? >> > > Yeah, let's rename it. How about EncodeHistogramDelta() for the standalone >> method? >> > > > https://codereview.chromium.org/794493002/diff/160001/ > components/cronet/android/histogram_manager_unittest.cc > >> File components/cronet/android/histogram_manager_unittest.cc (right): >> > > > https://codereview.chromium.org/794493002/diff/160001/ > components/cronet/android/histogram_manager_unittest.cc#newcode18 > >> components/cronet/android/histogram_manager_unittest.cc:18: class >> HistogramManagerTest : public testing::Test { >> If this class doesn't have anything in it, you can actually just remove it >> entirely and change TEST_F() to TEST() below. >> > > > https://codereview.chromium.org/794493002/diff/160001/ > components/cronet/android/histogram_manager_unittest.cc#newcode29 > >> components/cronet/android/histogram_manager_unittest.cc:29: using >> metrics::HistogramEventProto; >> Nit: I prefer not having using statements and just prefix these in the >> code. >> > > > https://codereview.chromium.org/794493002/diff/160001/ > components/cronet/android/histogram_manager_unittest.cc#newcode31 > >> components/cronet/android/histogram_manager_unittest.cc:31: >> TEST_F(HistogramManagerTest, HistogramBucketFields) { >> On 2014/12/11 02:21:53, ramant wrote: >> > Tested this code by moving all HistogramManager.* into >> components/metrics >> > area > >> > and running unit tests there. >> > >> > Wondering if we should most of the histogram_managet.* code >> > into components/metrics and just leave Java wrapper code in >> > cronet/android area. >> > >> > Alexei/Misha: wdyt? >> > > My preference is to keep this in the cronet component. But if it's really >> infeasible to set up the tests here, I am ok with this being inside the >> > metrics > >> component for now with a TODO to fix later. >> > > > https://codereview.chromium.org/794493002/diff/160001/ > components/cronet/android/histogram_manager_unittest.cc#newcode39 > >> components/cronet/android/histogram_manager_unittest.cc:39: EXPECT_LT(0u, >> data.size()); >> Shouldn't this expect be before ParseFromArray()? Also, if you're just >> testing >> that data is not empty, I'd just check .empty() is false. >> > > Also, please check the return value of ParseFromArray. >> > > Currently all Cronet unit tests are in fact Android Instrumentation tests > that > exercise native library through its Java wrappers. > I guess ideally the test should verify content of UMA protobuffs on Java > side, > but I've no idea how to set it up. > > https://codereview.chromium.org/794493002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alternatively, does Cronet code get built as part of the Chromium tree? If so, what about the new unit test to a components_unittests target in Chromium? On Thu, Dec 11, 2014 at 11:04 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Can the test have JNI? If so, it could just call C++ test code... > > On Thu, Dec 11, 2014 at 10:48 AM, <mef@chromium.org> wrote: > >> On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: >> >> https://codereview.chromium.org/794493002/diff/40001/ >> components/metrics/metrics_log.cc >> >>> File components/metrics/metrics_log.cc (right): >>> >> >> >> https://codereview.chromium.org/794493002/diff/40001/ >> components/metrics/metrics_log.cc#newcode182 >> >>> components/metrics/metrics_log.cc:182: >>> metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); >>> On 2014/12/11 02:21:53, ramant wrote: >>> > On 2014/12/10 22:36:37, Alexei Svitkine wrote: >>> > > Nit: No need for metrics:: prefix here. >>> > >>> > Compiler prefers to use class method and complains about wrong >>> arguments. >>> Should >>> > we rename the extern method? >>> >> >> Yeah, let's rename it. How about EncodeHistogramDelta() for the >>> standalone >>> method? >>> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> components/cronet/android/histogram_manager_unittest.cc >> >>> File components/cronet/android/histogram_manager_unittest.cc (right): >>> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> components/cronet/android/histogram_manager_unittest.cc#newcode18 >> >>> components/cronet/android/histogram_manager_unittest.cc:18: class >>> HistogramManagerTest : public testing::Test { >>> If this class doesn't have anything in it, you can actually just remove >>> it >>> entirely and change TEST_F() to TEST() below. >>> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> components/cronet/android/histogram_manager_unittest.cc#newcode29 >> >>> components/cronet/android/histogram_manager_unittest.cc:29: using >>> metrics::HistogramEventProto; >>> Nit: I prefer not having using statements and just prefix these in the >>> code. >>> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> components/cronet/android/histogram_manager_unittest.cc#newcode31 >> >>> components/cronet/android/histogram_manager_unittest.cc:31: >>> TEST_F(HistogramManagerTest, HistogramBucketFields) { >>> On 2014/12/11 02:21:53, ramant wrote: >>> > Tested this code by moving all HistogramManager.* into >>> components/metrics >>> >> area >> >>> > and running unit tests there. >>> > >>> > Wondering if we should most of the histogram_managet.* code >>> > into components/metrics and just leave Java wrapper code in >>> > cronet/android area. >>> > >>> > Alexei/Misha: wdyt? >>> >> >> My preference is to keep this in the cronet component. But if it's really >>> infeasible to set up the tests here, I am ok with this being inside the >>> >> metrics >> >>> component for now with a TODO to fix later. >>> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> components/cronet/android/histogram_manager_unittest.cc#newcode39 >> >>> components/cronet/android/histogram_manager_unittest.cc:39: >>> EXPECT_LT(0u, >>> data.size()); >>> Shouldn't this expect be before ParseFromArray()? Also, if you're just >>> testing >>> that data is not empty, I'd just check .empty() is false. >>> >> >> Also, please check the return value of ParseFromArray. >>> >> >> Currently all Cronet unit tests are in fact Android Instrumentation tests >> that >> exercise native library through its Java wrappers. >> I guess ideally the test should verify content of UMA protobuffs on Java >> side, >> but I've no idea how to set it up. >> >> https://codereview.chromium.org/794493002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/11 16:06:16, asvitkine (OOO Dec13 - Jan4) wrote: > Alternatively, does Cronet code get built as part of the Chromium tree? Yes, Cronet does get built as part of the Chromium tree. > If so, what about the new unit test to a components_unittests target in > Chromium? Um, could you elaborate? This CL makes components/cronet depend on components/metrics. Are you suggesting to add cronet-specific unit tests to components_unittests? > > On Thu, Dec 11, 2014 at 11:04 AM, Alexei Svitkine <mailto:asvitkine@chromium.org> > wrote: > > > Can the test have JNI? If so, it could just call C++ test code... Yes, we can add custom C++ code to test library and call it via JNI. > > > > On Thu, Dec 11, 2014 at 10:48 AM, <mailto:mef@chromium.org> wrote: > > > >> On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > >> > >> https://codereview.chromium.org/794493002/diff/40001/ > >> components/metrics/metrics_log.cc > >> > >>> File components/metrics/metrics_log.cc (right): > >>> > >> > >> > >> https://codereview.chromium.org/794493002/diff/40001/ > >> components/metrics/metrics_log.cc#newcode182 > >> > >>> components/metrics/metrics_log.cc:182: > >>> metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); > >>> On 2014/12/11 02:21:53, ramant wrote: > >>> > On 2014/12/10 22:36:37, Alexei Svitkine wrote: > >>> > > Nit: No need for metrics:: prefix here. > >>> > > >>> > Compiler prefers to use class method and complains about wrong > >>> arguments. > >>> Should > >>> > we rename the extern method? > >>> > >> > >> Yeah, let's rename it. How about EncodeHistogramDelta() for the > >>> standalone > >>> method? > >>> > >> > >> > >> https://codereview.chromium.org/794493002/diff/160001/ > >> components/cronet/android/histogram_manager_unittest.cc > >> > >>> File components/cronet/android/histogram_manager_unittest.cc (right): > >>> > >> > >> > >> https://codereview.chromium.org/794493002/diff/160001/ > >> components/cronet/android/histogram_manager_unittest.cc#newcode18 > >> > >>> components/cronet/android/histogram_manager_unittest.cc:18: class > >>> HistogramManagerTest : public testing::Test { > >>> If this class doesn't have anything in it, you can actually just remove > >>> it > >>> entirely and change TEST_F() to TEST() below. > >>> > >> > >> > >> https://codereview.chromium.org/794493002/diff/160001/ > >> components/cronet/android/histogram_manager_unittest.cc#newcode29 > >> > >>> components/cronet/android/histogram_manager_unittest.cc:29: using > >>> metrics::HistogramEventProto; > >>> Nit: I prefer not having using statements and just prefix these in the > >>> code. > >>> > >> > >> > >> https://codereview.chromium.org/794493002/diff/160001/ > >> components/cronet/android/histogram_manager_unittest.cc#newcode31 > >> > >>> components/cronet/android/histogram_manager_unittest.cc:31: > >>> TEST_F(HistogramManagerTest, HistogramBucketFields) { > >>> On 2014/12/11 02:21:53, ramant wrote: > >>> > Tested this code by moving all HistogramManager.* into > >>> components/metrics > >>> > >> area > >> > >>> > and running unit tests there. > >>> > > >>> > Wondering if we should most of the histogram_managet.* code > >>> > into components/metrics and just leave Java wrapper code in > >>> > cronet/android area. > >>> > > >>> > Alexei/Misha: wdyt? > >>> > >> > >> My preference is to keep this in the cronet component. But if it's really > >>> infeasible to set up the tests here, I am ok with this being inside the > >>> > >> metrics > >> > >>> component for now with a TODO to fix later. > >>> > >> > >> > >> https://codereview.chromium.org/794493002/diff/160001/ > >> components/cronet/android/histogram_manager_unittest.cc#newcode39 > >> > >>> components/cronet/android/histogram_manager_unittest.cc:39: > >>> EXPECT_LT(0u, > >>> data.size()); > >>> Shouldn't this expect be before ParseFromArray()? Also, if you're just > >>> testing > >>> that data is not empty, I'd just check .empty() is false. > >>> > >> > >> Also, please check the return value of ParseFromArray. > >>> > >> > >> Currently all Cronet unit tests are in fact Android Instrumentation tests > >> that > >> exercise native library through its Java wrappers. > >> I guess ideally the test should verify content of UMA protobuffs on Java > >> side, > >> but I've no idea how to set it up. > >> > >> https://codereview.chromium.org/794493002/ > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Thu, Dec 11, 2014 at 11:19 AM, <mef@chromium.org> wrote: > On 2014/12/11 16:06:16, asvitkine (OOO Dec13 - Jan4) wrote: > >> Alternatively, does Cronet code get built as part of the Chromium tree? >> > Yes, Cronet does get built as part of the Chromium tree. > > If so, what about the new unit test to a components_unittests target in >> Chromium? >> > > Um, could you elaborate? This CL makes components/cronet depend on > components/metrics. > Are you suggesting to add cronet-specific unit tests to > components_unittests? Yeah - same way that components/metrics unit tests are part of components_unittests, the same can be done for components/cronet C++ unit tests, no? > > > > On Thu, Dec 11, 2014 at 11:04 AM, Alexei Svitkine >> > <mailto:asvitkine@chromium.org> > >> wrote: >> > > > Can the test have JNI? If so, it could just call C++ test code... >> > Yes, we can add custom C++ code to test library and call it via JNI. > > > > > >> > On Thu, Dec 11, 2014 at 10:48 AM, <mailto:mef@chromium.org> wrote: >> > >> >> On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: >> >> >> >> https://codereview.chromium.org/794493002/diff/40001/ >> >> components/metrics/metrics_log.cc >> >> >> >>> File components/metrics/metrics_log.cc (right): >> >>> >> >> >> >> >> >> https://codereview.chromium.org/794493002/diff/40001/ >> >> components/metrics/metrics_log.cc#newcode182 >> >> >> >>> components/metrics/metrics_log.cc:182: >> >>> metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); >> >>> On 2014/12/11 02:21:53, ramant wrote: >> >>> > On 2014/12/10 22:36:37, Alexei Svitkine wrote: >> >>> > > Nit: No need for metrics:: prefix here. >> >>> > >> >>> > Compiler prefers to use class method and complains about wrong >> >>> arguments. >> >>> Should >> >>> > we rename the extern method? >> >>> >> >> >> >> Yeah, let's rename it. How about EncodeHistogramDelta() for the >> >>> standalone >> >>> method? >> >>> >> >> >> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> >> components/cronet/android/histogram_manager_unittest.cc >> >> >> >>> File components/cronet/android/histogram_manager_unittest.cc (right): >> >>> >> >> >> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> >> components/cronet/android/histogram_manager_unittest.cc#newcode18 >> >> >> >>> components/cronet/android/histogram_manager_unittest.cc:18: class >> >>> HistogramManagerTest : public testing::Test { >> >>> If this class doesn't have anything in it, you can actually just >> remove >> >>> it >> >>> entirely and change TEST_F() to TEST() below. >> >>> >> >> >> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> >> components/cronet/android/histogram_manager_unittest.cc#newcode29 >> >> >> >>> components/cronet/android/histogram_manager_unittest.cc:29: using >> >>> metrics::HistogramEventProto; >> >>> Nit: I prefer not having using statements and just prefix these in the >> >>> code. >> >>> >> >> >> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> >> components/cronet/android/histogram_manager_unittest.cc#newcode31 >> >> >> >>> components/cronet/android/histogram_manager_unittest.cc:31: >> >>> TEST_F(HistogramManagerTest, HistogramBucketFields) { >> >>> On 2014/12/11 02:21:53, ramant wrote: >> >>> > Tested this code by moving all HistogramManager.* into >> >>> components/metrics >> >>> >> >> area >> >> >> >>> > and running unit tests there. >> >>> > >> >>> > Wondering if we should most of the histogram_managet.* code >> >>> > into components/metrics and just leave Java wrapper code in >> >>> > cronet/android area. >> >>> > >> >>> > Alexei/Misha: wdyt? >> >>> >> >> >> >> My preference is to keep this in the cronet component. But if it's >> really >> >>> infeasible to set up the tests here, I am ok with this being inside >> the >> >>> >> >> metrics >> >> >> >>> component for now with a TODO to fix later. >> >>> >> >> >> >> >> >> https://codereview.chromium.org/794493002/diff/160001/ >> >> components/cronet/android/histogram_manager_unittest.cc#newcode39 >> >> >> >>> components/cronet/android/histogram_manager_unittest.cc:39: >> >>> EXPECT_LT(0u, >> >>> data.size()); >> >>> Shouldn't this expect be before ParseFromArray()? Also, if you're just >> >>> testing >> >>> that data is not empty, I'd just check .empty() is false. >> >>> >> >> >> >> Also, please check the return value of ParseFromArray. >> >>> >> >> >> >> Currently all Cronet unit tests are in fact Android Instrumentation >> tests >> >> that >> >> exercise native library through its Java wrappers. >> >> I guess ideally the test should verify content of UMA protobuffs on >> Java >> >> side, >> >> but I've no idea how to set it up. >> >> >> >> https://codereview.chromium.org/794493002/ >> >> >> > >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/794493002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can you set a BUG= for this? https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... components/cronet/android/histogram_manager.cc:22: HistogramManager::HistogramManager() { Remove? https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... components/cronet/android/histogram_manager.h:21: class HistogramManager { Remove? Since its in the metrics component now? https://codereview.chromium.org/794493002/diff/180001/components/metrics/hist... File components/metrics/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/180001/components/metrics/hist... components/metrics/histogram_manager.h:39: // HistogramFlattener methods: Nit: This comment syntax is preferred instead: // base::HistogramFlattener: https://codereview.chromium.org/794493002/diff/180001/components/metrics/hist... components/metrics/histogram_manager.h:48: void RecordHistogramDelta(const std::string& histogram_name, This can be removed.
Patchset #6 (id:200001) has been deleted
Hi Alexei, I am running blind with cronet changes. I haven't tested them (working with mef to test them). Thanks for your early comments. https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/794493002/diff/40001/components/metrics/metri... components/metrics/metrics_log.cc:182: metrics::RecordHistogramDelta(histogram_name, snapshot, &uma_proto_); On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > On 2014/12/11 02:21:53, ramant wrote: > > On 2014/12/10 22:36:37, Alexei Svitkine wrote: > > > Nit: No need for metrics:: prefix here. > > > > Compiler prefers to use class method and complains about wrong arguments. > Should > > we rename the extern method? > > Yeah, let's rename it. How about EncodeHistogramDelta() for the standalone > method? Done. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... File components/cronet/android/histogram_manager_unittest.cc (right): https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:18: class HistogramManagerTest : public testing::Test { On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > If this class doesn't have anything in it, you can actually just remove it > entirely and change TEST_F() to TEST() below. Done. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:29: using metrics::HistogramEventProto; On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > Nit: I prefer not having using statements and just prefix these in the code. Done. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:31: TEST_F(HistogramManagerTest, HistogramBucketFields) { On 2014/12/11 15:35:40, mef wrote: > On 2014/12/11 02:21:53, ramant wrote: > > Tested this code by moving all HistogramManager.* into components/metrics area > > and running unit tests there. > > > > Wondering if we should most of the histogram_managet.* code > > into components/metrics and just leave Java wrapper code in > > cronet/android area. > > > > Alexei/Misha: wdyt? > > sgtm. I like making Java wrapper as thin as possible. Acknowledged. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:31: TEST_F(HistogramManagerTest, HistogramBucketFields) { On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > On 2014/12/11 02:21:53, ramant wrote: > > Tested this code by moving all HistogramManager.* into components/metrics area > > and running unit tests there. > > > > Wondering if we should most of the histogram_managet.* code > > into components/metrics and just leave Java wrapper code in > > cronet/android area. > > > > Alexei/Misha: wdyt? > > My preference is to keep this in the cronet component. But if it's really > infeasible to set up the tests here, I am ok with this being inside the metrics > component for now with a TODO to fix later. Done. https://codereview.chromium.org/794493002/diff/160001/components/cronet/andro... components/cronet/android/histogram_manager_unittest.cc:39: EXPECT_LT(0u, data.size()); On 2014/12/11 15:38:58, asvitkine (OOO Dec13 - Jan4) wrote: > Shouldn't this expect be before ParseFromArray()? Also, if you're just testing > that data is not empty, I'd just check .empty() is false. > > Also, please check the return value of ParseFromArray. Done. https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... components/cronet/android/histogram_manager.cc:22: HistogramManager::HistogramManager() { On 2014/12/11 20:09:09, asvitkine (OOO Dec13 - Jan4) wrote: > Remove? Done. https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... File components/cronet/android/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/180001/components/cronet/andro... components/cronet/android/histogram_manager.h:21: class HistogramManager { On 2014/12/11 20:09:09, asvitkine (OOO Dec13 - Jan4) wrote: > Remove? Since its in the metrics component now? Haven't tested this code on cronet. Added GetHistogramDeltas function prototype. https://codereview.chromium.org/794493002/diff/180001/components/metrics/hist... File components/metrics/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/180001/components/metrics/hist... components/metrics/histogram_manager.h:39: // HistogramFlattener methods: On 2014/12/11 20:09:09, asvitkine (OOO Dec13 - Jan4) wrote: > Nit: This comment syntax is preferred instead: > > // base::HistogramFlattener: Done. https://codereview.chromium.org/794493002/diff/180001/components/metrics/hist... components/metrics/histogram_manager.h:48: void RecordHistogramDelta(const std::string& histogram_name, On 2014/12/11 20:09:09, asvitkine (OOO Dec13 - Jan4) wrote: > This can be removed. Done.
LGTM % comment below Also, please associate this CL with a crbug.com via BUG= https://codereview.chromium.org/794493002/diff/220001/components/metrics/hist... File components/metrics/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/220001/components/metrics/hist... components/metrics/histogram_manager.h:24: class HistogramManager : public base::HistogramFlattener { Please add a TODO() here to move it to components/cronet and file a crbug to address the TODO.
Patchset #7 (id:240001) has been deleted
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
The CQ bit was checked by rtenneti@chromium.org
https://codereview.chromium.org/794493002/diff/220001/components/metrics/hist... File components/metrics/histogram_manager.h (right): https://codereview.chromium.org/794493002/diff/220001/components/metrics/hist... components/metrics/histogram_manager.h:24: class HistogramManager : public base::HistogramFlattener { On 2014/12/11 20:29:28, asvitkine (OOO Dec13 - Jan4) wrote: > Please add a TODO() here to move it to components/cronet and file a crbug to > address the TODO. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/300001
Mostly nits and questions. I assume that components_unittests succeed. Do I understand correctly that there is no need for explicit initialization of HistogramManager? https://codereview.chromium.org/794493002/diff/300001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/794493002/diff/300001/components/components_t... components/components_tests.gyp:160: 'metrics/histogram_encoder_unittest.cc', is there corresponding gn file? https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... components/cronet/android/histogram_manager.cc:24: metrics::HistogramManager::GetInstance()->GetDeltas(); qq: how big are deltas? Would this result in extra copy of the data? https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... components/cronet/android/histogram_manager.cc:26: return base::android::ToJavaByteArray(env, &data[0], data.size()).Release(); nit: no need for curly braces. https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java (right): https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java:11: nit: extra nl. https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... File components/metrics/histogram_encoder.cc (right): https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... components/metrics/histogram_encoder.cc:22: DCHECK_NE(0, snapshot.TotalCount()); DCHECK that uma_proto is not null? https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... File components/metrics/histogram_encoder.h (right): https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... components/metrics/histogram_encoder.h:22: // Record any changes in a given histogram for transmission. maybe better comment? https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... File components/metrics/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... components/metrics/histogram_manager.cc:50: std::vector<uint8> HistogramManager::GetDeltas() { should this be HistogramManager::GetDeltas(std::vector<uint8>* data) instead?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PTAL. https://codereview.chromium.org/794493002/diff/300001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/794493002/diff/300001/components/components_t... components/components_tests.gyp:160: 'metrics/histogram_encoder_unittest.cc', On 2014/12/11 23:11:40, mef wrote: > is there corresponding gn file? Added the unit tests to BUILD.gn also. https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... File components/cronet/android/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... components/cronet/android/histogram_manager.cc:24: metrics::HistogramManager::GetInstance()->GetDeltas(); On 2014/12/11 23:11:40, mef wrote: > qq: how big are deltas? Would this result in extra copy of the data? Do we do return value optimization on Android? Assumed, in C++11, we don't support RVO and changed GetDeltas to accept std::vector* as argument. http://en.cppreference.com/w/cpp/language/copy_elision http://en.wikipedia.org/wiki/Return_value_optimization. https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... components/cronet/android/histogram_manager.cc:26: return base::android::ToJavaByteArray(env, &data[0], data.size()).Release(); On 2014/12/11 23:11:40, mef wrote: > nit: no need for curly braces. Done. https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java (right): https://codereview.chromium.org/794493002/diff/300001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java:11: On 2014/12/11 23:11:41, mef wrote: > nit: extra nl. Done. https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... File components/metrics/histogram_encoder.cc (right): https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... components/metrics/histogram_encoder.cc:22: DCHECK_NE(0, snapshot.TotalCount()); On 2014/12/11 23:11:41, mef wrote: > DCHECK that uma_proto is not null? Done. https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... File components/metrics/histogram_encoder.h (right): https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... components/metrics/histogram_encoder.h:22: // Record any changes in a given histogram for transmission. On 2014/12/11 23:11:41, mef wrote: > maybe better comment? Done. https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... File components/metrics/histogram_manager.cc (right): https://codereview.chromium.org/794493002/diff/300001/components/metrics/hist... components/metrics/histogram_manager.cc:50: std::vector<uint8> HistogramManager::GetDeltas() { On 2014/12/11 23:11:41, mef wrote: > should this be HistogramManager::GetDeltas(std::vector<uint8>* data) instead? Done.
On 2014/12/11 23:11:41, mef wrote: > Mostly nits and questions. I assume that components_unittests succeed. > > Do I understand correctly that there is no need for explicit initialization of > HistogramManager? > HistogramManager::GetInstance() creates a Singleton. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/single... HistogramManager is a HistogramFlattener and it has HistogramSnapshotManager. We clear protobuf whenever we call GetDeltas(). HistogramSnapshotManager::logged_samples_ has the samples that is has already recorded and uses that to record only deltas. HistogramSnapshotManager::PrepareDeltas() takes care of snapshotting (using logged_samples_) and calls RecordDelta with the histogram deltas. RecordDelta fills the protobuf with histogram deltas. GetDeltas serializes protobuf returns it as a std::vector.
On 2014/12/12 03:27:54, ramant wrote: > On 2014/12/11 23:11:41, mef wrote: > > Mostly nits and questions. I assume that components_unittests succeed. Yes (except ios_dbg_simulator. On that platform, the failure is similar to issue crbug.com/435320).
lgtm, thanks a lot for doing this!
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/320001
I have a suspicion about the test failures. I think the specific test is flaky and can fail if another test executed first. And since this is tweaking tests, it might be changing the order that the tests end up executing on the bots. I'll try to look at this later today, but in the meantime feel free to disable the test on iOS for now and point at the drmemory bug.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
https://codereview.chromium.org/794493002/diff/320001/components/metrics/hist... File components/metrics/histogram_manager_unittest.cc (right): https://codereview.chromium.org/794493002/diff/320001/components/metrics/hist... components/metrics/histogram_manager_unittest.cc:22: EXPECT_LT(0u, data.size()); I thought I mention this in a previous comment, but I think this should be EXPECT_FALSE(data.empty()); https://codereview.chromium.org/794493002/diff/320001/components/metrics/hist... components/metrics/histogram_manager_unittest.cc:30: ASSERT_EQ(1, histogram_proto.bucket_size()); I got a test failure here: [----------] 1 test from HistogramManager [ RUN ] HistogramManager.HistogramBucketFields ../../components/metrics/histogram_manager_unittest.cc:30: Failure Value of: histogram_proto.bucket_size() Actual: 6 Expected: 1 [ FAILED ] HistogramManager.HistogramBucketFields (4 ms) [----------] 1 test from HistogramManager (4 ms total) When running with the following command-line locally: ./out/Debug/components_unittests --single-process-tests I think this is flaky because it may pick up other histograms that were logged before the test has run. I suggest adding an initial GetDeltas() call to capture those at the start of the test, so later GetDeltas() calls won't pick them up.
https://codereview.chromium.org/794493002/diff/320001/components/metrics/hist... File components/metrics/histogram_manager_unittest.cc (right): https://codereview.chromium.org/794493002/diff/320001/components/metrics/hist... components/metrics/histogram_manager_unittest.cc:22: EXPECT_LT(0u, data.size()); On 2014/12/12 16:57:00, asvitkine (OOO until Jan4) wrote: > I thought I mention this in a previous comment, but I think this should be > > EXPECT_FALSE(data.empty()); Sorry missed the above change. Done. https://codereview.chromium.org/794493002/diff/320001/components/metrics/hist... components/metrics/histogram_manager_unittest.cc:30: ASSERT_EQ(1, histogram_proto.bucket_size()); On 2014/12/12 16:57:00, asvitkine (OOO until Jan4) wrote: > I got a test failure here: > > > [----------] 1 test from HistogramManager > [ RUN ] HistogramManager.HistogramBucketFields > ../../components/metrics/histogram_manager_unittest.cc:30: Failure > Value of: histogram_proto.bucket_size() > Actual: 6 > Expected: 1 > [ FAILED ] HistogramManager.HistogramBucketFields (4 ms) > [----------] 1 test from HistogramManager (4 ms total) > > > > When running with the following command-line locally: > > ./out/Debug/components_unittests --single-process-tests > > I think this is flaky because it may pick up other histograms that were logged > before the test has run. I suggest adding an initial GetDeltas() call to capture > those at the start of the test, so later GetDeltas() calls won't pick them up. Awesome. Many many thanks for the above idea. Removes the potential flakiness. Done.
The CQ bit was checked by rtenneti@chromium.org
LGTM!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/360001
On 2014/12/12 19:30:01, asvitkine (OOO until Jan4) wrote: > LGTM! Many many thanks Alexei and Misha. This CL is more of team effort. Wishing you both happy holidays.
On 2014/12/12 19:30:50, ramant wrote: > On 2014/12/12 19:30:01, asvitkine (OOO until Jan4) wrote: > > LGTM! > > Many many thanks Alexei and Misha. This CL is more of team effort. Wishing you > both happy holidays. Thank you, you too!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
still lgtm
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/400001
Message was sent while issue was closed.
Committed patchset #12 (id:400001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/4eb0fd47de0736f3f4c970e8d7f8813ef2177886 Cr-Commit-Position: refs/heads/master@{#308239}
Message was sent while issue was closed.
andresantoso@chromium.org changed reviewers: + andresantoso@chromium.org
Message was sent while issue was closed.
HistogramManagerBucketFields times out on Win7 Tests. https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2... Reverting, https://codereview.chromium.org/800363002.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794493002/420001
Message was sent while issue was closed.
Committed patchset #13 (id:420001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/7254696a81e31cc34a509a692c145690c4e58755 Cr-Commit-Position: refs/heads/master@{#308427} |
