Record UMA statistics for file_stream operations.

Record UMA statistics for file_stream operations.

Allows control over whether or not to record the statistics based on flags.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102560

[ahendrickson, 2011-08-12 16:06:21]

http://codereview.chromium.org/7583049/diff/4002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/4002/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:150: explicit AsyncContext();
Removed 'explicit'

http://codereview.chromium.org/7583049/diff/4002/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:349: RecordFileError(errno, FILE_ERROR_TYPES_OPEN,
class_flags_);
Saved errno, in case it gets changed by the recording code.

Here, and below.

[cbentzel, 2011-08-15 19:49:11]

Top-level request: Remove the notion of class from this CL, as it's not needed
now and adds a significant amount of complexity. If it's ever needed, add it in
then.

You'll then only need two histograms - the one recording the known
DOWNLOAD_ERROR_TYPES, and the one recording errno/HRESULT style errors. The
DOWNLOAD_ERROR_TYPES can be common code. The others should move into
file_stream_posix and file_stream_win to simplify all the #if defined's.

http://codereview.chromium.org/7583049/diff/8001/content/browser/download/bas...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/7583049/diff/8001/content/browser/download/bas...
content/browser/download/base_file.cc:39: if (file_stream_.get())
When will you not have a file_stream? It's fine to be safe, I just don't know
when this would happen.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.cc#newc...
net/base/file_stream.cc:22: #if  defined(OS_WIN)
This would be a lot clearer and fewer #if defined sections if most of the logic
moved to file_stream_posix and file_stream_win rather than a combined file. Move
the platform independent code to file_stream_metrics.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:23: enum RecordingClass {
This CL would be a lot easier without the RecordingClass. It doesn't seem to add
much, and I'd strongly recommend removing it. I definitely want to see the
global removed.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:30: enum FileErrorTypes {
This doesn't need to be exposed in this header - it's internal to the
implementation of file_stream. If you want it to be shared, move it to a
file_stream_metrics.h header which only file_stream_posix and file_stream_win
include.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:30: enum FileErrorTypes {
Nit: maybe FileError rather than FileErrorTypes

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:44: void RecordFileError(int error, FileErrorTypes type,
int class_flags);
This would also move to the new header.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:57: explicit FileStream(int class_flags);
Remove the new constructors and just use EnableRecording.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:67: // Construct a FileStream with an existing file
handle and opening flags.
Ditto.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:167: void EnableRecording(bool enable, int class_flags);
Just make this 

void EnableRecording();

enable is never non-true by callers. If you really want to include |class_flags|
you can.

Also, document this method.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:288: auto_closed_(true) {
class_flags_ needs to be initialized.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:303: auto_closed_(false) {
class_flags_ needs to be initialized.

[ahendrickson, 2011-08-15 23:42:26]

http://codereview.chromium.org/7583049/diff/8001/content/browser/download/bas...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/7583049/diff/8001/content/browser/download/bas...
content/browser/download/base_file.cc:39: if (file_stream_.get())
On 2011/08/15 19:49:11, cbentzel wrote:
> When will you not have a file_stream? It's fine to be safe, I just don't know
> when this would happen. 

This is the typical case for downloads, as the file might not determined until
the user selects a name.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.cc#newc...
net/base/file_stream.cc:22: #if  defined(OS_WIN)
On 2011/08/15 19:49:11, cbentzel wrote:
> This would be a lot clearer and fewer #if defined sections if most of the
logic
> moved to file_stream_posix and file_stream_win rather than a combined file.
Move
> the platform independent code to file_stream_metrics.

Moved the code to file_stream_metrics*.*.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:23: enum RecordingClass {
On 2011/08/15 19:49:11, cbentzel wrote:
> This CL would be a lot easier without the RecordingClass. It doesn't seem to
add
> much, and I'd strongly recommend removing it. I definitely want to see the
> global removed.

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:30: enum FileErrorTypes {
On 2011/08/15 19:49:11, cbentzel wrote:
> Nit: maybe FileError rather than FileErrorTypes

Renamed to FileErrorSource.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:30: enum FileErrorTypes {
On 2011/08/15 19:49:11, cbentzel wrote:
> This doesn't need to be exposed in this header - it's internal to the
> implementation of file_stream. If you want it to be shared, move it to a
> file_stream_metrics.h header which only file_stream_posix and file_stream_win
> include.

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:44: void RecordFileError(int error, FileErrorTypes type,
int class_flags);
On 2011/08/15 19:49:11, cbentzel wrote:
> This would also move to the new header.

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:57: explicit FileStream(int class_flags);
On 2011/08/15 19:49:11, cbentzel wrote:
> Remove the new constructors and just use EnableRecording. 

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:67: // Construct a FileStream with an existing file
handle and opening flags.
On 2011/08/15 19:49:11, cbentzel wrote:
> Ditto. 

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream.h#newco...
net/base/file_stream.h:167: void EnableRecording(bool enable, int class_flags);
On 2011/08/15 19:49:11, cbentzel wrote:
> Just make this 
> 
> void EnableRecording();
> 
> enable is never non-true by callers. If you really want to include
|class_flags|
> you can.
> 
> Also, document this method.

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:288: auto_closed_(true) {
On 2011/08/15 19:49:11, cbentzel wrote:
> class_flags_ needs to be initialized.

Done.

http://codereview.chromium.org/7583049/diff/8001/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:303: auto_closed_(false) {
On 2011/08/15 19:49:11, cbentzel wrote:
> class_flags_ needs to be initialized.

Done.

[cbentzel, 2011-08-16 13:36:02]

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream.h#newc...
net/base/file_stream.h:136: void EnableRecording();
Nit: EnableErrorRecording? or EnableErrorReporting?

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:9: #include <windows.h>
You shouldn't need to include windows.h here.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:14: #include "net/base/file_stream.h"
You shouldn't need to include file_stream.h either.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:30: DLOG(WARNING) << "() " << "Recording error "
<< error
Remove this.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:36: max_error = MaxError();
You might want to document that this can never change during the run of a
program.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:40: {
Nit: Do you need these braces? May be less vertical space without.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:5: // This file defines FileStream, a basic
interface for reading and writing files
This comment is wrong.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:31: void RecordFileError(int error,
FileErrorSource type);
After seeing this be used, may be easier to change the signature to 

RecordFileError(int error, FileErrorSource type, bool record);

Also, you should document what |error| is - in this case, it is an errno/HRESULT
style error rather than a net error.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:31: void RecordFileError(int error,
FileErrorSource type);
In Win, this is actually a DWORD for error. DWORD is an unsigned long. Yes,
probably can cast to an int but may be worth some additional consideration.

errno is an int on the errno.h's I've seen - may be specified by OpenGroup as
well.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:37: { 1898, 1938, false },
How did you get these ranges? Why are there gaps?

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:134: return kErrorBucketStart + (2 * n) - 1;
 // In gap before the bucket.
I'm lost about what this is doing.

Say I have an error of value 1950

This will trigger on n = 13 

Then I return 400 + (2*13) - 1 = 425

I get that the 400 needs to be greater than 321 - and that you may want to leave
a gap.

What is the 2x multiple for? 

Note: I also think this going to be very difficult to decipher when looking at
the histograms. I think it would be much simpler to have two histograms - one
where error codes are individually bucketed, another where they are bucketed in
constant-sized buckets. What do you think?

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:367: int error = errno;
Perhaps a helper function like

MapAndRecordErrorCode(error, record_uma_)

to help minimize duplication here.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:380: if (record_uma_)
I don't think it's worth recording these errors related to 

if (!IsOpen())

at the top of many of the functions. It's more of a programming error than
return codes we are seeing from the OS/library calls.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:488: int result = ftruncate(file_, bytes);
This existed before - but we may want to make this HANDLE_EINTR(ftruncate(file_,
bytes)). It looks like EINTR can be a return value during ftruncate - perhaps
only when trying to expand the file rather than shrink it.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:509: record_uma_ = true;
Do you need to do the 

if (async_context_)
  async_context_->EnableRecording()

here as well? You have it in the Win version, and then you could replace passing
in record_uma to all of the function calls on the async_context_.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:117: RecordFileError(error, FILE_ERROR_SOURCE_READ);
This could be done for both reads and writes. You probably need to record the
source in the AsyncContext.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:201: if (record_uma_)
Again - in this file I would remove the recording of bad IsOpen calls at the top
of the functions.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:337: if (async_context_.get() && error ==
ERROR_IO_PENDING) {
So, in the Windows case I _don't_ think it will help to have a helper function
to MapAndRecordErrorCode() due to all the special cases.

[ahendrickson, 2011-08-17 20:12:04]

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream.h#newc...
net/base/file_stream.h:136: void EnableRecording();
On 2011/08/16 13:36:02, cbentzel wrote:
> Nit: EnableErrorRecording? or EnableErrorReporting? 

EnableErrorStatistics.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:9: #include <windows.h>
On 2011/08/16 13:36:02, cbentzel wrote:
> You shouldn't need to include windows.h here. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:14: #include "net/base/file_stream.h"
On 2011/08/16 13:36:02, cbentzel wrote:
> You shouldn't need to include file_stream.h either. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:30: DLOG(WARNING) << "() " << "Recording error "
<< error
On 2011/08/16 13:36:02, cbentzel wrote:
> Remove this. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:36: max_error = MaxError();
On 2011/08/16 13:36:02, cbentzel wrote:
> You might want to document that this can never change during the run of a
> program.

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:40: {
On 2011/08/16 13:36:02, cbentzel wrote:
> Nit: Do you need these braces? May be less vertical space without. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:31: void RecordFileError(int error,
FileErrorSource type);
On 2011/08/16 13:36:02, cbentzel wrote:
> After seeing this be used, may be easier to change the signature to 
> 
> RecordFileError(int error, FileErrorSource type, bool record);
> 
> Also, you should document what |error| is - in this case, it is an
errno/HRESULT
> style error rather than a net error. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:31: void RecordFileError(int error,
FileErrorSource type);
On 2011/08/16 13:36:02, cbentzel wrote:
> In Win, this is actually a DWORD for error. DWORD is an unsigned long. Yes,
> probably can cast to an int but may be worth some additional consideration.
> 
> errno is an int on the errno.h's I've seen - may be specified by OpenGroup as
> well.

On Win, I mask out the 16 high bits anyway, so it doesn't matter if it's signed
or not.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:134: return kErrorBucketStart + (2 * n) - 1;
 // In gap before the bucket.
On 2011/08/16 13:36:02, cbentzel wrote:
> I'm lost about what this is doing.
> 
> Say I have an error of value 1950
> 
> This will trigger on n = 13 
> 
> Then I return 400 + (2*13) - 1 = 425
> 
> I get that the 400 needs to be greater than 321 - and that you may want to
leave
> a gap.
> 
> What is the 2x multiple for? 
> 
> Note: I also think this going to be very difficult to decipher when looking at
> the histograms. I think it would be much simpler to have two histograms - one
> where error codes are individually bucketed, another where they are bucketed
in
> constant-sized buckets. What do you think?

The 2x multiple is in case we get an error that falls in the gaps between
defined ranges.

In your example, 1950 is such a value, falling between the ranges { 1898, 1938 }
and { 2000, 2024 }.

If you had 1933 instead, it would fall inside the range { 1898, 1938 } and get a
value of 424.

Splitting it into two histograms sounds good.  Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:367: int error = errno;
On 2011/08/16 13:36:02, cbentzel wrote:
> Perhaps a helper function like
> 
> MapAndRecordErrorCode(error, record_uma_)
> 
> to help minimize duplication here.

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:380: if (record_uma_)
On 2011/08/16 13:36:02, cbentzel wrote:
> I don't think it's worth recording these errors related to 
> 
> if (!IsOpen())
> 
> at the top of many of the functions. It's more of a programming error than
> return codes we are seeing from the OS/library calls. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:488: int result = ftruncate(file_, bytes);
On 2011/08/16 13:36:02, cbentzel wrote:
> This existed before - but we may want to make this
HANDLE_EINTR(ftruncate(file_,
> bytes)). It looks like EINTR can be a return value during ftruncate - perhaps
> only when trying to expand the file rather than shrink it.

What does HANDLE_EINTR do?

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:509: record_uma_ = true;
On 2011/08/16 13:36:02, cbentzel wrote:
> Do you need to do the 
> 
> if (async_context_)
>   async_context_->EnableRecording()
> 
> here as well? You have it in the Win version, and then you could replace
passing
> in record_uma to all of the function calls on the async_context_.

Doing something similar now:  Setting it at the time of the asynchronous
invocation.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:117: RecordFileError(error, FILE_ERROR_SOURCE_READ);
On 2011/08/16 13:36:02, cbentzel wrote:
> This could be done for both reads and writes. You probably need to record the
> source in the AsyncContext. 

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:201: if (record_uma_)
On 2011/08/16 13:36:02, cbentzel wrote:
> Again - in this file I would remove the recording of bad IsOpen calls at the
top
> of the functions.

Done.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:337: if (async_context_.get() && error ==
ERROR_IO_PENDING) {
On 2011/08/16 13:36:02, cbentzel wrote:
> So, in the Windows case I _don't_ think it will help to have a helper function
> to MapAndRecordErrorCode() due to all the special cases. 

It's not as useful, but still works.

[cbentzel, 2011-08-18 13:31:27]

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:488: int result = ftruncate(file_, bytes);
On 2011/08/17 20:12:04, ahendrickson wrote:
> On 2011/08/16 13:36:02, cbentzel wrote:
> > This existed before - but we may want to make this
> HANDLE_EINTR(ftruncate(file_,
> > bytes)). It looks like EINTR can be a return value during ftruncate -
perhaps
> > only when trying to expand the file rather than shrink it.
> 
> What does HANDLE_EINTR do?

HANDLE_EINTR re-executes the function if the errno was EINTR. Many POSIX
functions do this if it's possible that they can be stopped early due to an
interrupt. 

I think this is discussed in W. Richard Stevens Unix Network Programming vol. 1.
 There's probably online resources about it as well.

It's probably a good idea to do this in a separate CL, however.

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:509: record_uma_ = true;
On 2011/08/17 20:12:04, ahendrickson wrote:
> On 2011/08/16 13:36:02, cbentzel wrote:
> > Do you need to do the 
> > 
> > if (async_context_)
> >   async_context_->EnableRecording()
> > 
> > here as well? You have it in the Win version, and then you could replace
> passing
> > in record_uma to all of the function calls on the async_context_.
> 
> Doing something similar now:  Setting it at the time of the asynchronous
> invocation.

Yup. After the fact I realized how different the two implementations were. Note:
I'm not a huge fan of how the POSIX implementation uses worker threads to handle
IO. Perhaps we should look at async file IO on Posix. Certainly not in this CL
though.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream.h#newc...
net/base/file_stream.h:17: #include "net/base/file_stream_metrics.h"
This include isn't needed.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream.h#newc...
net/base/file_stream.h:143: int RecordAndMapError(int error, FileErrorSource
source);
This doesn't need to be a member function. It can just be a function defined in
file_stream_posix and file_stream_win. Then you can remove the
file_stream_metrics include.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:35:
UMA_HISTOGRAM_ENUMERATION("FileErrorRange.Open", bucket, max_bucket);
Could you just use UMA_HISTOGRAM_CUSTOM_ENUMERATION here? 

That will automatically take care of figuring out which bucket to use to prevent
you from needing to do that. 

Then, you only need one platform-specific function to return the static ranges. 

For an example, see
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/base/host_resolve...

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
net/base/file_stream_metrics.h:27: // The largest bucket number, plus 1.
Nit: there's usually a new-line between function declaration and the comment
afterwards.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:27: { 383, 387, false },
I really don't understand why you want gaps, as you end up recording these
anyway. Are they unexpected values?

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:73: int error = errno;
Why don't you call RecordAndMapErrorCode(errno, FILE_ERROR_SOURCE_READ,
record_uma) here?

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:75: void set_operation(AsyncOperation operation) {
operation_ = operation; }
Why isn't this just FILE_ERROR_SOURCE? That way you don't have to worry about
the new enum and the mapping of that enum to FILE_ERROR_SOURCE, which is all you
care about anyway.

Also - is it possible for there to be multiple operations on the same
AsyncContext? I don't think there is, but just want to double-check.

[ahendrickson, 2011-08-18 15:56:45]

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/15001/net/base/file_stream_posix....
net/base/file_stream_posix.cc:488: int result = ftruncate(file_, bytes);
On 2011/08/18 13:31:27, cbentzel wrote:
> On 2011/08/17 20:12:04, ahendrickson wrote:
> > On 2011/08/16 13:36:02, cbentzel wrote:
> > > This existed before - but we may want to make this
> > HANDLE_EINTR(ftruncate(file_,
> > > bytes)). It looks like EINTR can be a return value during ftruncate -
> perhaps
> > > only when trying to expand the file rather than shrink it.
> > 
> > What does HANDLE_EINTR do?
> 
> HANDLE_EINTR re-executes the function if the errno was EINTR. Many POSIX
> functions do this if it's possible that they can be stopped early due to an
> interrupt. 
> 
> I think this is discussed in W. Richard Stevens Unix Network Programming vol.
1.
>  There's probably online resources about it as well.
> 
> It's probably a good idea to do this in a separate CL, however.

OK, removed.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream.h#newc...
net/base/file_stream.h:17: #include "net/base/file_stream_metrics.h"
On 2011/08/18 13:31:28, cbentzel wrote:
> This include isn't needed.

Done.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream.h#newc...
net/base/file_stream.h:143: int RecordAndMapError(int error, FileErrorSource
source);
On 2011/08/18 13:31:28, cbentzel wrote:
> This doesn't need to be a member function. It can just be a function defined
in
> file_stream_posix and file_stream_win. Then you can remove the
> file_stream_metrics include.

Done.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:35:
UMA_HISTOGRAM_ENUMERATION("FileErrorRange.Open", bucket, max_bucket);
On 2011/08/18 13:31:28, cbentzel wrote:
> Could you just use UMA_HISTOGRAM_CUSTOM_ENUMERATION here? 
> 
> That will automatically take care of figuring out which bucket to use to
prevent
> you from needing to do that. 
> 
> Then, you only need one platform-specific function to return the static
ranges. 
> 
> For an example, see
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/base/host_resolve...

I don't see that as being any simpler.  The complexity simply goes to creating
the buckets.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
net/base/file_stream_metrics.h:27: // The largest bucket number, plus 1.
On 2011/08/18 13:31:28, cbentzel wrote:
> Nit: there's usually a new-line between function declaration and the comment
> afterwards.

Done.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:27: { 383, 387, false },
On 2011/08/18 13:31:28, cbentzel wrote:
> I really don't understand why you want gaps, as you end up recording these
> anyway. Are they unexpected values?

They would be values that don't appear in WinError.h.

If they happen, I want to be able to record them.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_posix....
net/base/file_stream_posix.cc:73: int error = errno;
On 2011/08/18 13:31:28, cbentzel wrote:
> Why don't you call RecordAndMapErrorCode(errno, FILE_ERROR_SOURCE_READ,
> record_uma) here?

Now that RecordAndMapErrorCode is no longer a member function of FileStream,
that is possible.

Done.

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/7583049/diff/21002/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:75: void set_operation(AsyncOperation operation) {
operation_ = operation; }
On 2011/08/18 13:31:28, cbentzel wrote:
> Why isn't this just FILE_ERROR_SOURCE? That way you don't have to worry about
> the new enum and the mapping of that enum to FILE_ERROR_SOURCE, which is all
you
> care about anyway.
> 
> Also - is it possible for there to be multiple operations on the same
> AsyncContext? I don't think there is, but just want to double-check. 

I believe there is 0 or 1 AsyncContext's per FileStream, depending on how it's
created.  That implies that all operations done on a FileStream (read, write,
seek) can go through the AsyncContext, so multiple operations are possible.

However, I would expect one operation to complete before another one is started.
 What do you think?

Changed operation_ to be a FileErrorSource, and renamed it.

[Randy Smith (Not in Mondays), 2011-08-19 15:08:29]

Unless Andy or Chris have major objections, I'm happy to rely on Chris' review
of this CL.

[cbentzel, 2011-08-23 01:52:02]

Sorry for the delay - I thought I had completed another round of review earlier.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:27: int bucket = GetErrorBucket(error);
Rather than write logic to figure out which bucket to use, you could use

UMA_HISTOGRAM_CUSTOM_ENUMERATION

and then per-platform you will only need something to come up with the bucket
ranges.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:34: UMA_HISTOGRAM_ENUMERATION("FileError.Open",
error, max_error);
This should be 

Net.FileOpen_Error
Net.FileOpen_ErrorRange

or similar, so this is cleaner in the dashboard.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:37: 
Nit: most code I've seen doesn't have newlines between the break; and the next
case, but I think it makes this clearer. It doesn't appear to be against the
style guide either.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White...

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:24: Range kErrorRangeList[] = {
I'll talk with you in person, but I still don't understand why we need gaps. I
think it will simply make the analysis very difficult.

[cbentzel, 2011-08-30 14:48:23]

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:27: int bucket = GetErrorBucket(error);
On 2011/08/23 01:52:02, cbentzel wrote:
> Rather than write logic to figure out which bucket to use, you could use
> 
> UMA_HISTOGRAM_CUSTOM_ENUMERATION
> 
> and then per-platform you will only need something to come up with the bucket
> ranges. 

As we discussed, I'm OK with the approach you took instead of using
UMA_HISTOGRAM_CUSTOM_ENUMERATION.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:24: Range kErrorRangeList[] = {
On 2011/08/23 01:52:02, cbentzel wrote:
> I'll talk with you in person, but I still don't understand why we need gaps. I
> think it will simply make the analysis very difficult.

After chatting, I understand the need for gaps.

I would change this structure to just have min and max. The true/false is not
needed anymore.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:126: for (size_t n = 0; n < kNumErrorRanges;
++n) {
I'm fine with this being a linear search, given how infrequently it will be
used. Might be worth mentioning it, though.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:142: int max_error = 0;
I'd just make this a constant, rather than a function to generate it.

[ahendrickson, 2011-08-31 19:39:52]

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
File net/base/file_stream_metrics.cc (right):

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics.cc:34: UMA_HISTOGRAM_ENUMERATION("FileError.Open",
error, max_error);
On 2011/08/23 01:52:02, cbentzel wrote:
> This should be 
> 
> Net.FileOpen_Error
> Net.FileOpen_ErrorRange
> 
> or similar, so this is cleaner in the dashboard.

Done.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:24: Range kErrorRangeList[] = {
On 2011/08/30 14:48:23, cbentzel wrote:
> On 2011/08/23 01:52:02, cbentzel wrote:
> > I'll talk with you in person, but I still don't understand why we need gaps.
I
> > think it will simply make the analysis very difficult.
> 
> After chatting, I understand the need for gaps.
> 
> I would change this structure to just have min and max. The true/false is not
> needed anymore.

Done.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:126: for (size_t n = 0; n < kNumErrorRanges;
++n) {
On 2011/08/30 14:48:23, cbentzel wrote:
> I'm fine with this being a linear search, given how infrequently it will be
> used. Might be worth mentioning it, though.

Done.

http://codereview.chromium.org/7583049/diff/26001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:142: int max_error = 0;
On 2011/08/30 14:48:23, cbentzel wrote:
> I'd just make this a constant, rather than a function to generate it.

For Windows, I'd like to be able to change it if we need to, and it should be
based on the values in kErrorRangeList.

Changed.

[cbentzel, 2011-08-31 19:53:23]

LGTM

http://codereview.chromium.org/7583049/diff/33001/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/7583049/diff/33001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:24: // UMA error statistics gathering.
Perhaps these should be in a nested file_stream_metrics namespace in net. They
have pretty generic names for top-level functions.

http://codereview.chromium.org/7583049/diff/33001/net/base/file_stream_metric...
File net/base/file_stream_metrics_win.cc (right):

http://codereview.chromium.org/7583049/diff/33001/net/base/file_stream_metric...
net/base/file_stream_metrics_win.cc:129: return (2 * (n + 1)) - 1;  // In gap
before the range.
This could be 2 * n and the "in-range" as 2*n+1 to not leave a hole at 0. Not a
huge deal.

[Randy Smith (Not in Mondays), 2011-09-01 16:48:27]

Rubber-stamp LGTM based on Chris' review.

[ahendrickson, 2011-09-01 18:01:20]

http://codereview.chromium.org/7583049/diff/33001/net/base/file_stream_metrics.h
File net/base/file_stream_metrics.h (right):

http://codereview.chromium.org/7583049/diff/33001/net/base/file_stream_metric...
net/base/file_stream_metrics.h:24: // UMA error statistics gathering.
On 2011/08/31 19:53:23, cbentzel wrote:
> Perhaps these should be in a nested file_stream_metrics namespace in net. They
> have pretty generic names for top-level functions.

I've had objections to using nested namespaces here, so I've renamed the
functions to be less generic.

[cbentzel, 2011-09-01 18:05:49]

Even more LGTM

You can remove the BUG=NONE and TEST=None in the description

[commit-bot: I haz the power, 2011-09-01 19:39:44]

Try job failure for 7583049-39001 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-09-02 03:54:31]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac, revision is 99299, job name
was 7583049-39001 (previous was lost) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-03 21:58:48]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac, revision is 99576, job name
was 7583049-46001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-09-07 22:42:19]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac, revision is 100021, job name
was 7583049-52001 (retry) (retry) (retry) (retry).

[cbentzel, 2011-09-20 18:26:49]

On 2011/09/07 22:42:19, I haz the power (commit-bot) wrote:
> The commit queue went berserk retrying too often for a
> seemingly flaky test. Builder is mac, revision is 100021, job name
> was 7583049-52001 (retry) (retry) (retry) (retry).

Are you planning on relanding this change?

[ahendrickson, 2011-09-20 18:43:32]

Yes, once I figure out what the problem is.

Andy

On 2011/09/20 18:26:49, cbentzel wrote:
> On 2011/09/07 22:42:19, I haz the power (commit-bot) wrote:
> > The commit queue went berserk retrying too often for a
> > seemingly flaky test. Builder is mac, revision is 100021, job name
> > was 7583049-52001 (retry) (retry) (retry) (retry).
> 
> Are you planning on relanding this change?

[ahendrickson, 2011-09-20 22:24:31]

http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/50257/step...

There are a lot of files open when ExtensionServiceTest.PackExtensionOpenSSLKey
runs, and it runs out of file handles.

It looks like the previous test, ExtensionServiceTest.PackPunctuatedExtension is
the one that opens them.  Could this be due to running the tests in parallel?

Andy

On 2011/09/20 18:43:32, ahendrickson wrote:
> Yes, once I figure out what the problem is.
> 
> Andy
> 
> On 2011/09/20 18:26:49, cbentzel wrote:
> > On 2011/09/07 22:42:19, I haz the power (commit-bot) wrote:
> > > The commit queue went berserk retrying too often for a
> > > seemingly flaky test. Builder is mac, revision is 100021, job name
> > > was 7583049-52001 (retry) (retry) (retry) (retry).
> > 
> > Are you planning on relanding this change?

[commit-bot: I haz the power, 2011-09-23 16:33:27]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/7583049/78001

[commit-bot: I haz the power, 2011-09-23 20:38:23]

Change committed as 102560