Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
227873002:
Create a histogram serialization mechanism in components/metrics (Closed)
|
Created:
6 years, 8 months ago by bsimonnet Modified:
6 years, 7 months ago Reviewers:
achaulk, jar (doing other things), gauravsh, Luigi Semenzato, Alexei Svitkine (slow), Ilya Sherman, Ben Chan, blundell CC:
chromium-reviews, erikwright+watch_chromium.org, jar (doing other things), asvitkine+watch_chromium.org, lcwu1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCreate a histogram serialization mechanism in base
We are removing the dependency of ChromeOS on Chrome to send metrics.
If Chrome is not install on a system, we will provide a simple service that will
upload the metrics. This module will need to read the same logs and use the same
parsing mechanism as Chrome does now. The common serialization code will be kept
in base to make it accessible to both Chrome and ChromeOS.
The code in chrome/browser/chromeos/ will be migrated to use this in an other
CL.
BUG=chromium:358371
TEST=unittest included
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271341
Patch Set 1 #
Total comments: 16
Patch Set 2 : Refactoring to use string instead of char buffers #
Total comments: 19
Patch Set 3 : Namespacing the files with _chromeos, commenting the code, fixing nits #Patch Set 4 : Fixing clang complains about override keyword. #
Total comments: 16
Patch Set 5 : Moving to components instead of base. Adding sanity checks. Fixing lint #
Total comments: 32
Patch Set 6 : Fixing nits and adding sanity checks. #Patch Set 7 : Moving files into an export folder #Patch Set 8 : Fixing uninitialized values possible responsible for trybot fails. #Patch Set 9 : Running tests only for chromeos. #
Total comments: 23
Patch Set 10 : Convert to use scoped_ptr. Remove using declarations. Fixed linting. #
Total comments: 20
Patch Set 11 : Move files into chromeos. Fixed linting issues. #
Total comments: 16
Patch Set 12 : Simplify the code to only one metric sample class. #
Total comments: 11
Patch Set 13 : Rewriting the file manipulation to using blocking locks (using flock) #
Total comments: 16
Patch Set 14 : Using ScopedFD. Fixing buggy test. Fixing nits #Patch Set 15 : Renaming gyp target (nit) #
Total comments: 23
Patch Set 16 : Fixing nits. #Patch Set 17 : Documents the endianness situation. #Patch Set 18 : Update components_test.gyp to match metrics_export. #
Total comments: 28
Patch Set 19 : Fixing bad naming and nits. #
Total comments: 10
Patch Set 20 : Putting ReadMessage into an anonymous namespace. #Patch Set 21 : Fixing last nit and rebasing. #Patch Set 22 : Putting metrics_chromeos behind a flag, hoping to pass mac chromium dbg. #Messages
Total messages: 70 (0 generated)
Here is the first version of the metrics serialization refactor. Please keep in mind that this is my first c++ CL, it is probably not a masterpiece ;) Thanks
https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... File base/metrics/chromeos_metrics.cc (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:28: MetricSample* ChromeOSMetrics::ReadSample(int32 message_size, uint8* buffer) { You're doing an awful lot of reinterpret_casts in this code. You also don't seem to actually need uint8 here, char* would be fine. Maybe pass in std::string (which can hold arbitrary binary data since it is sized)? Maybe use ReadFileToString & Tokenize to split your key-values? Since none of the types seem to need a arbitrary-contents TLV system, you could get away with newlines as separators too https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:58: int32_t ChromeOSMetrics::FormatSample(int32_t buffer_size, char* buffer, This doesn't seem very useful since it just calls vsnprintf and you still need to check the return https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:70: if (message_length < 0) { I don't think negative values are possible with the s variants since they refer to actual output errors https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:90: message_length = sample->Write(buffer_size-len_size, &buffer[len_size]); Should have the write functions return a std::string so you don't have to mess around with buffer sizes https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:128: File::FLAG_READ | File::FLAG_WRITE); Why have write here? If you want to truncate, wouldn't it make sense to do that just before writing? And most people don't expect a ReadX function to really be ReadXAndTruncate https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:136: int read = file.ReadAtCurrentPos(reinterpret_cast<char*>(&message_length), Possible endian issues? Do these files ever leave the device? https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:142: char serialized_sample[message_length]; I think we are avoiding this GCC extension https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metrics.h File base/metrics/chromeos_metrics.h (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.h:8: #include <fcntl.h> Typically the style is to forward declare as much as possible instead of #including in headers https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.h:21: static MetricSample* ReadMessage(int fd); Since everything is static, why not just have this be a namespace instead of a class? https://codereview.chromium.org/227873002/diff/1/base/metrics/linearhistogram... File base/metrics/linearhistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/linearhistogram... base/metrics/linearhistogram_sample.cc:39: sscanf(hist_repr.c_str(), "%127s %d %d", name, &sample, &max); Need to check return. Might want to note that spaces are illegal in names https://codereview.chromium.org/227873002/diff/1/base/metrics/linearhistogram... base/metrics/linearhistogram_sample.cc:47: "linearhistogram%c%s %d %d", '\0', name_.c_str(), Why not inline \0? Also indentation https://codereview.chromium.org/227873002/diff/1/base/metrics/metric_sample.h File base/metrics/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/metric_sample.h... base/metrics/metric_sample.h:24: SampleType getType(); name() for accessor and name_. The variable also shouldn't be public if you have an accessor. https://codereview.chromium.org/227873002/diff/1/base/metrics/useraction_samp... File base/metrics/useraction_sample.h (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/useraction_samp... base/metrics/useraction_sample.h:11: No spacing here
https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... File base/metrics/chromeos_metrics.cc (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:128: File::FLAG_READ | File::FLAG_WRITE); On 2014/04/07 22:16:00, achaulk wrote: > Why have write here? If you want to truncate, wouldn't it make sense to do that > just before writing? And most people don't expect a ReadX function to really be > ReadXAndTruncate The way it works it that the reading and writing would be done by two separate processes that dont communicate so we need to read and truncate while locking the file (and keep that duration as small as possible). My goal was to keep only one copy of each metric at all time (in memory or on the disk) to simplify the logic. https://codereview.chromium.org/227873002/diff/1/base/metrics/chromeos_metric... base/metrics/chromeos_metrics.cc:136: int read = file.ReadAtCurrentPos(reinterpret_cast<char*>(&message_length), On 2014/04/07 22:16:00, achaulk wrote: > Possible endian issues? Do these files ever leave the device? This file will always stay on the device. It is only used to communicate with the running chrome instance. https://codereview.chromium.org/227873002/diff/1/base/metrics/linearhistogram... File base/metrics/linearhistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/1/base/metrics/linearhistogram... base/metrics/linearhistogram_sample.cc:47: "linearhistogram%c%s %d %d", '\0', name_.c_str(), On 2014/04/07 22:16:00, achaulk wrote: > Why not inline \0? Also indentation the compiler complains when I embed the \0 in a format string. Is there a way to go around ?
some quick comments. I will look more after you have addressed this initial batch. https://codereview.chromium.org/227873002/diff/20001/base/metrics/chromeos_me... File base/metrics/chromeos_metrics.h (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/chromeos_me... base/metrics/chromeos_metrics.h:11: class MetricSample; What is the purpose of this file? Are these chromeos specific metrics functions? If so, you probably want to cal them blahblah_chromeos.{cc|h}. This lets the build tooling skip these files on non-CrOS builds. https://codereview.chromium.org/227873002/diff/20001/base/metrics/chromeos_me... base/metrics/chromeos_metrics.h:19: }; NIT: no ; Also needs // namespace ChromeOSMetrics. https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... File base/metrics/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... base/metrics/metric_sample.cc:13: MetricSample::MetricSample(MetricSample::SampleType sample_type) { Use constructor initializer list here (and other places). See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... base/metrics/metric_sample.cc:17: MetricSample::SampleType MetricSample::type() { return type_; } NIT: keep order of definition consistent with declaration in .h file https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... File base/metrics/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... base/metrics/metric_sample.h:15: class BASE_EXPORT MetricSample { I would think about adding comments for these classes, what it represents, when to use it since these are being exported. https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... base/metrics/metric_sample.h:26: virtual bool isValid() const; NIT: newline before this. https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... base/metrics/metric_sample.h:26: virtual bool isValid() const; function names (except accessors) start uppsercase. So IsValid() here and ToString() below. https://codereview.chromium.org/227873002/diff/20001/base/metrics/metric_samp... base/metrics/metric_sample.h:27: SampleType type(); This can be a const function. https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... File base/metrics/sparsehistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.cc:9: #include "base/logging.h" Is this being used? https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.cc:12: #include "base/metrics/sparsehistogram_sample.h" this needs to be after this #include block. https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.cc:22: name_ = histname; Use constructor initializer lists wherever possible. https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.cc:32: int result = sscanf(hist_repr.c_str(), "%127s %d", name, &sample); There's probably a better way of doing this than using sscanf. Have you look at base/strings/ and seen anything there that might be let you do this without having to worry about overflow and other issues? https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.cc:43: "sparsehistogram%c%s %d%c", '\0', name_.c_str(), sample_, '\0'); This is returning a string with embedded NULL bytes, when would a caller use this? https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... File base/metrics/sparsehistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.h:18: int sample(); this can be a const function. https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.h:20: static SparseHistogramSample* ReadSparseHistogram( Needs comment. https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.h:21: const std::string& serializedhist); NIT: serialized_hist. Also avoid abbreviations wherever possible. so perhaps serialized_histograms https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.h:25: int sample_; Do you intend to subclass this? make this private. If you want tests to access use FRIEND_TEST or friend class.
https://codereview.chromium.org/227873002/diff/20001/base/metrics/chromeos_me... File base/metrics/chromeos_metrics.h (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/chromeos_me... base/metrics/chromeos_metrics.h:11: class MetricSample; On 2014/04/11 23:50:51, gauravsh wrote: > What is the purpose of this file? Are these chromeos specific metrics functions? > If so, you probably want to cal them blahblah_chromeos.{cc|h}. This lets the > build tooling skip these files on non-CrOS builds. This will be used by chrome and libmetrics to communicate by reading and writing from /var/log/metrics/uma-events. They will not be needed outside of chromeos so we should suffix them. https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... File base/metrics/sparsehistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/20001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample.cc:43: "sparsehistogram%c%s %d%c", '\0', name_.c_str(), sample_, '\0'); On 2014/04/11 23:50:51, gauravsh wrote: > This is returning a string with embedded NULL bytes, when would a caller use > this? This is used for the serialization. The message format to store the metrics in the /var/log/metrics/uma-events is: message type, \0, message content, \0. ToString is meant to return the serialized version on the metric sample.
Can you give me your opinion on this?
Some initial style comments below. I'm not quite convinced that these should be in base. I understand the pragmatic reason of them not being dependent on anything other than base and ChromeOS wishing to use a minimum set of Chromium targets to build upon. But still, these are pretty specific to live in base, whereas base is generally for things that are widely used/useful which is (at least currently) not the case for these. How about putting these into the metrics component? https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... File base/metrics/metric_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... base/metrics/metric_sample_chromeos.h:29: explicit MetricSample(SampleType t, string metric_name); Use a better name than |t|. Make |metric_name| a const ref. https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... base/metrics/metric_sample_chromeos.h:37: SampleType type() const; For trivial successors, you can inline them in the header. https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... base/metrics/metric_sample_chromeos.h:51: protected: Do these need to be protected or can they be private? (Since the ctor takes the initial values and there's accessors to get them.) Can they also be const? Can you add a DISALLOW_COPY_AND_ASSIGN macro to private section for all of these classes? https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... File base/metrics/metrics_utils_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... base/metrics/metrics_utils_chromeos.cc:7: #include <fcntl.h> Are all these system includes needed? https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... File base/metrics/metrics_utils_unittest_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... base/metrics/metrics_utils_unittest_chromeos.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Nit: Remove (c) from copyright strings for new files. Please fix throughout. https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... base/metrics/metrics_utils_unittest_chromeos.cc:5: // Test of crash class Remove this comment. Make the metrics_utils_chromeos.h the first include. https://codereview.chromium.org/227873002/diff/60001/base/metrics/sparsehisto... File base/metrics/sparsehistogram_sample_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/sparsehisto... base/metrics/sparsehistogram_sample_chromeos.cc:30: if (parts.size() != 2) { Nit: No need for {}'s for single-line bodies. Please fix throughout. https://codereview.chromium.org/227873002/diff/60001/base/metrics/useraction_... File base/metrics/useraction_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/useraction_... base/metrics/useraction_sample_chromeos.h:14: using std::string; Nit: Remove and fully-qualify string. Please fix throughout for all std library using decls. https://codereview.chromium.org/227873002/diff/60001/base/metrics/useraction_... base/metrics/useraction_sample_chromeos.h:16: namespace base { Nit: Please add a blank line after this. Fix throughout. https://codereview.chromium.org/227873002/diff/60001/base/metrics/useraction_... base/metrics/useraction_sample_chromeos.h:29: // static No need for "// static" comment in header file. Put it above the function implementation in the .cc file instead. https://codereview.chromium.org/227873002/diff/60001/base/metrics/useraction_... base/metrics/useraction_sample_chromeos.h:35: } // namespace base Please add a blank line before this. Please fix throughout. https://codereview.chromium.org/227873002/diff/60001/base/metrics/useraction_... base/metrics/useraction_sample_chromeos.h:36: #endif // BASE_METRICS_CHROMEOS_USERACTION_SAMPLE_CHROMEOS_H Please add a blank line before this. Please fix throughout.
https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... File base/metrics/metric_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... base/metrics/metric_sample_chromeos.h:37: SampleType type() const; On 2014/04/17 16:33:21, Alexei Svitkine wrote: > For trivial successors, you can inline them in the header. *accessors
https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... File base/metrics/metric_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/metric_samp... base/metrics/metric_sample_chromeos.h:12: using std::string; I think the general rule is no using declarations in header files https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... File base/metrics/metrics_utils_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... base/metrics/metrics_utils_chromeos.cc:78: int32_t size = msg.length() + sizeof(int32_t); Check that msg.length() <= kMaxMessageLength to avoid creating files that can't be read properly. Or remove the fixed size restriction https://codereview.chromium.org/227873002/diff/60001/base/metrics/metrics_uti... base/metrics/metrics_utils_chromeos.cc:104: char serialized_sample[kMessageMaxLength]; Sanity check that message_length <= kMessageMaxLength otherwise you smash the stack
On 2014/04/17 16:33:21, Alexei Svitkine wrote: > Some initial style comments below. > > I'm not quite convinced that these should be in base. > > I understand the pragmatic reason of them not being dependent on anything other > than base and ChromeOS wishing to use a minimum set of Chromium targets to build > upon. > > But still, these are pretty specific to live in base, whereas base is generally > for things that are widely used/useful which is (at least currently) not the > case for these. > > How about putting these into the metrics component? I agree, we wanted to put it in base because it would have been easier to share it with chromeos (through libchromeos). As we will already be pulling code from component/metrics and dealing with libchromeos will be painful either way, lets put it there.
some fly-by comments https://codereview.chromium.org/227873002/diff/80001/components/metrics.gypi File components/metrics.gypi (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics.gypi#... components/metrics.gypi:19: 'metrics/metrics_utils_chromeos.cc', nit: fix indentation https://codereview.chromium.org/227873002/diff/80001/components/metrics.gypi#... components/metrics.gypi:20: 'metrics/metrics_utils_chromeos.h', I noticed some other parts of chrome uses a chromeos subdirectory to group many chromeos-specific files together instead of using the _chromeos sufffix on individual files. If these classes only exist on chromeos, wouldn't it be better to use the subdirectory approach, especially when there are many of them. That said, it's better check with OWNERS on the preferred ways. https://codereview.chromium.org/227873002/diff/80001/components/metrics/crash... File components/metrics/crash_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/crash... components/metrics/crash_sample_chromeos.h:5: #ifndef COMPONENTS_METRICS_CHROMEOS_CRASH_SAMPLE_CHROMEOS_H nit: I believe the convention is COMPONENTS_METRICS_CHROMEOS_CRASH_SAMPLE_CHROMEOS_H_ instead of COMPONENTS_METRICS_CHROMEOS_CRASH_SAMPLE_CHROMEOS_H https://codereview.chromium.org/227873002/diff/80001/components/metrics/crash... components/metrics/crash_sample_chromeos.h:22: // Produce a serialized version of the crash. nit: [OCD] the comments in this file and other files use singular and plural form inconsistently, e.g. Return vs Returns. https://codereview.chromium.org/227873002/diff/80001/components/metrics/histo... File components/metrics/histogram_sample_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/histo... components/metrics/histogram_sample_chromeos.cc:30: sample_(sample), nit: when possible, order the member variables in the same way as the constructor arguments, so that one may not be mistaken than the variables are initialized out-of-order in constructor initializer list https://codereview.chromium.org/227873002/diff/80001/components/metrics/histo... components/metrics/histogram_sample_chromeos.cc:51: if (parts.size() != 5) return NULL; nit: add a comment to describe the serialized format https://codereview.chromium.org/227873002/diff/80001/components/metrics/histo... components/metrics/histogram_sample_chromeos.cc:57: return new HistogramSample(parts[0], sample, min, max, nbucket); I think SplitString still gives 5 components on a string like " 1 2 3 4", which means parts[0] will be an empty string. Do you want to check parts[0]? https://codereview.chromium.org/227873002/diff/80001/components/metrics/histo... File components/metrics/histogram_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/histo... components/metrics/histogram_sample_chromeos.h:32: // Returns the sample serialized on the format: nit: add a line break https://codereview.chromium.org/227873002/diff/80001/components/metrics/linea... File components/metrics/linearhistogram_sample_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/linea... components/metrics/linearhistogram_sample_chromeos.cc:46: return new LinearHistogramSample(parts[0], sample, max); see previous comments on commenting serialized format and checking parts[0]. https://codereview.chromium.org/227873002/diff/80001/components/metrics/linea... File components/metrics/linearhistogram_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/linea... components/metrics/linearhistogram_sample_chromeos.h:5: #ifndef COMPONENTS_METRICS_LINEARHISTOGRAM_SAMPLE_CHROMEOS_H ditto https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... File components/metrics/metric_sample_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metric_sample_chromeos.cc:12: MetricSample::MetricSample(MetricSample::SampleType sample_type, nit: header calls it |type|, here it's called |sample_type|. https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... File components/metrics/metric_sample_chromeos.h (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metric_sample_chromeos.h:29: explicit MetricSample(SampleType type, const std::string& metric_name); explicit is not needed when there are two or more arguments https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metric_sample_chromeos.h:38: std::string name() const { return name_; } why not "const std::string& name() const" https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... File components/metrics/metrics_utils_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_chromeos.cc:39: const string name = vector[0]; const string& https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_chromeos.cc:53: DLOG(ERROR) << "invalid event type: " << name; would it be useful to print the value as well? https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... File components/metrics/metrics_utils_chromeos.h (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_chromeos.h:15: // chrome os. nit: [ocd] inconsistency: chrome os vs ChromeOS in other files. https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... File components/metrics/metrics_utils_unittest_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:30: ASSERT_EQ(a->name(), b->name()); ASSERT_ stops the test immediately, do you want EXPECT_ instead as you may still want to check other values even if one mismatch is found https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:84: static const int buff_size = 1024; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:85: char* buffer; scoped_ptr<char[]> buffer; https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:86: std::string filename; you can drop std:: as you already have 'using std::string' above. https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:89: filename = "/tmp/chromeossampletest"; avoid hardcoding a temp file for test (and without cleaning up) use base::ScopedTempDir to create a temp directory and then create a temp file there https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:98: std::string serialized(sample->ToString()); you can drop std:: as you already have 'using std::string' above. https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:139: std::string input(StringPrintf("sparsehistogram%cname foo%c", '\0', '\0')); you can drop std:: as you already have 'using std::string' above. https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:147: int64 size; if the compiler yells, you may need to initialize |size| to 0 https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:148: GetFileSize(filepath, &size); ASSERT_TRUE(GetFileSize(filepath, &size)); https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:160: } you can simply do: string name(MetricsUtils::kMessageMaxLength, 'c'); https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:165: GetFileSize(filepath, &size); ditto https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:170: File f(filepath, File::FLAG_OPEN_ALWAYS | File::FLAG_APPEND); nit: |f| feels like a bad variable https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:174: message = message + c; ditto https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:178: f.WriteAtCurrentPos(reinterpret_cast<char*>(&message_size), const char* https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:214: GetFileSize(filepath, &size); ditto
isherman, asvitkine, do you have a preference between suffixing all files with _chromeos or putting them in a chromeos directory and managing the inclusion in metrics.gypi? https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... File components/metrics/metrics_utils_unittest_chromeos.cc (right): https://codereview.chromium.org/227873002/diff/80001/components/metrics/metri... components/metrics/metrics_utils_unittest_chromeos.cc:30: ASSERT_EQ(a->name(), b->name()); On 2014/04/18 20:27:08, Ben Chan wrote: > ASSERT_ stops the test immediately, do you want EXPECT_ instead as you may still > want to check other values even if one mismatch is found yes ! :)
On 2014/04/18 23:53:48, bsimonnet wrote: > isherman, asvitkine, do you have a preference between suffixing all files with > _chromeos or putting them in a chromeos directory and managing the inclusion in > metrics.gypi? I slightly prefer using a chromeos directory because there are plenty of files, but I don't feel strongly about it. You could also do both, if you want to group the files but don't want to make the .gypi file more complicated.
I prefer having a separate directory for them as well. Don't have a strong opinion on file names. (Though one possibility is to have the directory be named according to the functionality it's implementing rather than simply 'chromeos'). On Fri, Apr 18, 2014 at 11:46 PM, <isherman@chromium.org> wrote: > On 2014/04/18 23:53:48, bsimonnet wrote: > >> isherman, asvitkine, do you have a preference between suffixing all files >> with >> _chromeos or putting them in a chromeos directory and managing the >> inclusion >> > in > >> metrics.gypi? >> > > I slightly prefer using a chromeos directory because there are plenty of > files, > but I don't feel strongly about it. You could also do both, if you want to > group the files but don't want to make the .gypi file more complicated. > > https://codereview.chromium.org/227873002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It seems to have passed all the test after some (a lot?) of flaky test on windows.
Ping.
Please update the CL description, since these are no longer moving to base. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/histogram_sample.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/histogram_sample.h:44: const int sample_, min_, max_, nbucket_; Declare these separately. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.cc:11: namespace metrics { Nit: Add a blank line after this. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.cc:23: string MetricSample::ToString() const { return "not implemented"; } Put method body on separate line. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.h:28: }; Nit: Add a blank line after this. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.h:36: virtual bool IsValid() const; Add a blank line after this. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metrics_utils.cc:62: if (!sample.IsValid()) return false; Put return on next line. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metrics_utils.cc:64: File(FilePath(filename), File::FLAG_OPEN_ALWAYS | File::FLAG_APPEND); Nit: You can just do: File file(FilePath(filename), ...); https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metrics_utils.h:23: BASE_EXPORT MetricSample* ReadSample(const std::string& sample); These shouldn't use BASE_EXPORT anymore. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/sparsehistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/sparsehistogram_sample.cc:19: using std::vector; Using declarations are generally discouraged. It doesn't look like they're adding much here, so please remove them. Please do this in the other files too. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/sparsehistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/sparsehistogram_sample.h:25: // Serialize a sample on the format: Nit: Add a blank line above this. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/useraction_sample.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/useraction_sample.cc:28: return new UserActionSample(action); I believe the best practice for APIs that transfer ownership of the object returned is to return the object as a scoped_ptr<Type>. Can you change to this style throughout?
https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/histogram_sample.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/histogram_sample.h:44: const int sample_, min_, max_, nbucket_; On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Declare these separately. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.cc:11: namespace metrics { On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Nit: Add a blank line after this. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.cc:23: string MetricSample::ToString() const { return "not implemented"; } On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Put method body on separate line. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.h:28: }; On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Nit: Add a blank line after this. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metric_sample.h:36: virtual bool IsValid() const; On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Add a blank line after this. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metrics_utils.cc:62: if (!sample.IsValid()) return false; On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Put return on next line. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metrics_utils.cc:64: File(FilePath(filename), File::FLAG_OPEN_ALWAYS | File::FLAG_APPEND); On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Nit: You can just do: > > File file(FilePath(filename), ...); Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/metrics_utils.h:23: BASE_EXPORT MetricSample* ReadSample(const std::string& sample); On 2014/04/30 15:25:24, Alexei Svitkine wrote: > These shouldn't use BASE_EXPORT anymore. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/sparsehistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/sparsehistogram_sample.cc:19: using std::vector; On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Using declarations are generally discouraged. It doesn't look like they're > adding much here, so please remove them. > > Please do this in the other files too. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/sparsehistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/sparsehistogram_sample.h:25: // Serialize a sample on the format: On 2014/04/30 15:25:24, Alexei Svitkine wrote: > Nit: Add a blank line above this. Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... File components/metrics/export/useraction_sample.cc (right): https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/useraction_sample.cc:28: return new UserActionSample(action); On 2014/04/30 15:25:24, Alexei Svitkine wrote: > I believe the best practice for APIs that transfer ownership of the object > returned is to return the object as a scoped_ptr<Type>. Can you change to this > style throughout? Done. https://codereview.chromium.org/227873002/diff/160001/components/metrics/expo... components/metrics/export/useraction_sample.cc:28: return new UserActionSample(action); On 2014/04/30 15:25:24, Alexei Svitkine wrote: > I believe the best practice for APIs that transfer ownership of the object > returned is to return the object as a scoped_ptr<Type>. Can you change to this > style throughout? Yes, it is probably much cleaner.
FYI, when you specify five reviewers for a CL, it's not obvious to the members of that list whose review you're waiting for. That said, since this CL is implementing functionality specific to ChromeOS, I'd like someone familiar with the existing ChromeOS metrics serialization code to review this for correctness. Once you have their approval, Alexei or I can provide an OWNERS-level review -- which should hopefully be quite quick by that point. One comment that I have at this time: The semantics of "export" as a directory name are not clear to me. If this is code intended to be used only on ChromeOS, then "chromeos" is likely a better directory name. If you prefer to go with a name that describes the functionality of the code contained within, then perhaps something like "serialization"?
Sorry, I am still not used to the chromium review process. I added semenzato@ to the review, he build a lot of the metrics part of ChromeOS. I'll rename the directory to chromeos, I feel like any other name will just end up being more confusing than chromeos.
More style comments for you https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/histogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/histogram_sample.cc:55: !base::StringToInt(parts[4], &nbucket)) Nit: {} https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/linearhistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/linearhistogram_sample.cc:34: // separated by space. Nit: This comment should only say: // static The real comment about it should be in the .h file. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/linearhistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/linearhistogram_sample.h:39: const int sample_, max_; Nit: 1 per line. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils.cc:83: base::File file = base::File( Nit: Just do base::File file(...); https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils.cc:95: if (read != sizeof(message_length)) break; Nit: Put on next line. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils.h:13: namespace metrics { Nit: Add empty line after this. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/metrics_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils_unittest.cc:86: } Add empty line after this. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils_unittest.cc:88: base::DeleteFile(filepath, false); What's this call for? https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils_unittest.cc:98: scoped_ptr<char[]> buffer; Is this used somewhere? https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/sparsehistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/sparsehistogram_sample.cc:34: const std::string& histogram_serialized) { Nit: serialized_histogram which also makes it consistent with .h file. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/sparsehistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/sparsehistogram_sample.h:20: class BASE_EXPORT SparseHistogramSample : public MetricSample { Remove BASE_EXPORT. Please fix throughout. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/useraction_sample.h (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/useraction_sample.h:33: const std::string& useraction); Nit: user_action or just action. Make consistent with .cc file.
https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/histogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/histogram_sample.cc:55: !base::StringToInt(parts[4], &nbucket)) On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Nit: {} Done. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/linearhistogram_sample.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/linearhistogram_sample.cc:34: // separated by space. On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Nit: This comment should only say: // static > > The real comment about it should be in the .h file. Done. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/linearhistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/linearhistogram_sample.h:39: const int sample_, max_; On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Nit: 1 per line. Done. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils.cc:83: base::File file = base::File( On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Nit: Just do base::File file(...); Done. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils.cc:95: if (read != sizeof(message_length)) break; On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Nit: Put on next line. Done. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils.h:13: namespace metrics { On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Nit: Add empty line after this. Done. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... File components/metrics/export/metrics_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils_unittest.cc:88: base::DeleteFile(filepath, false); On 2014/05/01 15:57:09, Alexei Svitkine wrote: > What's this call for? To make sure the file is not here and therefore no metrics are currently stored. https://codereview.chromium.org/227873002/diff/180001/components/metrics/expo... components/metrics/export/metrics_utils_unittest.cc:98: scoped_ptr<char[]> buffer; On 2014/05/01 15:57:09, Alexei Svitkine wrote: > Is this used somewhere? no I forgot to delete it.
semenzato: Can you give me some feedback on this ?
Unfortunately my main comment invalidates most of this work. Certainly I am not a C++ person and hope that there are good rebuttals to this. In addition to the small comments in the code, the main problem I have is that this is way too much code for the functionality. Specifically, is it necessary to define classes for each histogram sample type? I don't think it is. There is essentially one operation for a histogram sample, which is SEND. (Internally there are also serialize/deserialize operations, but that's still a very small set). I don't think Chrome define classes for the histogram samples: methods (or even objectless functions) for each histogram operation seem to be the right abstraction. You could consider creating classes for histograms (rather than for samples) but the API for the external (Chrome OS) metrics is stateless: every sample contains all the histogram state (name, bounds, number of buckets). At this point I don't see much benefit in changing that API. If I were to do this, I would move the external metrics code from the Chrome OS directory with as few changes as possible. I might consider changing the serialization to use protobufs, but then you have to worry about synchronizing the Chrome and Chrome OS sides when you push that change, or support mixed versions (if even possible---I don't think the current histogram sample protocol has version info). I am completely fine if somebody overrides this opinion. https://codereview.chromium.org/227873002/diff/200001/components/metrics.gypi File components/metrics.gypi (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics.gypi... components/metrics.gypi:37: 'metrics/chromeos/linearhistogram_sample.cc', Whoa, that's a lot of small files/classes. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/crash_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/crash_sample.h:28: static scoped_ptr<CrashSample> ReadCrash(const std::string& crash_serialized); I didn't know that there are "crash samples" in UMA. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/histogram_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/histogram_sample.h:31: int nbucket() const { return nbucket_; } This may be a residual of my old code, but I have learned since (and agree with) that some less ambiguous name such as "bucket_count" would be better. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/linearhistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/linearhistogram_sample.h:16: // Represent a linear histogram sample (called Enum too). Is this really correct? It is possible that the initial chromeos metrics code merged enumerated histograms and linear histograms because they are similar. But wouldn't a linear histogram possibly have a non-zero lower bound, whereas the enum histograms always starts at zero? https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:32: // Return true if the sample is valid (can be serialized without ambiguity). What does this mean? https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:35: // to disk. Maybe "to disk" is assuming too much here? https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:48: // The format is akward but it is implemented independently in both chrome os typo: awkward What does "implemented independently" mean? https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:29: // We should have a two null terminated strings so split should produce Typo: "... have a two null ..." https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:30: // three chuncks. Chunks. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:32: DLOG(ERROR) << "wrong length" << parts.size(); I like more descriptive messages, for instance: "unexpected length " << parts.size() << " of serialized sample" https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:103: if (file.Seek(base::File::FROM_CURRENT, message_length) != -1) continue; I am not sure the style code accepts a statement on the same line of the IF condition.
On 2014/05/08 17:19:37, Luigi Semenzato wrote: > Unfortunately my main comment invalidates most of this work. Certainly I am not > a C++ person and hope that there are good rebuttals to this. > > In addition to the small comments in the code, the main problem I have is that > this is way too much code for the functionality. Specifically, is it necessary > to define classes for each histogram sample type? I don't think it is. There > is essentially one operation for a histogram sample, which is SEND. (Internally > there are also serialize/deserialize operations, but that's still a very small > set). I don't think Chrome define classes for the histogram samples: methods > (or even objectless functions) for each histogram operation seem to be the right > abstraction. > > You could consider creating classes for histograms (rather than for samples) but > the API for the external (Chrome OS) metrics is stateless: every sample contains > all the histogram state (name, bounds, number of buckets). At this point I > don't see much benefit in changing that API. > > If I were to do this, I would move the external metrics code from the Chrome OS > directory with as few changes as possible. I might consider changing the > serialization to use protobufs, but then you have to worry about synchronizing > the Chrome and Chrome OS sides when you push that change, or support mixed > versions (if even possible---I don't think the current histogram sample protocol > has version info). > > I am completely fine if somebody overrides this opinion. I agree that the model is not very good. The classes are very similar and only implement a simple interface (serialize/deserialize). We clearly dont need to create an object in order to serialize it. When we deserialize it, we need to know how to process the sample (done by the RecordHistogram function in external_metrics). The original goal of this design was to be able to have different "record histogram" implementations for Chrome and ChromeOS. I might have gone too far... A simpler solution might be to have an interface (Recorder) for adding histograms, crash and user actions that would be implemented by external_metrics on Chrome's side and the upload_service on Chrome OS side. A recorder would have the methods recordHistogram, recordCrash, recordUserAction and would be passed as argument to the function that reads metrics. Would that be better? For the serialization mechanism, a protobuf might not be the best option as we would have to deserialize and re-serialize the whole protobuf everytime we need to add a sample. A simpler format could be csv. It would remove the need to check the length of the message and split on both \0 and spaces.
On 2014/05/08 18:25:05, bsimonnet wrote: > > I agree that the model is not very good. The classes are very similar and only > implement > a simple interface (serialize/deserialize). > We clearly dont need to create an object in order to serialize it. > When we deserialize it, we need to know how to process the sample (done by the > RecordHistogram > function in external_metrics). > > The original goal of this design was to be able to have different "record > histogram" > implementations for Chrome and ChromeOS. I might have gone too far... > > A simpler solution might be to have an interface (Recorder) for adding > histograms, > crash and user actions that would be implemented by external_metrics on Chrome's > side > and the upload_service on Chrome OS side. > A recorder would have the methods recordHistogram, recordCrash, recordUserAction > and > would be passed as argument to the function that reads metrics. > > Would that be better? I think so, but I'll defer to someone with deeper C++ insight. The higher-level abstraction is that you have a histogram API and some way of configuring it to use different back ends (ship the samples through Chrome, or through a different agent). New back ends will be added infrequently at best, and the back end will be configured at build time, right? But even if you want to configure it dynamically at run time (init time), passing an enum (METRICS_USE_CHROME_AS_PROXY, METRICS_SHIP_NATIVELY) may be enough. We also have the C API to worry about, although I am not sure what the plans are for that. I would keep it though, since there is a lot of C code around that might like to send histograms or counts. > For the serialization mechanism, a protobuf might not be the best option as we > would have > to deserialize and re-serialize the whole protobuf everytime we need to add a > sample. > A simpler format could be csv. It would remove the need to check the length of > the message and > split on both \0 and spaces.
https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/crash_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/crash_sample.h:28: static scoped_ptr<CrashSample> ReadCrash(const std::string& crash_serialized); On 2014/05/08 17:19:37, Luigi Semenzato wrote: > I didn't know that there are "crash samples" in UMA. There is a way to log a crash (user crash, kernel crash or unclean shutdown of the system) that will be reporter as a stability metric (crash count). I could not find it on uma's metrics so I am not sure if we are still using it. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:32: // Return true if the sample is valid (can be serialized without ambiguity). On 2014/05/08 17:19:37, Luigi Semenzato wrote: > What does this mean? As the fields are separated by spaces, we do not want to have spaces in the histogram names (or \0) as it would make it impossible for the sample to be deserialized. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:48: // The format is akward but it is implemented independently in both chrome os On 2014/05/08 17:19:37, Luigi Semenzato wrote: > typo: awkward > > What does "implemented independently" mean? Two different piece of code that are not synchronized or tested together. The real comment should be: The serialization mechanism is implemented on Chrome OS and deserialization mechanism is implemented on Chrome. It is not easy to change both implementations at the same time and to make sure that on every system the same version will be used. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:103: if (file.Seek(base::File::FROM_CURRENT, message_length) != -1) continue; On 2014/05/08 17:19:37, Luigi Semenzato wrote: > I am not sure the style code accepts a statement on the same line of the IF > condition. It is authorized to enhance readability but I feel like it hurts readability everytime. I stopped using it but forgot some on older CLs.
gaurav, can you give us your opinion on this ?
FWIW, I also kind of agree with semenzato@ about this being too many classes. Sorry I didn't speak up about this earlier... Can we have a single class for this, with different methods for different histogram types? If we later decide we need to different implementations, the methods can be made virtual. On Thu, May 8, 2014 at 4:07 PM, <bsimonnet@chromium.org> wrote: > gaurav, can you give us your opinion on this ? > > https://codereview.chromium.org/227873002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
After talking to gauravsh, a quick summary of the two alternatives with pros and cons: ==The current version== Pros: * easy to test * no dependency injection Cons: * lot of reinterpret_cast * one class for each type of metrics ==Alternative: No metrics_sample (or subclass)== metrics_utils has methods: WriteHistogram WriteLinearHistogram WriteSparseHistogram WriteCrash WriteUserAction ReadAllMetrics (takes a MetricsRecorder) MetricsRecorder is an interface with the following methods: RecordUserAction RecordLinearHistogram RecordSparseHistogram RecordCrash RecordUserAction MetricsRecorder would be implemented by Chrome and ChromeOS to store the metrics in memory and combine them. Pros: * Less reinterpret_cast * Less classes (metrics_utils is only a namspace and MetricsRecorder would be an interface) Cons: * harder to test the serialization/deserialization need to mock the MetricsRecorder and test if a method was called. * need to define an interface in Chrome that will be used in Chrome OS I like the testability of the current version even if it is verbose. The alternative version would reduce boilerplate but is harder to test which makes it less attractive. asvitkine, semenzato, is the alternative what you were thinking about?
On 2014/05/08 21:24:15, bsimonnet wrote: > Cons: > * lot of reinterpret_cast Note: reinterpret_cast is generally not safe. Please see [ http://stackoverflow.com/questions/573294/when-to-use-reinterpret-cast ] for more info.
On 2014/05/08 21:24:15, bsimonnet wrote: > After talking to gauravsh, a quick summary of the two alternatives with pros and > cons: > > ==The current version== > Pros: > * easy to test > * no dependency injection > Cons: > * lot of reinterpret_cast > * one class for each type of metrics > > ==Alternative: No metrics_sample (or subclass)== > metrics_utils has methods: > WriteHistogram > WriteLinearHistogram > WriteSparseHistogram > WriteCrash > WriteUserAction > ReadAllMetrics (takes a MetricsRecorder) > > MetricsRecorder is an interface with the following methods: > RecordUserAction > RecordLinearHistogram > RecordSparseHistogram > RecordCrash > RecordUserAction > > MetricsRecorder would be implemented by Chrome and ChromeOS to store the metrics > in memory and combine them. > Pros: > * Less reinterpret_cast > * Less classes (metrics_utils is only a namspace and MetricsRecorder would be an > interface) > Cons: > * harder to test the serialization/deserialization need to mock the > MetricsRecorder and test if a method was called. > * need to define an interface in Chrome that will be used in Chrome OS > > I like the testability of the current version even if it is verbose. > The alternative version would reduce boilerplate but is harder to test which > makes it less attractive. > > asvitkine, semenzato, is the alternative what you were thinking about? Yes, that goes along with what I was thinking. But I don't understand the testability issues, i.e. I don't see why testability encourages you to create that many classes. (This doesn't mean that there isn't a good reason, just that I don't see it.) Basically, I don't have enough C++ authority or ownership of this Chrome code to make a strong request to rewrite this CL. Isn't there someone in Chrome who should do this instead? Maybe jar@?
Jim, can you take a quick look at this, with particular regard to my comment and the following ones? If you cannot, can you suggest someone else? Thanks!
On 2014/05/08 21:24:15, bsimonnet wrote: > After talking to gauravsh, a quick summary of the two alternatives with pros and > cons: > > ==The current version== > Pros: > * easy to test > * no dependency injection > Cons: > * lot of reinterpret_cast > * one class for each type of metrics > > ==Alternative: No metrics_sample (or subclass)== > metrics_utils has methods: > WriteHistogram > WriteLinearHistogram > WriteSparseHistogram > WriteCrash > WriteUserAction > ReadAllMetrics (takes a MetricsRecorder) > > MetricsRecorder is an interface with the following methods: > RecordUserAction > RecordLinearHistogram > RecordSparseHistogram > RecordCrash > RecordUserAction > > MetricsRecorder would be implemented by Chrome and ChromeOS to store the metrics > in memory and combine them. > Pros: > * Less reinterpret_cast > * Less classes (metrics_utils is only a namspace and MetricsRecorder would be an > interface) > Cons: > * harder to test the serialization/deserialization need to mock the > MetricsRecorder and test if a method was called. > * need to define an interface in Chrome that will be used in Chrome OS > > I like the testability of the current version even if it is verbose. > The alternative version would reduce boilerplate but is harder to test which > makes it less attractive. > > asvitkine, semenzato, is the alternative what you were thinking about? One other alternative it to merge all the functionality of the subclasses into MetricsSample (I think Alexei was suggesting that?) since they are small and straightforward to implement. You don't have to deal with reinterpret_cast (which as pointed out is unsafe) while you can still have a similar implementation of metrics_util. You can still use a vector of MetricSample objects. The serialization methods can do the right depending on type(). Testing-wise, that'd be similar since you rely on the type to choose the right equality comparison method to use anyway.
On 2014/05/08 22:22:47, gauravsh wrote: > On 2014/05/08 21:24:15, bsimonnet wrote: > > After talking to gauravsh, a quick summary of the two alternatives with pros > and > > cons: > > > > ==The current version== > > Pros: > > * easy to test > > * no dependency injection > > Cons: > > * lot of reinterpret_cast > > * one class for each type of metrics > > > > ==Alternative: No metrics_sample (or subclass)== > > metrics_utils has methods: > > WriteHistogram > > WriteLinearHistogram > > WriteSparseHistogram > > WriteCrash > > WriteUserAction > > ReadAllMetrics (takes a MetricsRecorder) > > > > MetricsRecorder is an interface with the following methods: > > RecordUserAction > > RecordLinearHistogram > > RecordSparseHistogram > > RecordCrash > > RecordUserAction > > > > MetricsRecorder would be implemented by Chrome and ChromeOS to store the > metrics > > in memory and combine them. > > Pros: > > * Less reinterpret_cast > > * Less classes (metrics_utils is only a namspace and MetricsRecorder would be > an > > interface) > > Cons: > > * harder to test the serialization/deserialization need to mock the > > MetricsRecorder and test if a method was called. > > * need to define an interface in Chrome that will be used in Chrome OS > > > > I like the testability of the current version even if it is verbose. > > The alternative version would reduce boilerplate but is harder to test which > > makes it less attractive. > > > > asvitkine, semenzato, is the alternative what you were thinking about? > > One other alternative it to merge all the functionality of the subclasses into > MetricsSample (I think Alexei was suggesting that?) since they are small and > straightforward to implement. You don't have to deal with reinterpret_cast > (which as pointed out is unsafe) while you can still have a similar > implementation of metrics_util. You can still use a vector of MetricSample > objects. The serialization methods can do the right depending on type(). > > Testing-wise, that'd be similar since you rely on the type to choose the right > equality comparison method to use anyway.
Sorry asvitkine, I did not understand what you were suggesting (which seems to be the solution gauravsh is describing). It seems to be the best solution as we get the best of both my ideas.
I refactored to code to keep only one class for metric sample as suggested. I am still running trybots but it looks good so far. https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... File components/metrics/chromeos/linearhistogram_sample.h (right): https://codereview.chromium.org/227873002/diff/200001/components/metrics/chro... components/metrics/chromeos/linearhistogram_sample.h:16: // Represent a linear histogram sample (called Enum too). On 2014/05/08 17:19:37, Luigi Semenzato wrote: > Is this really correct? It is possible that the initial chromeos metrics code > merged enumerated histograms and linear histograms because they are similar. > But wouldn't a linear histogram possibly have a non-zero lower bound, whereas > the enum histograms always starts at zero? I remove that comment. The README recommends not to use histograms for enumerations so it is probably better not to mention it here. The current code in external metrics sets min to 1 so it is probably not a good idea so send an enum.
lgtm from my persepctive. Please wait for Alexei's review. I think you will need an OWNERS review (jochen or blundell) for components/component_tests.gyp as well. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... File components/metrics/chromeos/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:97: bool IsEqual(MetricSample* sample); This can be const? https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:37: bool WriteMetricToFile(MetricSample* sample, const std::string& filename); The input could const MetricSample* sample?
Looks good, just a few minor comments. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... File components/metrics/chromeos/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:64: // Build a Histogram "Build a histogram sample" may be better. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:71: // Deserialize a Histogram ... and also here and the following places. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:56: base::File::Error err = file.Lock(); I assume file.Lock() waits for a lock to be released if it finds it locked, correct? Otherwise this will occasionally fail if someone is writing to the file at the same time. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:58: DLOG(ERROR) << "could not lock the file"; Is it possible to print the errno here? Or maybe file.Lock already produced that info? https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:114: file.SetLength(0); Where is the file unlocked? Is it possible to unlock it here so that it's clearer what goes on? If SetLength closes the fd, then that will work, but a comment might help. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:30: // Serialize a metric and write it to filename. Use "sample" instead of "metric" is possible. (It's at least partly my fault that we abuse the word "metric", it's really only plural with that meaning.) https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:31: // The format for the message wrote is: Typo.
https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:56: base::File::Error err = file.Lock(); On 2014/05/12 23:04:48, Luigi Semenzato wrote: > I assume file.Lock() waits for a lock to be released if it finds it locked, > correct? Otherwise this will occasionally fail if someone is writing to the > file at the same time. I thought it did but it does not (it is using fcntl which is asynchronous). File locks on linux are more complicated than I thought: fcntl locks (libbase) and flock (external_metrics and libmetrics) are incompatible. I need to change this to use flock. https://codereview.chromium.org/227873002/diff/220001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:114: file.SetLength(0); On 2014/05/12 23:04:48, Luigi Semenzato wrote: > Where is the file unlocked? Is it possible to unlock it here so that it's > clearer what goes on? If SetLength closes the fd, then that will work, but a > comment might help. The file is closed when |file| is destroyed which unlocks the file too. It should be more explicit after the rewrite with flock.
I rewrote the file locking mechanism to use flock in order to have blocking file locks. @blundell: I added you for the OWNERS review. I also edited the README as you suggested in https://codereview.chromium.org/239093004/.
components_tests.gyp and the README file LGTM with nit https://codereview.chromium.org/227873002/diff/240001/components/metrics.gypi File components/metrics.gypi (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics.gypi... components/metrics.gypi:54: 'target_name': 'metrics_export', This should be named metrics_chromeos I think.
https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... File components/metrics/chromeos/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:150: scoped_ptr<MetricSample> MetricSample::ReadLinearHistogram( Please add: // static Before all the static method definitions. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:158: !base::StringToInt(parts[2], &max)) Nit: {}'s https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:23: #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) Please use arraysize() or ARRAYSIZE_UNSAFE() from base/macros.h instead. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:66: DLOG(ERROR) << "error openning the file"; return false here? https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:94: close(file_descriptor); Can you use ScopedFD from base/files/scoped_file.h instead? https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:116: int fd = open(filename.c_str(), O_RDWR); Can you use ScopedFD from base/files/scoped_file.h instead? https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... File components/metrics/chromeos/metrics_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:153: AreEq(hist.get(), vect[0]); You're ignoring the return value of this - since your AreEq() function returns the result. Honestly, I don't really see the point of AreEq(), just do: EXPECT_TRUE(hist->IsEqual(vect[1]));
https://codereview.chromium.org/227873002/diff/240001/components/metrics.gypi File components/metrics.gypi (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics.gypi... components/metrics.gypi:54: 'target_name': 'metrics_export', On 2014/05/13 07:58:16, blundell wrote: > This should be named metrics_chromeos I think. Done. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... File components/metrics/chromeos/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:150: scoped_ptr<MetricSample> MetricSample::ReadLinearHistogram( On 2014/05/13 15:36:26, Alexei Svitkine wrote: > Please add: > > // static > > Before all the static method definitions. Done. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:158: !base::StringToInt(parts[2], &max)) On 2014/05/13 15:36:26, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:23: #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) On 2014/05/13 15:36:26, Alexei Svitkine wrote: > Please use arraysize() or ARRAYSIZE_UNSAFE() from base/macros.h instead. It is a remainder from the refactoring, I deleted it. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:66: DLOG(ERROR) << "error openning the file"; On 2014/05/13 15:36:26, Alexei Svitkine wrote: > return false here? Done. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:94: close(file_descriptor); On 2014/05/13 15:36:26, Alexei Svitkine wrote: > Can you use ScopedFD from base/files/scoped_file.h instead? Done. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:116: int fd = open(filename.c_str(), O_RDWR); On 2014/05/13 15:36:26, Alexei Svitkine wrote: > Can you use ScopedFD from base/files/scoped_file.h instead? Done. https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... File components/metrics/chromeos/metrics_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/240001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:153: AreEq(hist.get(), vect[0]); On 2014/05/13 15:36:26, Alexei Svitkine wrote: > You're ignoring the return value of this - since your AreEq() function returns > the result. Thanks for catching that, one of those was not equal because of a bug I introduced and it was not failing the test. > Honestly, I don't really see the point of AreEq(), just do: > > EXPECT_TRUE(hist->IsEqual(vect[1])); I agree. AreEq is not useful now that IsEqual contains the logic.
https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:19: const int sample, Nit: No need for primitive-typed params to be const. (Whereas it does make sense for member variables). https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:44: "sparsehistogram%c%s %d%c", '\0', name().c_str(), sample_, '\0'); Nit: Standardize on wrapping - you're using a different way to wrap here than in the cases below. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:141: if (parts[0].length() == 0 || !base::StringToInt(parts[1], &sample)) Nit: .length() == 0 -> emtpty() Change throughout, please. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:180: CHECK(metric); Can the function take a const& to avoid needing to CHECK for NULL entirely? https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:80: int32_t size = msg.length() + sizeof(int32_t); Move |size| to right before it's used, i.e. line 85. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:86: msg = std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; Can you use two WriteFileDescriptor() calls instead of doing this extra temporary string construction? Also, this code currently doesn't guarantee a byte order for |size|. You probably want to ensure it's always in a given endianness (e.g. little endian) via utils from "base/sys_byteorder.h". Same for the read code. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:40: const int kMessageMaxLength = 1024; Does this need to be in the header file? https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:28: virtual void SetUp() { OVERRIDE https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:36: sample->IsEqual(MetricsUtils::ReadSample(serialized).release())); I think you want .get() instead of .release() here, else you're leaking the object. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:120: int32_t message_size = message.length() + sizeof(int32_t); Nit: Prefer int32 over int32_t types for new code. You seem to be using a mix of the two throughout.
https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:19: const int sample, On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Nit: No need for primitive-typed params to be const. (Whereas it does make sense > for member variables). Done. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:44: "sparsehistogram%c%s %d%c", '\0', name().c_str(), sample_, '\0'); On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Nit: Standardize on wrapping - you're using a different way to wrap here than in > the cases below. Done. This formatting was produced by git cl format. Should I use it to format my CLs? https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:141: if (parts[0].length() == 0 || !base::StringToInt(parts[1], &sample)) On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Nit: .length() == 0 -> emtpty() > > Change throughout, please. Done. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:180: CHECK(metric); On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Can the function take a const& to avoid needing to CHECK for NULL entirely? Done. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:80: int32_t size = msg.length() + sizeof(int32_t); On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Move |size| to right before it's used, i.e. line 85. Actually I should use size below. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:86: msg = std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Can you use two WriteFileDescriptor() calls instead of doing this extra > temporary string construction? Done > Also, this code currently doesn't guarantee a byte order for |size|. You > probably want to ensure it's always in a given endianness (e.g. little endian) > via utils from "base/sys_byteorder.h". Same for the read code. TL;DR; we do not have a way to make atomic change on chromeos and chrome. Ideally we would have that but the current code (on chrome and chromeos) do not guarantee the endianness either. If we make the endianness platform independant here, we will have problem reading the metrics on a system where this library is not used on both sides. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:40: const int kMessageMaxLength = 1024; On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Does this need to be in the header file? I think it does. It needs to be access by the tests so it needs to be defined in the header file. The compiler complains if I do not initialize it in the header (Probably because MetricsUtils is a namespace and not a class). https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:28: virtual void SetUp() { On 2014/05/13 19:52:42, Alexei Svitkine wrote: > OVERRIDE Done. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:36: sample->IsEqual(MetricsUtils::ReadSample(serialized).release())); On 2014/05/13 19:52:42, Alexei Svitkine wrote: > I think you want .get() instead of .release() here, else you're leaking the > object. Done. https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils_unittest.cc:120: int32_t message_size = message.length() + sizeof(int32_t); On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Nit: Prefer int32 over int32_t types for new code. You seem to be using a mix of > the two throughout. Done.
https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:86: msg = std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; On 2014/05/13 21:28:00, bsimonnet wrote: > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > Can you use two WriteFileDescriptor() calls instead of doing this extra > > temporary string construction? > Done > > > Also, this code currently doesn't guarantee a byte order for |size|. You > > probably want to ensure it's always in a given endianness (e.g. little endian) > > via utils from "base/sys_byteorder.h". Same for the read code. > > TL;DR; we do not have a way to make atomic change on chromeos and chrome. > > Ideally we would have that but the current code (on chrome and chromeos) do not > guarantee the endianness either. If we make the endianness platform independant > here, we will have problem reading the metrics on a system where this library is > not used on both sides. I see. Are there actual big-endian ChromeOS devices currently? I was assuming that there wasn't so this would be a safe change to do since it wouldn't break anything - but will be futureproof if later there is a big-endian device. If not, then I suggest adding a comment explicitly mentioning why it's not being done.
https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:86: msg = std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; On 2014/05/13 19:52:42, Alexei Svitkine wrote: > Can you use two WriteFileDescriptor() calls instead of doing this extra > temporary string construction? > > Also, this code currently doesn't guarantee a byte order for |size|. You > probably want to ensure it's always in a given endianness (e.g. little endian) > via utils from "base/sys_byteorder.h". Same for the read code. The producer and the consumer of this data are running on the same device, so they have the same endianness.
https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:86: msg = std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; On 2014/05/13 21:36:35, Luigi Semenzato wrote: > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > Can you use two WriteFileDescriptor() calls instead of doing this extra > > temporary string construction? > > > > Also, this code currently doesn't guarantee a byte order for |size|. You > > probably want to ensure it's always in a given endianness (e.g. little endian) > > via utils from "base/sys_byteorder.h". Same for the read code. > > The producer and the consumer of this data are running on the same device, so > they have the same endianness. Benchan mentioned that there are no big endian machines out there at the moment but that might change in the future. We can force the serialization to use little endian now without fearing a break. It will be easier than enforcing a common endianness later on.
On 2014/05/13 21:49:53, bsimonnet wrote: > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > File components/metrics/chromeos/metrics_utils.cc (right): > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > components/metrics/chromeos/metrics_utils.cc:86: msg = > std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; > On 2014/05/13 21:36:35, Luigi Semenzato wrote: > > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > > Can you use two WriteFileDescriptor() calls instead of doing this extra > > > temporary string construction? > > > > > > Also, this code currently doesn't guarantee a byte order for |size|. You > > > probably want to ensure it's always in a given endianness (e.g. little > endian) > > > via utils from "base/sys_byteorder.h". Same for the read code. > > > > The producer and the consumer of this data are running on the same device, so > > they have the same endianness. > > Benchan mentioned that there are no big endian machines out there at the moment > but that might change in the future. > We can force the serialization to use little endian now without fearing a break. > It will be easier than enforcing a common endianness later on. No, this is a problem only if the producer and consumer of this data run on machines with different endianness i.e. the producer runs on a big endian machine and the consumer on a little endian machine or vice versa. But the producer and consumer are two processes on the same machine so the byte order cannot change (unless we assign endianness on a per-process basis but I don't think so). We are never contemplating a distributed version of this.
On 2014/05/13 21:49:53, bsimonnet wrote: > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > File components/metrics/chromeos/metrics_utils.cc (right): > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > components/metrics/chromeos/metrics_utils.cc:86: msg = > std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; > On 2014/05/13 21:36:35, Luigi Semenzato wrote: > > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > > Can you use two WriteFileDescriptor() calls instead of doing this extra > > > temporary string construction? > > > > > > Also, this code currently doesn't guarantee a byte order for |size|. You > > > probably want to ensure it's always in a given endianness (e.g. little > endian) > > > via utils from "base/sys_byteorder.h". Same for the read code. > > > > The producer and the consumer of this data are running on the same device, so > > they have the same endianness. > > Benchan mentioned that there are no big endian machines out there at the moment > but that might change in the future. > We can force the serialization to use little endian now without fearing a break. > It will be easier than enforcing a common endianness later on. No, this is a problem only if the producer and consumer of this data run on machines with different endianness i.e. the producer runs on a big endian machine and the consumer on a little endian machine or vice versa. But the producer and consumer are two processes on the same machine so the byte order cannot change (unless we assign endianness on a per-process basis but I don't think so). We are never contemplating a distributed version of this.
On 2014/05/13 22:01:09, Luigi Semenzato wrote: > On 2014/05/13 21:49:53, bsimonnet wrote: > > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > > File components/metrics/chromeos/metrics_utils.cc (right): > > > > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > > components/metrics/chromeos/metrics_utils.cc:86: msg = > > std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; > > On 2014/05/13 21:36:35, Luigi Semenzato wrote: > > > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > > > Can you use two WriteFileDescriptor() calls instead of doing this extra > > > > temporary string construction? > > > > > > > > Also, this code currently doesn't guarantee a byte order for |size|. You > > > > probably want to ensure it's always in a given endianness (e.g. little > > endian) > > > > via utils from "base/sys_byteorder.h". Same for the read code. > > > > > > The producer and the consumer of this data are running on the same device, > so > > > they have the same endianness. > > > > Benchan mentioned that there are no big endian machines out there at the > moment > > but that might change in the future. > > We can force the serialization to use little endian now without fearing a > break. > > It will be easier than enforcing a common endianness later on. > > No, this is a problem only if the producer and consumer of this data run on > machines with different endianness i.e. the producer runs on a big endian > machine and the consumer on a little endian machine or vice versa. But the > producer and consumer are two processes on the same machine so the byte order > cannot change (unless we assign endianness on a per-process basis but I don't > think so). We are never contemplating a distributed version of this. Agreed. We do not need to have a fixed endianness. This implementation will work the same way on both little endian and big endian. The upside of fixing the endianness to LE is that people get scared when seeing an int being written to disk without any mention of endianness. Also, right now, fixing the endianness is a three line change. If we ever find it useful to have a fixed endianness in the future, the change will be significantly more painful.
On 2014/05/13 22:22:07, bsimonnet wrote: > On 2014/05/13 22:01:09, Luigi Semenzato wrote: > > On 2014/05/13 21:49:53, bsimonnet wrote: > > > > > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > > > File components/metrics/chromeos/metrics_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > > > components/metrics/chromeos/metrics_utils.cc:86: msg = > > > std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; > > > On 2014/05/13 21:36:35, Luigi Semenzato wrote: > > > > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > > > > Can you use two WriteFileDescriptor() calls instead of doing this extra > > > > > temporary string construction? > > > > > > > > > > Also, this code currently doesn't guarantee a byte order for |size|. You > > > > > probably want to ensure it's always in a given endianness (e.g. little > > > endian) > > > > > via utils from "base/sys_byteorder.h". Same for the read code. > > > > > > > > The producer and the consumer of this data are running on the same device, > > so > > > > they have the same endianness. > > > > > > Benchan mentioned that there are no big endian machines out there at the > > moment > > > but that might change in the future. > > > We can force the serialization to use little endian now without fearing a > > break. > > > It will be easier than enforcing a common endianness later on. > > > > No, this is a problem only if the producer and consumer of this data run on > > machines with different endianness i.e. the producer runs on a big endian > > machine and the consumer on a little endian machine or vice versa. But the > > producer and consumer are two processes on the same machine so the byte order > > cannot change (unless we assign endianness on a per-process basis but I don't > > think so). We are never contemplating a distributed version of this. > > Agreed. We do not need to have a fixed endianness. This implementation will work > the same way on both little endian and big endian. The upside of fixing the > endianness to LE is that people get scared when seeing an int being written to > disk without any mention of endianness. Also, right now, fixing the endianness > is a three line change. If we ever find it useful to have a fixed endianness in > the future, the change will be significantly more painful. To prevent scare, a comment would suffice. By a similar reasoning, "fixing" the endianness might confuse readers into believing this goes over the network. I really don't see any benefit in standardizing the byte order but I am not objecting to it.
On 2014/05/13 22:31:02, Luigi Semenzato wrote: > On 2014/05/13 22:22:07, bsimonnet wrote: > > On 2014/05/13 22:01:09, Luigi Semenzato wrote: > > > On 2014/05/13 21:49:53, bsimonnet wrote: > > > > > > > > > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > > > > File components/metrics/chromeos/metrics_utils.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/227873002/diff/270001/components/metrics/chro... > > > > components/metrics/chromeos/metrics_utils.cc:86: msg = > > > > std::string(reinterpret_cast<char*>(&size), sizeof(int32_t)) + msg; > > > > On 2014/05/13 21:36:35, Luigi Semenzato wrote: > > > > > On 2014/05/13 19:52:42, Alexei Svitkine wrote: > > > > > > Can you use two WriteFileDescriptor() calls instead of doing this > extra > > > > > > temporary string construction? > > > > > > > > > > > > Also, this code currently doesn't guarantee a byte order for |size|. > You > > > > > > probably want to ensure it's always in a given endianness (e.g. little > > > > endian) > > > > > > via utils from "base/sys_byteorder.h". Same for the read code. > > > > > > > > > > The producer and the consumer of this data are running on the same > device, > > > so > > > > > they have the same endianness. > > > > > > > > Benchan mentioned that there are no big endian machines out there at the > > > moment > > > > but that might change in the future. > > > > We can force the serialization to use little endian now without fearing a > > > break. > > > > It will be easier than enforcing a common endianness later on. > > > > > > No, this is a problem only if the producer and consumer of this data run on > > > machines with different endianness i.e. the producer runs on a big endian > > > machine and the consumer on a little endian machine or vice versa. But the > > > producer and consumer are two processes on the same machine so the byte > order > > > cannot change (unless we assign endianness on a per-process basis but I > don't > > > think so). We are never contemplating a distributed version of this. > > > > Agreed. We do not need to have a fixed endianness. This implementation will > work > > the same way on both little endian and big endian. The upside of fixing the > > endianness to LE is that people get scared when seeing an int being written to > > disk without any mention of endianness. Also, right now, fixing the endianness > > is a three line change. If we ever find it useful to have a fixed endianness > in > > the future, the change will be significantly more painful. > > To prevent scare, a comment would suffice. > > By a similar reasoning, "fixing" the endianness might confuse readers into > believing this goes over the network. > > I really don't see any benefit in standardizing the byte order but I am not > objecting to it. I see your point. Fixing a non problem to avoid confusion might actually be more confusing... I will just put comments on the code explaining why we don't check the endianness.
Added comments to explain why we do not check the endianness when writing the message length.
I am okay either way, but the other benefit of fixing endianness is it allows the possibility for using this serialization mechanism for other purposes down the road that don't have the same assumptions (i.e. on the same machine). But again, I'll leave it up to you. On Tue, May 13, 2014 at 6:56 PM, <bsimonnet@chromium.org> wrote: > Added comments to explain why we do not check the endianness when writing > the > message length. > > https://codereview.chromium.org/227873002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
If we ever decide to relax that assumption, we would probably want to change the serialization mechanism completely. I think it is better to keep it as is for now.
asvitkine, do you have other suggestions ?
A few more comments, but I think this is pretty close. Thanks! https://codereview.chromium.org/227873002/diff/330001/components/metrics.gypi File components/metrics.gypi (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics.gypi... components/metrics.gypi:29: ], Nit: Bad indentation. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:69: CHECK(type_ == USER_ACTION); Nit: CHECK_EQ. Same for the ones below. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:78: CHECK(type_ != USER_ACTION && type_ != CRASH); Nit: Split into two statements. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:61: // Build a crash sample Nit: Add . at the end, same for the others. Also, use a consistent verb tense (e.g. here you use "Build" but bellow you use "Returns"). I suggest Build -> Builds, Deserialize -> Deserializes, etc. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:56: bool MetricsUtils::WriteMetricToFile(const MetricSample* sample, Can this take a const& sample? https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:113: if (errno != ENOENT) { Nit: No {}'s https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:133: // This processes all messages in the log. Each message starts with a 4-byte Nit: Add an empty line before this. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:146: if (result == 0) { // This indicates a normal EOF. Nit: No {}'s. Or Keep {}'s and move comment to the line below. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:154: // kMessageMaxLength applies to the entire message: the 4-byte This comment seems misplaced since the check directly below it doesn't use kMessageMaxLength. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:156: if (message_size < 2 + static_cast<int>(sizeof(message_size))) { Do we really need the magic number 2 here? It seems bad to put the assumption about the two null chars in this file. It seems to me that ReadSample() should already fail if the part after the message is < 2 chars in size, so maybe just remove the 2? (And the comment about null terminated strings from this function.) https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:181: } Nit: Consider making a helper function for reading the message that will make this function more concise. i.e. the loop body can then become: std::string message; bool success = ReadMessage(fd.get(), &message); if (!success) break; scoped_ptr<MetricSample> sample(ParseSample(message)); if (!sample) break; metrics->push_back(sample.release()); https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:184: if (sample.get() == NULL) { Nit: just !sample and remove {}'s https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:19: namespace MetricsUtils { Can this have a better name? It's already in the metrics namespace - how about SerializationUtils? (Likewise for the name of the file.) https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:24: scoped_ptr<MetricSample> ReadSample(const std::string& sample); Nit: A better name might be "ParseSample", since this function isn't actually reading from a file.
https://codereview.chromium.org/227873002/diff/330001/components/metrics.gypi File components/metrics.gypi (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics.gypi... components/metrics.gypi:29: ], On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: Bad indentation. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metric_sample.cc (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:69: CHECK(type_ == USER_ACTION); On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: CHECK_EQ. Same for the ones below. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metric_sample.cc:78: CHECK(type_ != USER_ACTION && type_ != CRASH); On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: Split into two statements. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metric_sample.h (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metric_sample.h:61: // Build a crash sample On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: Add . at the end, same for the others. > > Also, use a consistent verb tense (e.g. here you use "Build" but bellow you use > "Returns"). I suggest Build -> Builds, Deserialize -> Deserializes, etc. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.cc (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:56: bool MetricsUtils::WriteMetricToFile(const MetricSample* sample, On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Can this take a const& sample? Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:113: if (errno != ENOENT) { On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: No {}'s Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:133: // This processes all messages in the log. Each message starts with a 4-byte On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: Add an empty line before this. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:146: if (result == 0) { // This indicates a normal EOF. On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: No {}'s. Or Keep {}'s and move comment to the line below. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:154: // kMessageMaxLength applies to the entire message: the 4-byte On 2014/05/15 13:43:26, Alexei Svitkine wrote: > This comment seems misplaced since the check directly below it doesn't use > kMessageMaxLength. Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:156: if (message_size < 2 + static_cast<int>(sizeof(message_size))) { On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Do we really need the magic number 2 here? It seems bad to put the assumption > about the two null chars in this file. > > It seems to me that ReadSample() should already fail if the part after the > message is < 2 chars in size, so maybe just remove the 2? (And the comment about > null terminated strings from this function.) Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:181: } On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: Consider making a helper function for reading the message that will make > this function more concise. > > i.e. the loop body can then become: > > std::string message; > bool success = ReadMessage(fd.get(), &message); > if (!success) > break; > scoped_ptr<MetricSample> sample(ParseSample(message)); > if (!sample) > break; > metrics->push_back(sample.release()); Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.cc:184: if (sample.get() == NULL) { On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: just !sample and remove {}'s Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... File components/metrics/chromeos/metrics_utils.h (right): https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:19: namespace MetricsUtils { On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Can this have a better name? It's already in the metrics namespace - how about > SerializationUtils? (Likewise for the name of the file.) Done. https://codereview.chromium.org/227873002/diff/330001/components/metrics/chro... components/metrics/chromeos/metrics_utils.h:24: scoped_ptr<MetricSample> ReadSample(const std::string& sample); On 2014/05/15 13:43:26, Alexei Svitkine wrote: > Nit: A better name might be "ParseSample", since this function isn't actually > reading from a file. Done.
https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... File components/metrics/chromeos/serialization_utils.cc (right): https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:111: bool SerializationUtils::ReadMessage(int file_descriptor, Nit: Put this function into an anon namespace at the top of the file, since it doesn't need to be publicly exposed in the header. https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:137: if (HANDLE_EINTR(lseek(file_descriptor, message_size - 4, SEEK_CUR)) == Nit: I think changing |file_descriptor| -> |fd| will make wrapping a bit nicer in this function. https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:142: *message = ""; Nit: This isn't needed, right? https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... File components/metrics/chromeos/serialization_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils_unittest.cc:37: ASSERT_FALSE(deserialized.get() == NULL); Nit: EXPECT_TRUE()?
https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... File components/metrics/chromeos/serialization_utils.cc (right): https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:142: *message = ""; On 2014/05/15 19:41:44, Alexei Svitkine wrote: > Nit: This isn't needed, right? Not in the current format, this is meant to be a protection against refactoring as the behaviour of ReadMessage is not very standard. As we dont know what is in message in the beginning of this function if message contains a valid message, the code will be wrong. I'll add DCHECK(message.empty()) at the beginning of the function to make sure it is used properly.
https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... File components/metrics/chromeos/serialization_utils.cc (right): https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:111: bool SerializationUtils::ReadMessage(int file_descriptor, On 2014/05/15 19:41:44, Alexei Svitkine wrote: > Nit: Put this function into an anon namespace at the top of the file, since it > doesn't need to be publicly exposed in the header. Done. https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:137: if (HANDLE_EINTR(lseek(file_descriptor, message_size - 4, SEEK_CUR)) == On 2014/05/15 19:41:44, Alexei Svitkine wrote: > Nit: I think changing |file_descriptor| -> |fd| will make wrapping a bit nicer > in this function. Done. https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:142: *message = ""; On 2014/05/15 20:50:21, bsimonnet wrote: > On 2014/05/15 19:41:44, Alexei Svitkine wrote: > > Nit: This isn't needed, right? > > Not in the current format, this is meant to be a protection against refactoring > as the behaviour of ReadMessage is not very standard. > > As we dont know what is in message in the beginning of this function if message > contains a valid message, the code will be wrong. I'll add > DCHECK(message.empty()) at the beginning of the function to make sure it is used > properly. Imposing constraint on the pointer used to store the output does not seem like a good idea. I feel like it makes more sense to consider that reading a badly formatted sample is like reading an empty sample. I added a comment to explain it. Would that work? https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... File components/metrics/chromeos/serialization_utils_unittest.cc (right): https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils_unittest.cc:37: ASSERT_FALSE(deserialized.get() == NULL); On 2014/05/15 19:41:44, Alexei Svitkine wrote: > Nit: EXPECT_TRUE()? Done.
LGTM! https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... File components/metrics/chromeos/serialization_utils.cc (right): https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... components/metrics/chromeos/serialization_utils.cc:142: *message = ""; On 2014/05/15 21:19:42, bsimonnet wrote: > On 2014/05/15 20:50:21, bsimonnet wrote: > > On 2014/05/15 19:41:44, Alexei Svitkine wrote: > > > Nit: This isn't needed, right? > > > > Not in the current format, this is meant to be a protection against > refactoring > > as the behaviour of ReadMessage is not very standard. > > > > As we dont know what is in message in the beginning of this function if > message > > contains a valid message, the code will be wrong. I'll add > > DCHECK(message.empty()) at the beginning of the function to make sure it is > used > > properly. > > Imposing constraint on the pointer used to store the output does not seem like a > good idea. > I feel like it makes more sense to consider that reading a badly formatted > sample is like reading an empty sample. > I added a comment to explain it. > Would that work? Sorry, my comment was actually incorrect - I had missed that this returned true and the empty message was so that parsing isn't aborted, but rather than the message skipped. Comment SGTM, thanks! Nit: Change *message = ""; to message->clear(); to avoid constructing temporaries.
On 2014/05/15 22:40:50, Alexei Svitkine wrote: > LGTM! > > https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... > File components/metrics/chromeos/serialization_utils.cc (right): > > https://codereview.chromium.org/227873002/diff/380001/components/metrics/chro... > components/metrics/chromeos/serialization_utils.cc:142: *message = ""; > On 2014/05/15 21:19:42, bsimonnet wrote: > > On 2014/05/15 20:50:21, bsimonnet wrote: > > > On 2014/05/15 19:41:44, Alexei Svitkine wrote: > > > > Nit: This isn't needed, right? > > > > > > Not in the current format, this is meant to be a protection against > > refactoring > > > as the behaviour of ReadMessage is not very standard. > > > > > > As we dont know what is in message in the beginning of this function if > > message > > > contains a valid message, the code will be wrong. I'll add > > > DCHECK(message.empty()) at the beginning of the function to make sure it is > > used > > > properly. > > > > Imposing constraint on the pointer used to store the output does not seem like > a > > good idea. > > I feel like it makes more sense to consider that reading a badly formatted > > sample is like reading an empty sample. > > I added a comment to explain it. > > Would that work? > > Sorry, my comment was actually incorrect - I had missed that this returned true > and the empty message was so that parsing isn't aborted, but rather than the > message skipped. Comment SGTM, thanks! > > Nit: Change *message = ""; to message->clear(); to avoid constructing > temporaries. All sane trybot passed. I'm commiting.
The CQ bit was checked by bsimonnet@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/227873002/430001
The CQ bit was checked by bsimonnet@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bsimonnet@chromium.org/227873002/430001
Message was sent while issue was closed.
Change committed as 271341 |
