Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1273303002:
Measuring data use of different ModelTypes in Sync Service. (Closed)
|
Created:
5 years, 4 months ago by amohammadkhan Modified:
5 years, 3 months ago CC:
chromium-reviews, tim+watch_chromium.org, cbentzel+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, asvitkine+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@NewServices Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMeasuring data use of different ModelTypes in Sync Service.
To understand the contribution of each ModelType to total traffic of
Sync service, SyncEntries of different ModelTypes in update and commit
messages are counteda and sparse UMA histograms are made to record them.
ModelTypes are buckets in these histograms. Beside the aforementioned
information, a histogram records the amount of byte useage of
ProgressMarkers in update messages.
BUG=516455
Committed: https://crrev.com/30c09859c0ec4afbc12b09fc6adda4312afdbd65
Cr-Commit-Position: refs/heads/master@{#345461}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Fixing formatting problems. #Patch Set 3 : Using SyncModelTypes of histograms.xml #Patch Set 4 : Updating previous message size in commit #
Total comments: 4
Patch Set 5 : Removing unnecessary dependencies. #
Total comments: 7
Patch Set 6 : Adding tests and updating owners #Patch Set 7 : Changing the name of histogramTester. #
Total comments: 4
Patch Set 8 : Adding bucket specific tests. #Patch Set 9 : Fix formatting issues caused by Eclipse autoformatting. #Patch Set 10 : Removing extra spaces #
Total comments: 2
Patch Set 11 : Moving the Macro to data_type_histogram.h #Patch Set 12 : Updating some of the comments #
Total comments: 8
Patch Set 13 : Changing test function to not be dependent on specific values. #Patch Set 14 : Fixing spacing in test function. #Patch Set 15 : Updating the recording macro. #
Total comments: 16
Patch Set 16 : Changing the recording macro to a recording function. #
Total comments: 16
Patch Set 17 : Addressing reviewers' comments. #
Total comments: 4
Patch Set 18 : Addresses reviewer's comments. #
Total comments: 2
Patch Set 19 : Rebased #Patch Set 20 : Adding data_type_histogram.cc to BUILD.gn #Patch Set 21 : Changing the initialization of vectors in syncer_unittest.cc #
Depends on Patchset: Messages
Total messages: 46 (6 generated)
amohammadkhan@google.com changed reviewers: + bengr@chromium.org
zea@chromium.org changed reviewers: + zea@chromium.org
https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5240: + Number of downloaded bytes of different data types in Sync service for What's the time window for each of these histograms? https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5253: +<histogram name="DataUse.Sync.ProgressMarker.Bytes" I wouldn't expect the progress marker to be particular interesting. It's part of each request, along with some other overhead we use to identify and enable the request, but it doesn't grow in size. https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:53589: +<enum name="DataUseSyncServiceType" type="int"> There already exists a sync model type enum. We should reuse it
https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5240: + Number of downloaded bytes of different data types in Sync service for On 2015/08/06 23:22:24, Nicolas Zea wrote: > What's the time window for each of these histograms? They are updated with each update or commit for ModelTypes including in them. https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5253: +<histogram name="DataUse.Sync.ProgressMarker.Bytes" On 2015/08/06 23:22:24, Nicolas Zea wrote: > I wouldn't expect the progress marker to be particular interesting. It's part of > each request, along with some other overhead we use to identify and enable the > request, but it doesn't grow in size. I was looking for a way to record request for types that doesn't lead to a update and "no update" is the response. Do you have any suggestion? and basically do you think is it beneficial to know it? https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:53589: +<enum name="DataUseSyncServiceType" type="int"> On 2015/08/06 23:22:24, Nicolas Zea wrote: > There already exists a sync model type enum. We should reuse it It seems that one doesn't match with the definition in the ModelType.h and it is not updated.
https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.cc (right): https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:138: void SparseDataUseReport(const std::string& histogram_name, Add a blank line above. https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:144: LOG(ERROR) << "Ali1 zero or negative count " << count; What is Ali1? https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.h (right): https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.h:20: void SparseDataUseReport(const std::string& histogram_name, #include <string> https://codereview.chromium.org/1273303002/diff/1/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/1/sync/engine/commit.cc#newco... sync/engine/commit.cc:87: int new_message_part_size = message.ByteSize()-previous_message_size; I have no idea what new_message_part_size means. Use a better name. https://codereview.chromium.org/1273303002/diff/1/sync/engine/commit.cc#newco... sync/engine/commit.cc:88: if (new_message_part_size > 0) Add curly braces. https://codereview.chromium.org/1273303002/diff/1/sync/engine/directory_updat... File sync/engine/directory_update_handler.cc (right): https://codereview.chromium.org/1273303002/diff/1/sync/engine/directory_updat... sync/engine/directory_update_handler.cc:57: if (progress_marker.ByteSize() > 0) Add curly braces. https://codereview.chromium.org/1273303002/diff/1/sync/engine/process_updates... File sync/engine/process_updates_util.cc (right): https://codereview.chromium.org/1273303002/diff/1/sync/engine/process_updates... sync/engine/process_updates_util.cc:314: if ((*update_it)->ByteSize() > 0) Add curly braces.
https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.cc (right): https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:138: void SparseDataUseReport(const std::string& histogram_name, On 2015/08/07 17:13:37, bengr wrote: > Add a blank line above. Done. https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:144: LOG(ERROR) << "Ali1 zero or negative count " << count; On 2015/08/07 17:13:37, bengr wrote: > What is Ali1? Sorry, basically this LOG(ERROR) was for testing to see if we have any negative or zero values and it should be removed. But beside that I have a question. Because at first I changed Data_use_measurement to add a function for measuring Sync data use. But I had to include it (net/url_request/data_use_measurement.h) in Sync files that I wasn't sure it is Okay or not. So I added my Macro to those files and didn't use function of data_use_measurement. So I wanted to ask you which approach is better and remove the code for the other one. Because now I have the code for both of approaches (The added function in data_use_measurement and the Macro in Sync files) https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.h (right): https://codereview.chromium.org/1273303002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.h:20: void SparseDataUseReport(const std::string& histogram_name, On 2015/08/07 17:13:37, bengr wrote: > #include <string> I'm gonna add necessary includes to the CL which this one is depend on. (Because the same problem exists there) https://codereview.chromium.org/1273303002/diff/1/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/1/sync/engine/commit.cc#newco... sync/engine/commit.cc:87: int new_message_part_size = message.ByteSize()-previous_message_size; On 2015/08/07 17:13:37, bengr wrote: > I have no idea what new_message_part_size means. Use a better name. Done. https://codereview.chromium.org/1273303002/diff/1/sync/engine/commit.cc#newco... sync/engine/commit.cc:88: if (new_message_part_size > 0) On 2015/08/07 17:13:37, bengr wrote: > Add curly braces. Done. https://codereview.chromium.org/1273303002/diff/1/sync/engine/directory_updat... File sync/engine/directory_update_handler.cc (right): https://codereview.chromium.org/1273303002/diff/1/sync/engine/directory_updat... sync/engine/directory_update_handler.cc:57: if (progress_marker.ByteSize() > 0) On 2015/08/07 17:13:37, bengr wrote: > Add curly braces. Done. https://codereview.chromium.org/1273303002/diff/1/sync/engine/process_updates... File sync/engine/process_updates_util.cc (right): https://codereview.chromium.org/1273303002/diff/1/sync/engine/process_updates... sync/engine/process_updates_util.cc:314: if ((*update_it)->ByteSize() > 0) On 2015/08/07 17:13:37, bengr wrote: > Add curly braces. Done.
https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5253: +<histogram name="DataUse.Sync.ProgressMarker.Bytes" On 2015/08/07 00:17:12, amohammadkhan wrote: > On 2015/08/06 23:22:24, Nicolas Zea wrote: > > I wouldn't expect the progress marker to be particular interesting. It's part > of > > each request, along with some other overhead we use to identify and enable the > > request, but it doesn't grow in size. > > I was looking for a way to record request for types that doesn't lead to a > update and "no update" is the response. Do you have any suggestion? and > basically do you think is it beneficial to know it? I'm not sure what you mean by "update and no update". Could you explain a bit more what you're trying to do? But at a high level I don't think that the progress marker itself is particularly interesting no. https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc#n... sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ Can these macros be pulled into a header file so they're not duplicated (AFAICT they're the same in the various files?) https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc#n... sync/engine/commit.cc:94: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Bytes", Why increment by 1 here?
https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5253: +<histogram name="DataUse.Sync.ProgressMarker.Bytes" On 2015/08/07 22:57:36, Nicolas Zea (OOO until Aug 17) wrote: > On 2015/08/07 00:17:12, amohammadkhan wrote: > > On 2015/08/06 23:22:24, Nicolas Zea wrote: > > > I wouldn't expect the progress marker to be particular interesting. It's > part > > of > > > each request, along with some other overhead we use to identify and enable > the > > > request, but it doesn't grow in size. > > > > I was looking for a way to record request for types that doesn't lead to a > > update and "no update" is the response. Do you have any suggestion? and > > basically do you think is it beneficial to know it? > > I'm not sure what you mean by "update and no update". Could you explain a bit > more what you're trying to do? But at a high level I don't think that the > progress marker itself is particularly interesting no. If I have understood it right, sometimes Sync service on a client requests for update, but no update exists. So no update is received and nothing is reflected in our other histograms. But the update request and received response are not small(They are at least few KBs) and I thought the main contributors to this size are progressMakers so I thought it may be a good idea to have statistics about them too. https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc#n... sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ On 2015/08/07 22:57:36, Nicolas Zea (OOO until Aug 17) wrote: > Can these macros be pulled into a header file so they're not duplicated (AFAICT > they're the same in the various files?) Yes you are right. I want to ask UMA team to add to sparseHistogram header file, if it is possible. If they do not agree, can I put them somewhere in sync? https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc#n... sync/engine/commit.cc:94: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Bytes", On 2015/08/07 22:57:36, Nicolas Zea (OOO until Aug 17) wrote: > Why increment by 1 here? You are right. It is incorrect. The histogram name should change to DataUse.Sync.Upload.Count.
On 2015/08/08 00:50:19, amohammadkhan wrote: > https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1273303002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:5253: +<histogram > name="DataUse.Sync.ProgressMarker.Bytes" > On 2015/08/07 22:57:36, Nicolas Zea (OOO until Aug 17) wrote: > > On 2015/08/07 00:17:12, amohammadkhan wrote: > > > On 2015/08/06 23:22:24, Nicolas Zea wrote: > > > > I wouldn't expect the progress marker to be particular interesting. It's > > part > > > of > > > > each request, along with some other overhead we use to identify and enable > > the > > > > request, but it doesn't grow in size. > > > > > > I was looking for a way to record request for types that doesn't lead to a > > > update and "no update" is the response. Do you have any suggestion? and > > > basically do you think is it beneficial to know it? > > > > I'm not sure what you mean by "update and no update". Could you explain a bit > > more what you're trying to do? But at a high level I don't think that the > > progress marker itself is particularly interesting no. > > If I have understood it right, sometimes Sync service on a client requests for > update, but no update exists. So no update is received and nothing is reflected > in our other histograms. > But the update request and received response are not small(They are at least few > KBs) and I thought the main contributors to this size are progressMakers so I > thought it may be a good idea to have statistics about them too. > > https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc > File sync/engine/commit.cc (right): > > https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc#n... > sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, > sample, value) \ > On 2015/08/07 22:57:36, Nicolas Zea (OOO until Aug 17) wrote: > > Can these macros be pulled into a header file so they're not duplicated > (AFAICT > > they're the same in the various files?) > > Yes you are right. I want to ask UMA team to add to sparseHistogram header file, > if it is possible. If they do not agree, can I put them somewhere in sync? > > https://codereview.chromium.org/1273303002/diff/60001/sync/engine/commit.cc#n... > sync/engine/commit.cc:94: > UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Bytes", > On 2015/08/07 22:57:36, Nicolas Zea (OOO until Aug 17) wrote: > > Why increment by 1 here? > > You are right. It is incorrect. The histogram name should change to > DataUse.Sync.Upload.Count. UMA team didn't agree with putting the Macro in their header. Any suggestion for a good place to put it? Can I make a file in sync/engine/ and put this Macro in it? And do you think is it better to keep it as Macro or having it as a function is better?
amohammadkhan@google.com changed reviewers: + sclittle@chromium.org
https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc#n... sync/engine/commit.cc:90: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Bytes", Could you test somewhere that these histograms are recorded properly in unittests? Maybe in sync/engine/syncer_unittest.cc. HistogramTester might be useful: https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc#n... sync/engine/commit.cc:94: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Count", If you're just incrementing by one, could you just use the normal UMA_HISTOGRAM_SPARSE_SLOWLY? https://codereview.chromium.org/1273303002/diff/80001/sync/engine/directory_u... File sync/engine/directory_update_handler.cc (right): https://codereview.chromium.org/1273303002/diff/80001/sync/engine/directory_u... sync/engine/directory_update_handler.cc:17: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ This code is duplicated, could this be put in sparse_histogram.h or some other common location? Are you planning to write future code that will also use this call? https://codereview.chromium.org/1273303002/diff/80001/sync/engine/process_upd... File sync/engine/process_updates_util.cc (right): https://codereview.chromium.org/1273303002/diff/80001/sync/engine/process_upd... sync/engine/process_updates_util.cc:319: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Download.Count", Just use the regular UMA_HISTOGRAM_SPARSE_SLOWLY for incrementing by 1 here.
Ali, if you upload an incomplete CL (e.g., this one because it has no tests), please add a note to the CL to inform reviewers of what's to come. Also please let reviewers know what you'd like reviewed. For example, if you are wondering about the general architecture that you've chosen, you could ask reviewers to look specifically at that.
On 2015/08/11 16:55:59, bengr wrote: > Ali, if you upload an incomplete CL (e.g., this one because it has no tests), > please add a note to the CL to inform reviewers of what's to come. Also please > let reviewers know what you'd like reviewed. For example, if you are wondering > about the general architecture that you've chosen, you could ask reviewers to > look specifically at that. Sure. For now my main purpose was to assure that the architecture is Okay and I have selected correct measuring points for commits and updates. Also I wanted to provide code beside my design doc to make it easier for its reviewer to give feedback to me. It seems the architecture is Okay, so I'll go on to polish the code and providing the tests.
https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc#n... sync/engine/commit.cc:90: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Bytes", On 2015/08/10 23:39:39, sclittle wrote: > Could you test somewhere that these histograms are recorded properly in > unittests? Maybe in sync/engine/syncer_unittest.cc. > > HistogramTester might be useful: > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... Thank you for your helpful suggestions. Done. https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc#n... sync/engine/commit.cc:94: UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Count", On 2015/08/10 23:39:39, sclittle wrote: > If you're just incrementing by one, could you just use the normal > UMA_HISTOGRAM_SPARSE_SLOWLY? Done. https://codereview.chromium.org/1273303002/diff/80001/sync/engine/directory_u... File sync/engine/directory_update_handler.cc (right): https://codereview.chromium.org/1273303002/diff/80001/sync/engine/directory_u... sync/engine/directory_update_handler.cc:17: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ On 2015/08/10 23:39:39, sclittle wrote: > This code is duplicated, could this be put in sparse_histogram.h or some other > common location? Are you planning to write future code that will also use this > call? You are right and I might need this macro later too. I was hopping to get permission from owner of sparse_histogram.h to put it there, but he didn't accept. So I have to find somewhere else for it. I have asked Nicolas to put it somewhere in sync/engine/ if it is possible and I am waiting for his response. (He is OOO now)
On 2015/08/11 21:04:44, amohammadkhan wrote: > https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc > File sync/engine/commit.cc (right): > > https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc#n... > sync/engine/commit.cc:90: > UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Bytes", > On 2015/08/10 23:39:39, sclittle wrote: > > Could you test somewhere that these histograms are recorded properly in > > unittests? Maybe in sync/engine/syncer_unittest.cc. > > > > HistogramTester might be useful: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... > > Thank you for your helpful suggestions. Done. > > https://codereview.chromium.org/1273303002/diff/80001/sync/engine/commit.cc#n... > sync/engine/commit.cc:94: > UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE("DataUse.Sync.Upload.Count", > On 2015/08/10 23:39:39, sclittle wrote: > > If you're just incrementing by one, could you just use the normal > > UMA_HISTOGRAM_SPARSE_SLOWLY? > > Done. > > https://codereview.chromium.org/1273303002/diff/80001/sync/engine/directory_u... > File sync/engine/directory_update_handler.cc (right): > > https://codereview.chromium.org/1273303002/diff/80001/sync/engine/directory_u... > sync/engine/directory_update_handler.cc:17: #define > UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ > On 2015/08/10 23:39:39, sclittle wrote: > > This code is duplicated, could this be put in sparse_histogram.h or some other > > common location? Are you planning to write future code that will also use this > > call? > > You are right and I might need this macro later too. I was hopping to get > permission from owner of sparse_histogram.h to put it there, but he didn't > accept. So I have to find somewhere else for it. I have asked Nicolas to put it > somewhere in sync/engine/ if it is possible and I am waiting for his response. > (He is OOO now) Currently the numbers in the added test are based the first round of Errors of Expect() and I think it may not be a good idea to get to the numbers in this way. Please let me know if you have any suggestions and feedback about it or basically about the whole test function.
https://codereview.chromium.org/1273303002/diff/70007/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/70007/sync/engine/commit.cc#n... sync/engine/commit.cc:96: //NOTREACHED(); Can you remove this? https://codereview.chromium.org/1273303002/diff/70007/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/70007/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:683: histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Count", 0); Instead of ExpectTotalCount, could you use ExpectUniqueSample or ExpectBucketCount on the BOOKMARKS type or something?
https://codereview.chromium.org/1273303002/diff/70007/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/70007/sync/engine/commit.cc#n... sync/engine/commit.cc:96: //NOTREACHED(); On 2015/08/14 19:10:59, sclittle wrote: > Can you remove this? Done. https://codereview.chromium.org/1273303002/diff/70007/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/70007/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:683: histogram_tester.ExpectTotalCount("DataUse.Sync.Upload.Count", 0); On 2015/08/14 19:10:59, sclittle wrote: > Instead of ExpectTotalCount, could you use ExpectUniqueSample or > ExpectBucketCount on the BOOKMARKS type or something? Done.
https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc#... sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ This should really be put in a common location somewhere instead of being duplicated in each of these files. Even somewhere like https://code.google.com/p/chromium/codesearch#chromium/src/sync/util/data_typ... might work, although I'm not familiar with the sync codebase.
https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc#... sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ On 2015/08/17 18:30:02, sclittle wrote: > This should really be put in a common location somewhere instead of being > duplicated in each of these files. Even somewhere like > https://code.google.com/p/chromium/codesearch#chromium/src/sync/util/data_typ... > might work, although I'm not familiar with the sync codebase. I completely agree with you. I am waiting to get a hint about good location for it from Nicolas and I'll update the files based on it.
I'll defer to sclittle@ and zea@.
On 2015/08/17 18:35:28, amohammadkhan wrote: > https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc > File sync/engine/commit.cc (right): > > https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc#... > sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, > sample, value) \ > On 2015/08/17 18:30:02, sclittle wrote: > > This should really be put in a common location somewhere instead of being > > duplicated in each of these files. Even somewhere like > > > https://code.google.com/p/chromium/codesearch#chromium/src/sync/util/data_typ... > > might work, although I'm not familiar with the sync codebase. > > I completely agree with you. I am waiting to get a hint about good location for > it from Nicolas and I'll update the files based on it. data_type_histogram.h I think would be appropriate.
On 2015/08/18 20:13:17, Nicolas Zea wrote: > On 2015/08/17 18:35:28, amohammadkhan wrote: > > https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc > > File sync/engine/commit.cc (right): > > > > > https://codereview.chromium.org/1273303002/diff/160006/sync/engine/commit.cc#... > > sync/engine/commit.cc:18: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, > > sample, value) \ > > On 2015/08/17 18:30:02, sclittle wrote: > > > This should really be put in a common location somewhere instead of being > > > duplicated in each of these files. Even somewhere like > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/sync/util/data_typ... > > > might work, although I'm not familiar with the sync codebase. > > > > I completely agree with you. I am waiting to get a hint about good location > for > > it from Nicolas and I'll update the files based on it. > > data_type_histogram.h I think would be appropriate. Thank you. Done. Please let me know if anything else needs to be fixed.
https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:670: // This test has three steps. In the first steps BOOKMARKS are sync and they are nit: "BOOKMARKS are sync and they are downloaded" -> "a BOOKMARKS update is received". Similarly, for the third step, "a BOOKMARK update is committed". https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:676: const ModelTypeSet throttled_types(BOOKMARKS); throttled_types appears to be unused? https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:689: histogram_tester.ExpectTotalCount("DataUse.Sync.Download.Bytes", 63); Where are these byte values from? It would be good if they could be calculated on the fly. I worry that anyone who updates the proto message we commit/download will also have to update these hardcoded byte values. https://codereview.chromium.org/1273303002/diff/200001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/200001/sync/util/data_type_hi... sync/util/data_type_histogram.h:15: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ I think it would be better to prefix this with SYNC rather than UMA, since it's not something defined within the normal UMA histograms. Maybe SYNC_RECORD_DATATYPE_BIN or something like that. Also add a comment describing how it's used for datatypes.
https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:670: // This test has three steps. In the first steps BOOKMARKS are sync and they are On 2015/08/18 21:55:18, Nicolas Zea wrote: > nit: "BOOKMARKS are sync and they are downloaded" -> "a BOOKMARKS update is > received". > Similarly, for the third step, "a BOOKMARK update is committed". Done. https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:676: const ModelTypeSet throttled_types(BOOKMARKS); On 2015/08/18 21:55:18, Nicolas Zea wrote: > throttled_types appears to be unused? Done. https://codereview.chromium.org/1273303002/diff/200001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:689: histogram_tester.ExpectTotalCount("DataUse.Sync.Download.Bytes", 63); On 2015/08/18 21:55:18, Nicolas Zea wrote: > Where are these byte values from? It would be good if they could be calculated > on the fly. I worry that anyone who updates the proto message we commit/download > will also have to update these hardcoded byte values. I got those values by test and you are right, they might be changed later. So I changed the test function to not be dependent on these specific values anymore. https://codereview.chromium.org/1273303002/diff/200001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/200001/sync/util/data_type_hi... sync/util/data_type_histogram.h:15: #define UMA_HISTOGRAM_SPARSE_SLOWLY_WITH_VALUE(name, sample, value) \ On 2015/08/18 21:55:18, Nicolas Zea wrote: > I think it would be better to prefix this with SYNC rather than UMA, since it's > not something defined within the normal UMA histograms. Maybe > SYNC_RECORD_DATATYPE_BIN or something like that. Also add a comment describing > how it's used for datatypes. Done.
https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:681: vector<int> progress_bookmark = {0, 0, 0}; nit: std:: here and below https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:692: EXPECT_TRUE(samples.size() == 1); nit: use EXPECT_EQ, EXPECT_GE, EXPECT_GT where possible here and below https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:703: progress_bookmark.at(0) += it->count; there should only be one bucket with value BOOKMARKS right? I think it's cleaner to do an assignment then, rather than += (here and below) https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:707: EXPECT_TRUE(progress_all.at(0) > 0); should progress_all == progress_bookmark? Maybe that's a better condition (here and below)
https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:682: vector<int> progress_all = {0, 0, 0}; Instead of using these progress vectors and comparing them, could you just use a different histogram_tester for each of the blocks below? i.e. maybe scope the histogram_tester within those blocks. You might be able to cut out the for loops in each of the blocks if there would only be 1 sample then. https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:700: for(std::vector<base::Bucket>::iterator it = samples.begin(); nit: use range-based for, e.g. for (const base::Bucket& bucket : samples) https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... sync/util/data_type_histogram.h:17: #define SYNC_RECORD_DATATYPE_BIN(name, sample, value) \ Does this have to be a macro? Would this be better as a helper function?
https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:681: vector<int> progress_bookmark = {0, 0, 0}; On 2015/08/19 22:01:51, Nicolas Zea wrote: > nit: std:: here and below Done. https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:682: vector<int> progress_all = {0, 0, 0}; On 2015/08/19 22:41:35, sclittle wrote: > Instead of using these progress vectors and comparing them, could you just use a > different histogram_tester for each of the blocks below? i.e. maybe scope the > histogram_tester within those blocks. You might be able to cut out the for loops > in each of the blocks if there would only be 1 sample then. By comparing these values I am showing the progress and we will need the for loops anyways. (Because of the progress makers of other types) If you see any other drawbacks besides the loops please let me know. https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:692: EXPECT_TRUE(samples.size() == 1); On 2015/08/19 22:01:51, Nicolas Zea wrote: > nit: use EXPECT_EQ, EXPECT_GE, EXPECT_GT where possible here and below Done. https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:700: for(std::vector<base::Bucket>::iterator it = samples.begin(); On 2015/08/19 22:41:35, sclittle wrote: > nit: use range-based for, e.g. for (const base::Bucket& bucket : samples) Done. https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:703: progress_bookmark.at(0) += it->count; On 2015/08/19 22:01:51, Nicolas Zea wrote: > there should only be one bucket with value BOOKMARKS right? I think it's cleaner > to do an assignment then, rather than += (here and below) Yes it should be one here. But I was thinking maybe it is better to count it if it is more than one for some reason. But in below, it could be more than one. https://codereview.chromium.org/1273303002/diff/260001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:707: EXPECT_TRUE(progress_all.at(0) > 0); On 2015/08/19 22:01:51, Nicolas Zea wrote: > should progress_all == progress_bookmark? Maybe that's a better condition (here > and below) They are not equal because of having progress markers for other types. progress_all is larger than progress_bookmark. https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... sync/util/data_type_histogram.h:17: #define SYNC_RECORD_DATATYPE_BIN(name, sample, value) \ On 2015/08/19 22:41:35, sclittle wrote: > Does this have to be a macro? Would this be better as a helper function? Done.
https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... sync/util/data_type_histogram.h:17: #define SYNC_RECORD_DATATYPE_BIN(name, sample, value) \ On 2015/08/20 00:20:13, amohammadkhan wrote: > On 2015/08/19 22:41:35, sclittle wrote: > > Does this have to be a macro? Would this be better as a helper function? > > Done. Actually, if I recall how histograms work you might run into problems having this be a function. The reason we used macros below is that the normal histogram macros actually instantiate a static variable when you invoke them. As such, by having it be a macro, you ensure that the different places you invoke the macro get different static variables. If you have it be a function, you wind up with a single static variable, which means your different histograms will cross-contaminate each other. Take a look at histogram_macros.h for more detail.
https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/260001/sync/util/data_type_hi... sync/util/data_type_histogram.h:17: #define SYNC_RECORD_DATATYPE_BIN(name, sample, value) \ On 2015/08/20 17:59:50, Nicolas Zea wrote: > On 2015/08/20 00:20:13, amohammadkhan wrote: > > On 2015/08/19 22:41:35, sclittle wrote: > > > Does this have to be a macro? Would this be better as a helper function? > > > > Done. > > Actually, if I recall how histograms work you might run into problems having > this be a function. The reason we used macros below is that the normal histogram > macros actually instantiate a static variable when you invoke them. As such, by > having it be a macro, you ensure that the different places you invoke the macro > get different static variables. If you have it be a function, you wind up with a > single static variable, which means your different histograms will > cross-contaminate each other. > > Take a look at histogram_macros.h for more detail. I see your point. But I used it without static pointers, so the histogram name can vary without making errors. (Or it can be implemented as a function)
I'll defer to zea@ for this code review. LGTM % nits. https://codereview.chromium.org/1273303002/diff/280001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/280001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:700: for(const base::Bucket& bucket : samples) { nit: add space between "for(", for all for loops here
amohammadkhan@google.com changed reviewers: + asvitkine@chromium.org
asvitkine@chromium.org: Please review changes in
https://codereview.chromium.org/1273303002/diff/280001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/280001/sync/engine/commit.cc#... sync/engine/commit.cc:78: it != contributions.end(); ++it) { Nit: Switch to C++11 for loop https://codereview.chromium.org/1273303002/diff/280001/sync/engine/commit.cc#... sync/engine/commit.cc:88: ModelTypeToHistogramInt(it->first)); Nit: Since you use ModelTypeToHistogramInt() twice, use a local var for it. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... File sync/util/data_type_histogram.cc (right): https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Nit: No (c) for new comments. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.cc:7: #include <string> This include should be in the header. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.cc:12: base::HistogramBase* histogram = base::SparseHistogram::FactoryGet( This is indented too much. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.h:17: void SyncRecordDatatypeBin(std::string name, int sample, int value); Nit: const std::string& https://codereview.chromium.org/1273303002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5220: + received updates. For all of these, mention when they're logged.
https://codereview.chromium.org/1273303002/diff/280001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/280001/sync/engine/commit.cc#... sync/engine/commit.cc:78: it != contributions.end(); ++it) { On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: Switch to C++11 for loop Done. https://codereview.chromium.org/1273303002/diff/280001/sync/engine/commit.cc#... sync/engine/commit.cc:88: ModelTypeToHistogramInt(it->first)); On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: Since you use ModelTypeToHistogramInt() twice, use a local var for it. Done. https://codereview.chromium.org/1273303002/diff/280001/sync/engine/syncer_uni... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1273303002/diff/280001/sync/engine/syncer_uni... sync/engine/syncer_unittest.cc:700: for(const base::Bucket& bucket : samples) { On 2015/08/24 18:22:46, sclittle wrote: > nit: add space between "for(", for all for loops here Done. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... File sync/util/data_type_histogram.cc (right): https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: No (c) for new comments. Done. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.cc:7: #include <string> On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > This include should be in the header. Done. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.cc:12: base::HistogramBase* histogram = base::SparseHistogram::FactoryGet( On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > This is indented too much. Done. https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/280001/sync/util/data_type_hi... sync/util/data_type_histogram.h:17: void SyncRecordDatatypeBin(std::string name, int sample, int value); On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: const std::string& Done. https://codereview.chromium.org/1273303002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5220: + received updates. On 2015/08/24 21:27:00, Alexei Svitkine (OOO Aug15-24) wrote: > For all of these, mention when they're logged. Done.
Lgtm, but left the careful review to sclittle.
lgtm https://codereview.chromium.org/1273303002/diff/300001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/300001/sync/util/data_type_hi... sync/util/data_type_histogram.h:10: #include "base/metrics/histogram.h" Nit: Change this to histogram_macros.h https://codereview.chromium.org/1273303002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5220: + received updates. It is updated when an update message received from sync Nit: "message received" -> "message is received" Please fix throughout.
https://codereview.chromium.org/1273303002/diff/300001/sync/util/data_type_hi... File sync/util/data_type_histogram.h (right): https://codereview.chromium.org/1273303002/diff/300001/sync/util/data_type_hi... sync/util/data_type_histogram.h:10: #include "base/metrics/histogram.h" On 2015/08/25 15:49:28, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: Change this to histogram_macros.h Done. https://codereview.chromium.org/1273303002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1273303002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5220: + received updates. It is updated when an update message received from sync On 2015/08/25 15:49:28, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: "message received" -> "message is received" > > Please fix throughout. Done.
lgtm https://codereview.chromium.org/1273303002/diff/320001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/320001/sync/engine/commit.cc#... sync/engine/commit.cc:86: UMA_HISTOGRAM_SPARSE_SLOWLY("DataUse.Sync.Upload.Count", Didn't you change this histogram macro/function?
https://codereview.chromium.org/1273303002/diff/320001/sync/engine/commit.cc File sync/engine/commit.cc (right): https://codereview.chromium.org/1273303002/diff/320001/sync/engine/commit.cc#... sync/engine/commit.cc:86: UMA_HISTOGRAM_SPARSE_SLOWLY("DataUse.Sync.Upload.Count", On 2015/08/25 17:38:10, Nicolas Zea wrote: > Didn't you change this histogram macro/function? Disregard, sent this comment accidentally.
The CQ bit was checked by amohammadkhan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org, bengr@chromium.org, asvitkine@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/1273303002/#ps380001 (title: "Changing the initialization of vectors in syncer_unittest.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1273303002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1273303002/380001
Message was sent while issue was closed.
Committed patchset #21 (id:380001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/30c09859c0ec4afbc12b09fc6adda4312afdbd65 Cr-Commit-Position: refs/heads/master@{#345461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
