Integrate ETW with VLOG logging.

Integrate ETW with VLOG logging.
This change extends the log severities into the negative for VLOG verbosities.
It also keeps track of file/line information and passes it to any registered log handler.
The log level is naturally controlled by translating ETW log levels to the severities.

BUG=none
TEST=none


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

[Sigurður Ásgeirsson, 2010-10-29 20:44:02]

Hi Brett, Fred,

please check this out. This change goes most of the way toward integrating ETW
to VLOG logging.
This associated the VLOG log verbosity to a separate ETW provider to allow
control of VLOG verbosity individual of LOG verbosity. 
It also carries the full file name to the VLOG hook, in order to allow filtering
on the full path at the sink.
I still need to figure out how to not leak the Vlog object, maybe it makes sense
to make the VLOG max_level a static variable in logging.cc?

[brettw, 2010-10-29 21:28:20]

I realized I don't know a lot about this ETW stuff and I'm told Ricardo did the
initial review. Ricardo, can you review this as well?

[akalin, 2010-11-01 20:48:32]

http://codereview.chromium.org/4164011/diff/9001/10001
File base/logging.cc (right):

http://codereview.chromium.org/4164011/diff/9001/10001#newcode395
base/logging.cc:395: if (g_vlog_info == NULL)
is this necessary since you're always initializing g_vlog_info?

http://codereview.chromium.org/4164011/diff/9001/10001#newcode404
base/logging.cc:404: if (g_vlog_info == NULL)
This, too.

http://codereview.chromium.org/4164011/diff/9001/10002
File base/logging.h (right):

http://codereview.chromium.org/4164011/diff/9001/10002#newcode214
base/logging.h:214: // Returns the previous level, or -1 if logging is not
initialized.
I think it's better if Set() just returns void, esp. since we're always
initializing vlog so this will never return -1.

http://codereview.chromium.org/4164011/diff/9001/10002#newcode789
base/logging.h:789: // The file passed in to VLOG messages.
If this will eventually be used for non-VLOG messages, may as well move it down
with the rest of the non-vlog vars.

[rvargas (doing something else), 2010-11-01 21:19:01]

So the idea is that the user will add a vlog level and module pattern the usual
way and then we can use etw to modify the level... but we cannot modify the
module pattern.

A long time ago I was playing with adding multiple providers so that different
parts of the code could be controlled independently. (I just uploaded part of
that here http://codereview.chromium.org/4108016/show). I was not really happy
with the result, and got too busy with other stuff, but there's a few things in
common with this CL... and then VLOG came along.

Is it really worth it to add another provider just to control the level knowing
that we are limiting that number to exactly 2 (and req

[brettw, 2010-11-01 21:24:55]

On Mon, Nov 1, 2010 at 2:19 PM,  <rvargas@chromium.org> wrote:
> So the idea is that the user will add a vlog level and module pattern the
> usual
> way and then we can use etw to modify the level... but we cannot modify the
> module pattern.
>
> A long time ago I was playing with adding multiple providers so that
> different
> parts of the code could be controlled independently. (I just uploaded part
> of
> that here http://codereview.chromium.org/4108016/show). I was not really
> happy
> with the result, and got too busy with other stuff, but there's a few things
> in
> common with this CL... and then VLOG came along.
>
> Is it really worth it to add another provider just to control the level
> knowing
> that we are limiting that number to exactly 2 (and req

I agree, I think it's kind of a lot of complexity to add the ability
to control all this stuff via ETW. It seems to me that this is the
kind of thing that only 3 people even know exists so imposing extra
complexity on the already byzantine logging system seems like a poor
trade-off.

Brett

[rvargas (doing something else), 2010-11-01 21:38:29]

(take number 2)

So the idea is that the user will add a vlog level and module pattern the usual
way and then we can use etw to modify the level... but we cannot modify the
module pattern.

A long time ago I was playing with adding multiple providers so that different
parts of the code could be controlled independently. (I just uploaded part of
that here http://codereview.chromium.org/4108016/show). I was not really happy
with the result, and got too busy with other stuff, but there's a few things in
common with this CL... and then VLOG came along.

Is it really worth it to add another provider just to control the level knowing
that we are limiting that number to exactly 2 (and require restarting to change
the command line anyway to add another file)?

If we are adding another handler to filter VLOG messages, should't we just use
the same handler for regular messages so that they are also better formatted
(file and line number on separate fields of the message)?

In other words, part of me wants to have more options and the ability to control
everything from the etw controller (so this CL doesn't look general enough). But
I'm not that sure how useful that would be as opposed to just wire all messages
directly.

Just thinking aloud.

[Sigurður Ásgeirsson, 2010-11-02 00:47:18]

On Mon, Nov 1, 2010 at 5:38 PM, <rvargas@chromium.org> wrote:

> (take number 2)
>
>
> So the idea is that the user will add a vlog level and module pattern the
> usual
> way and then we can use etw to modify the level... but we cannot modify the
> module pattern.
>
> Well, the idea here is to allow cranking the VLOG level up or down, and to
simply funnel *all* VLOGs out to the destination, then filter at the sink
(e.g. in Sawbuck). With the log volumes I've seen prior to the VLOG change,
this is pretty reasonable.


> A long time ago I was playing with adding multiple providers so that
> different
> parts of the code could be controlled independently. (I just uploaded part
> of
> that here http://codereview.chromium.org/4108016/show). I was not really
> happy
> with the result, and got too busy with other stuff, but there's a few
> things in
> common with this CL... and then VLOG came along.
>
> Is it really worth it to add another provider just to control the level
> knowing
> that we are limiting that number to exactly 2 (and require restarting to
> change
> the command line anyway to add another file)?
>

That's a good question. Registering a second provider is not a big deal in
terms of the code or overhead, but the configuration and use of this going
to get to be a bit of a mess.
I suppose I could control VLOG verbosity by treating the VLOG levels as log
levels above INFO. Alternatively I could carve out some enable flags.
You have a preference for one or the other?


>
> If we are adding another handler to filter VLOG messages, should't we just
> use
> the same handler for regular messages so that they are also better
> formatted
> (file and line number on separate fields of the message)?
>

Yups, I'd like to do that in a later change, however, as there are a few
users of the current hook.

>
> In other words, part of me wants to have more options and the ability to
> control
> everything from the etw controller (so this CL doesn't look general
> enough). But
> I'm not that sure how useful that would be as opposed to just wire all
> messages
> directly.
>
> Just thinking aloud.
>
>
> http://codereview.chromium.org/4164011/show
>

[Sigurður Ásgeirsson, 2010-11-02 00:48:48]

On Mon, Nov 1, 2010 at 8:47 PM, Sigurður Ásgeirsson <siggi@chromium.org>wrote:

> On Mon, Nov 1, 2010 at 5:38 PM, <rvargas@chromium.org> wrote:
>
>> (take number 2)
>>
>>
>> So the idea is that the user will add a vlog level and module pattern the
>> usual
>> way and then we can use etw to modify the level... but we cannot modify
>> the
>> module pattern.
>>
>> Well, the idea here is to allow cranking the VLOG level up or down, and to
> simply funnel *all* VLOGs out to the destination, then filter at the sink
> (e.g. in Sawbuck). With the log volumes I've seen prior to the VLOG change,
> this is pretty reasonable.
>

Note that for Chrome Frame and other plugin code, it's not an option to set
the filter criteria by command line flag in any case.


>
>
>> A long time ago I was playing with adding multiple providers so that
>> different
>> parts of the code could be controlled independently. (I just uploaded part
>> of
>> that here http://codereview.chromium.org/4108016/show). I was not really
>> happy
>> with the result, and got too busy with other stuff, but there's a few
>> things in
>> common with this CL... and then VLOG came along.
>>
>> Is it really worth it to add another provider just to control the level
>> knowing
>> that we are limiting that number to exactly 2 (and require restarting to
>> change
>> the command line anyway to add another file)?
>>
>
> That's a good question. Registering a second provider is not a big deal in
> terms of the code or overhead, but the configuration and use of this going
> to get to be a bit of a mess.
> I suppose I could control VLOG verbosity by treating the VLOG levels as log
> levels above INFO. Alternatively I could carve out some enable flags.
> You have a preference for one or the other?
>
>
>>
>> If we are adding another handler to filter VLOG messages, should't we just
>> use
>> the same handler for regular messages so that they are also better
>> formatted
>> (file and line number on separate fields of the message)?
>>
>
> Yups, I'd like to do that in a later change, however, as there are a few
> users of the current hook.
>
>>
>> In other words, part of me wants to have more options and the ability to
>> control
>> everything from the etw controller (so this CL doesn't look general
>> enough). But
>> I'm not that sure how useful that would be as opposed to just wire all
>> messages
>> directly.
>>
>> Just thinking aloud.
>>
>>
>> http://codereview.chromium.org/4164011/show
>>
>
>

[Jói, 2010-11-02 01:04:30]

> >> Well, the idea here is to allow cranking the VLOG level up or down, and to
> > simply funnel *all* VLOGs out to the destination, then filter at the sink
> > (e.g. in Sawbuck). With the log volumes I've seen prior to the VLOG change,
> > this is pretty reasonable.
> >
> 
> Note that for Chrome Frame and other plugin code, it's not an option to set
> the filter criteria by command line flag in any case.

That sounds reasonable to me, and this would be super useful for Chrome Frame
development, hope we can find a solution here that everyone is OK with.

Cheers,
Jói



On 2010/11/02 00:48:48, Ruðrugis wrote:
> On Mon, Nov 1, 2010 at 8:47 PM, Sigurður Ásgeirsson <siggi@chromium.org>wrote:
> 
> > On Mon, Nov 1, 2010 at 5:38 PM, <mailto:rvargas@chromium.org> wrote:
> >
> >> (take number 2)
> >>
> >>
> >> So the idea is that the user will add a vlog level and module pattern the
> >> usual
> >> way and then we can use etw to modify the level... but we cannot modify
> >> the
> >> module pattern.
> >>
> >> Well, the idea here is to allow cranking the VLOG level up or down, and to
> > simply funnel *all* VLOGs out to the destination, then filter at the sink
> > (e.g. in Sawbuck). With the log volumes I've seen prior to the VLOG change,
> > this is pretty reasonable.
> >
> 
> Note that for Chrome Frame and other plugin code, it's not an option to set
> the filter criteria by command line flag in any case.
> 
> 
> >
> >
> >> A long time ago I was playing with adding multiple providers so that
> >> different
> >> parts of the code could be controlled independently. (I just uploaded part
> >> of
> >> that here http://codereview.chromium.org/4108016/show). I was not really
> >> happy
> >> with the result, and got too busy with other stuff, but there's a few
> >> things in
> >> common with this CL... and then VLOG came along.
> >>
> >> Is it really worth it to add another provider just to control the level
> >> knowing
> >> that we are limiting that number to exactly 2 (and require restarting to
> >> change
> >> the command line anyway to add another file)?
> >>
> >
> > That's a good question. Registering a second provider is not a big deal in
> > terms of the code or overhead, but the configuration and use of this going
> > to get to be a bit of a mess.
> > I suppose I could control VLOG verbosity by treating the VLOG levels as log
> > levels above INFO. Alternatively I could carve out some enable flags.
> > You have a preference for one or the other?
> >
> >
> >>
> >> If we are adding another handler to filter VLOG messages, should't we just
> >> use
> >> the same handler for regular messages so that they are also better
> >> formatted
> >> (file and line number on separate fields of the message)?
> >>
> >
> > Yups, I'd like to do that in a later change, however, as there are a few
> > users of the current hook.
> >
> >>
> >> In other words, part of me wants to have more options and the ability to
> >> control
> >> everything from the etw controller (so this CL doesn't look general
> >> enough). But
> >> I'm not that sure how useful that would be as opposed to just wire all
> >> messages
> >> directly.
> >>
> >> Just thinking aloud.
> >>
> >>
> >> http://codereview.chromium.org/4164011/show
> >>
> >
> >
>

[rvargas (doing something else), 2010-11-02 01:37:54]

> I suppose I could control VLOG verbosity by treating the VLOG levels as log
> levels above INFO. Alternatively I could carve out some enable flags.
> You have a preference for one or the other?
> 

I guess that by definition verbose logging levels go after INFO.

Note that today we already have way more log messages when --v is enabled.

[akalin, 2010-11-02 01:39:54]

I also think that treating verbose logging levels as below INFO is the
most parsimonious way to do this.  That is, a log_level of x < 0
corresponds to a verbosity level of -x.

On Mon, Nov 1, 2010 at 6:37 PM,  <rvargas@chromium.org> wrote:
>> I suppose I could control VLOG verbosity by treating the VLOG levels as
>> log
>> levels above INFO. Alternatively I could carve out some enable flags.
>> You have a preference for one or the other?
>
>
> I guess that by definition verbose logging levels go after INFO.
>
> Note that today we already have way more log messages when --v is enabled.
>
> http://codereview.chromium.org/4164011/show
>

[tommi (sloooow) - chröme, 2010-11-02 01:45:28]

I took a quick look. +1 on usefulness for CF and likely other areas of the code
such as the installer.  lgtm as far as I'm concerned and a couple of comments.

http://codereview.chromium.org/4164011/diff/9001/10001
File base/logging.cc (right):

http://codereview.chromium.org/4164011/diff/9001/10001#newcode352
base/logging.cc:352: // TODO(siggi): This pointer leaks, which is unacceptable
for
File a bug for this or perhaps fix it now?

http://codereview.chromium.org/4164011/diff/9001/10003
File base/logging_win.cc (right):

http://codereview.chromium.org/4164011/diff/9001/10003#newcode174
base/logging_win.cc:174: event.SetField(3, strlen(file) + 1, file);
since this is windows only, perhaps just use lstrlenA? I'm assuming strlen is
the reason for the file == NULL check above, right?

http://codereview.chromium.org/4164011/diff/9001/10004
File base/logging_win.h (left):

http://codereview.chromium.org/4164011/diff/9001/10004#oldcode56
base/logging_win.h:56: DISALLOW_COPY_AND_ASSIGN(LogEventProvider);
did you mean to remove this too?

[brettw, 2010-11-02 04:31:53]

I'd like to get a better high-level understanding of what's going on here. Who
uses ETW and for what? What specifically does this "provider control over vlog"
enable? It seems to add a surprising amount of extra stuff to LogMessage.

[Sigurður Ásgeirsson, 2010-11-02 12:56:10]

On Tue, Nov 2, 2010 at 12:31 AM, <brettw@chromium.org> wrote:

> I'd like to get a better high-level understanding of what's going on here.
> Who
> uses ETW and for what? What specifically does this "provider control over
> vlog"
> enable? It seems to add a surprising amount of extra stuff to LogMessage.
>
> ETW is used a lot by Chrome Frame developers and testers, and I hope by
Chrome Windows developers as well.
If you want to try it out, you can download the Sawbuck installer [
http://code.google.com/p/sawbuck/downloads/detail?name=sawbuck-0.6.6.0-releas....
After you install this, you need to restart your Chrome, and then you'll be
able to sink logs to the Sawbuck GUI in realtime.
The GUI looks something like this:

My change aims to do two things:
- Support VLOGging to ETW, GCF can't currently use the VLOG macros, as
there's no way to control VLOG levels through ETW.
- Funnel the full file names through to ETW, to allow better filtering, e.g.
by path, directory name or such.


> http://codereview.chromium.org/4164011/show
>

[Jói, 2010-11-02 12:58:25]

+1 to trying out Sawbuck if you're developing Chrome on Windows - it's awesome.

2010/11/2 Sigurður Ásgeirsson <siggi@chromium.org>
>
> On Tue, Nov 2, 2010 at 12:31 AM, <brettw@chromium.org> wrote:
>>
>> I'd like to get a better high-level understanding of what's going on here.
Who
>> uses ETW and for what? What specifically does this "provider control over
vlog"
>> enable? It seems to add a surprising amount of extra stuff to LogMessage.
>
> ETW is used a lot by Chrome Frame developers and testers, and I hope by Chrome
Windows developers as well.
> If you want to try it out, you can download the Sawbuck installer
[http://code.google.com/p/sawbuck/downloads/detail?name=sawbuck-0.6.6.0-release.msi].
After you install this, you need to restart your Chrome, and then you'll be able
to sink logs to the Sawbuck GUI in realtime.
> The GUI looks something like this:
> My change aims to do two things:
> - Support VLOGging to ETW, GCF can't currently use the VLOG macros, as there's
no way to control VLOG levels through ETW.
> - Funnel the full file names through to ETW, to allow better filtering, e.g.
by path, directory name or such.
>>
>> http://codereview.chromium.org/4164011/show
>

[amit, 2010-11-02 16:49:10]

+1 for ETW (OS supported, low overhead) way of logging and sawbuck is
awesome.

Siggi, have you considered impact on startup and logging in sandbox?

On Tue, Nov 2, 2010 at 5:57 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> +1 to trying out Sawbuck if you're developing Chrome on Windows - it's
> awesome.
>
> 2010/11/2 Sigurður Ásgeirsson <siggi@chromium.org>
> >
> > On Tue, Nov 2, 2010 at 12:31 AM, <brettw@chromium.org> wrote:
> >>
> >> I'd like to get a better high-level understanding of what's going on
> here. Who
> >> uses ETW and for what? What specifically does this "provider control
> over vlog"
> >> enable? It seems to add a surprising amount of extra stuff to
> LogMessage.
> >
> > ETW is used a lot by Chrome Frame developers and testers, and I hope by
> Chrome Windows developers as well.
> > If you want to try it out, you can download the Sawbuck installer [
>
http://code.google.com/p/sawbuck/downloads/detail?name=sawbuck-0.6.6.0-releas....
> After you install this, you need to restart your Chrome, and then you'll be
> able to sink logs to the Sawbuck GUI in realtime.
> > The GUI looks something like this:
> > My change aims to do two things:
> > - Support VLOGging to ETW, GCF can't currently use the VLOG macros, as
> there's no way to control VLOG levels through ETW.
> > - Funnel the full file names through to ETW, to allow better filtering,
> e.g. by path, directory name or such.
> >>
> >> http://codereview.chromium.org/4164011/show
> >
>

[Sigurður Ásgeirsson, 2010-11-02 17:36:12]

On Tue, Nov 2, 2010 at 12:48 PM, Amit Joshi <amit@chromium.org> wrote:

> +1 for ETW (OS supported, low overhead) way of logging and sawbuck is
> awesome.
>
> Siggi, have you considered impact on startup and logging in sandbox?
>

The startup cost is unfortunately non-trivial on XP, which is why the ETW
logging is enabled behind an environment variable. Later Windowsen seem to
have reduced the startup cost to ETW to the point where it's unnoticeable.
There's no problem with logging in the sandbox, AFAIK.


>
> On Tue, Nov 2, 2010 at 5:57 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> +1 to trying out Sawbuck if you're developing Chrome on Windows - it's
>> awesome.
>>
>> 2010/11/2 Sigurður Ásgeirsson <siggi@chromium.org>
>> >
>> > On Tue, Nov 2, 2010 at 12:31 AM, <brettw@chromium.org> wrote:
>> >>
>> >> I'd like to get a better high-level understanding of what's going on
>> here. Who
>> >> uses ETW and for what? What specifically does this "provider control
>> over vlog"
>> >> enable? It seems to add a surprising amount of extra stuff to
>> LogMessage.
>> >
>> > ETW is used a lot by Chrome Frame developers and testers, and I hope by
>> Chrome Windows developers as well.
>> > If you want to try it out, you can download the Sawbuck installer [
>>
http://code.google.com/p/sawbuck/downloads/detail?name=sawbuck-0.6.6.0-releas....
>> After you install this, you need to restart your Chrome, and then you'll be
>> able to sink logs to the Sawbuck GUI in realtime.
>> > The GUI looks something like this:
>> > My change aims to do two things:
>> > - Support VLOGging to ETW, GCF can't currently use the VLOG macros, as
>> there's no way to control VLOG levels through ETW.
>> > - Funnel the full file names through to ETW, to allow better filtering,
>> e.g. by path, directory name or such.
>> >>
>> >> http://codereview.chromium.org/4164011/show
>> >
>>
>
>

[Sigurður Ásgeirsson, 2010-11-02 21:14:47]

Hi guys,

I reworked the change to extend the log severities into the negative for VLOG
verbosities and eliminated the second ETW provider. 
The change has a second "detail" log handler that is passed the file/line
information, but I'd like to eliminate that and just change the signature of the
current log handler - unless anyone objects.

[rvargas (doing something else), 2010-11-02 22:59:23]

http://codereview.chromium.org/4164011/diff/38001/39002
File base/logging.h (right):

http://codereview.chromium.org/4164011/diff/38001/39002#newcode278
base/logging.h:278: const LogSeverity LOG_NUM_SEVERITIES = 5;
Maybe comment that this is just to index the array of names.

http://codereview.chromium.org/4164011/diff/38001/39003
File base/logging_win.cc (right):

http://codereview.chromium.org/4164011/diff/38001/39003#newcode64
base/logging_win.cc:64: if (provider->enable_flags() &
ENABLE_STACK_TRACE_CAPTURE)
I think it is better to use a separate message type that doesn't have a stack
dump.

http://codereview.chromium.org/4164011/diff/38001/39003#newcode72
base/logging_win.cc:72: event.SetField(0, sizeof(depth), &depth);
It would be nice to change the order of the fields so that the stack dump is at
the end of the message. That way, a regular tool can be used to parse the
messages correctly (up to the stack). I guess it would be enough to have a
separate message type and disable stack collection at runtime...

http://codereview.chromium.org/4164011/diff/38001/39003#newcode85
base/logging_win.cc:85: if (severity < provider->old_log_level_)
Why this change?

http://codereview.chromium.org/4164011/diff/38001/39004
File base/logging_win.h (right):

http://codereview.chromium.org/4164011/diff/38001/39004#newcode35
base/logging_win.h:35: //  a stack trace,
Don't we need another message type for a message with line number and no stack
info?.

[Sigurður Ásgeirsson, 2010-11-03 20:50:13]

Hi guys,

This change:

1. switches to the new handler signature.
2. adds another enable bit, and reinstates the text
   only message format under that bit.
3. Passes all the Vlog unittests, without affecting
   global log volume.

please take another look.

http://codereview.chromium.org/4164011/diff/9001/10001
File base/logging.cc (right):

http://codereview.chromium.org/4164011/diff/9001/10001#newcode352
base/logging.cc:352: // TODO(siggi): This pointer leaks, which is unacceptable
for
On 2010/11/02 01:45:29, tommi wrote:
> File a bug for this or perhaps fix it now?

Done.

http://codereview.chromium.org/4164011/diff/9001/10001#newcode395
base/logging.cc:395: if (g_vlog_info == NULL)
On 2010/11/01 20:48:32, akalin wrote:
> is this necessary since you're always initializing g_vlog_info?

Redundant with later changes.

http://codereview.chromium.org/4164011/diff/9001/10001#newcode404
base/logging.cc:404: if (g_vlog_info == NULL)
On 2010/11/01 20:48:32, akalin wrote:
> This, too.

And this - in principle, you'd want the logging machinery to be robust to
logging before initialization, however.

http://codereview.chromium.org/4164011/diff/9001/10003
File base/logging_win.cc (right):

http://codereview.chromium.org/4164011/diff/9001/10003#newcode174
base/logging_win.cc:174: event.SetField(3, strlen(file) + 1, file);
On 2010/11/02 01:45:29, tommi wrote:
> since this is windows only, perhaps just use lstrlenA? I'm assuming strlen is
> the reason for the file == NULL check above, right?

No, I always need a valid string, because the zero terminator (if nothing else)
goes into the log message.

http://codereview.chromium.org/4164011/diff/9001/10004
File base/logging_win.h (left):

http://codereview.chromium.org/4164011/diff/9001/10004#oldcode56
base/logging_win.h:56: DISALLOW_COPY_AND_ASSIGN(LogEventProvider);
On 2010/11/02 01:45:29, tommi wrote:
> did you mean to remove this too?

Done.

http://codereview.chromium.org/4164011/diff/38001/39002
File base/logging.h (right):

http://codereview.chromium.org/4164011/diff/38001/39002#newcode278
base/logging.h:278: const LogSeverity LOG_NUM_SEVERITIES = 5;
On 2010/11/02 22:59:23, rvargas wrote:
> Maybe comment that this is just to index the array of names.

Done.

http://codereview.chromium.org/4164011/diff/38001/39003
File base/logging_win.cc (right):

http://codereview.chromium.org/4164011/diff/38001/39003#newcode64
base/logging_win.cc:64: if (provider->enable_flags() &
ENABLE_STACK_TRACE_CAPTURE)
On 2010/11/02 22:59:23, rvargas wrote:
> I think it is better to use a separate message type that doesn't have a stack
> dump.

Done.

http://codereview.chromium.org/4164011/diff/38001/39003#newcode72
base/logging_win.cc:72: event.SetField(0, sizeof(depth), &depth);
On 2010/11/02 22:59:23, rvargas wrote:
> It would be nice to change the order of the fields so that the stack dump is
at
> the end of the message. That way, a regular tool can be used to parse the
> messages correctly (up to the stack). I guess it would be enough to have a
> separate message type and disable stack collection at runtime...

Fields are generally ordered this way in the system's logs, e.g. with the
variable length stuff at the end. This means you get aligned data at the front,
and I'd be hesitant to change that.
When stack collection is disabled, you can parse the message by accounting for
the stack as a single long.

http://codereview.chromium.org/4164011/diff/38001/39003#newcode85
base/logging_win.cc:85: if (severity < provider->old_log_level_)
On 2010/11/02 22:59:23, rvargas wrote:
> Why this change?

This was broken before. Verbosity increases with decreased severity values, so
before this would quench more severe messages than the old log level when ETW
logging was enabled. E.g. if the previous level was WARNING (3), and got bumped
up to INFO (0), then we'd return true for messages logged with WARNING and FATAL
severities and quench them.
Thankfully there's an early return up above, so this only occurs while ETW
logging is enabled.

http://codereview.chromium.org/4164011/diff/38001/39004
File base/logging_win.h (right):

http://codereview.chromium.org/4164011/diff/38001/39004#newcode35
base/logging_win.h:35: //  a stack trace,
On 2010/11/02 22:59:23, rvargas wrote:
> Don't we need another message type for a message with line number and no stack
> info?.

I don't think it's a good idea to create formats for every permutation of
present fields. Turning off stack traces leaves only the count in the record,
and 4 bytes is negligible. 
Also, you can parse those records by accounting for the stack trace as a single
long when stack tracing is turned off.

[akalin, 2010-11-03 21:03:18]

I was discussing with siggi re. the interaction with vloglevel and minloglevel. 
We came to the conclusion that the hierarchy goes something like FATAL, ERROR,
.. INFO = 0, v1 = -1, v2 = -2, etc.  That is, --v=n should correspond to a
logging level of -n.

However, the current implementation stores minloglevel separately from the
vloglevel, which isn't ideal.  This CL remedies that somewhat by passing in a
pointer to the minloglevel to VlogInfo, but I think the best way is to make
GetVlogLevel() take in a 'default' level (i.e., -minloglevel), effectively
turning the VlogInfo class into a VmoduleInfo class.

siggi@, do you want to make this change in this CL?  Alternatively, I could do
the vloglevel/minloglevel integration in another CL so as not to enlarge this CL
even more.

[akalin, 2010-11-03 21:06:21]

Further discussion with siggi@, I'll make the changes described below myself,
but this CL can go in as-is.

so, LGTM (the vlog stuff at least).

On 2010/11/03 21:03:18, akalin wrote:
> I was discussing with siggi re. the interaction with vloglevel and
minloglevel. 
> We came to the conclusion that the hierarchy goes something like FATAL, ERROR,
> .. INFO = 0, v1 = -1, v2 = -2, etc.  That is, --v=n should correspond to a
> logging level of -n.
> 
> However, the current implementation stores minloglevel separately from the
> vloglevel, which isn't ideal.  This CL remedies that somewhat by passing in a
> pointer to the minloglevel to VlogInfo, but I think the best way is to make
> GetVlogLevel() take in a 'default' level (i.e., -minloglevel), effectively
> turning the VlogInfo class into a VmoduleInfo class.
> 
> siggi@, do you want to make this change in this CL?  Alternatively, I could do
> the vloglevel/minloglevel integration in another CL so as not to enlarge this
CL
> even more.

[akalin, 2010-11-03 21:11:59]

Actually, one more thing.

http://codereview.chromium.org/4164011/diff/61001/62001
File base/logging.cc (right):

http://codereview.chromium.org/4164011/diff/61001/62001#newcode386
base/logging.cc:386: min_log_level = level;
I think this is a merge error.

[Sigurður Ásgeirsson, 2010-11-03 21:12:02]

Thanks - I need to do a little more testing, but otherwise I think this is
good to go. Anyone else have opinions to the contrary?

On Wed, Nov 3, 2010 at 5:06 PM, <akalin@chromium.org> wrote:

> Further discussion with siggi@, I'll make the changes described below
> myself,
> but this CL can go in as-is.
>
> so, LGTM (the vlog stuff at least).
>
>
> On 2010/11/03 21:03:18, akalin wrote:
>
>> I was discussing with siggi re. the interaction with vloglevel and
>>
> minloglevel.
>
>> We came to the conclusion that the hierarchy goes something like FATAL,
>> ERROR,
>> .. INFO = 0, v1 = -1, v2 = -2, etc.  That is, --v=n should correspond to a
>> logging level of -n.
>>
>
>  However, the current implementation stores minloglevel separately from the
>> vloglevel, which isn't ideal.  This CL remedies that somewhat by passing
>> in a
>> pointer to the minloglevel to VlogInfo, but I think the best way is to
>> make
>> GetVlogLevel() take in a 'default' level (i.e., -minloglevel), effectively
>> turning the VlogInfo class into a VmoduleInfo class.
>>
>
>  siggi@, do you want to make this change in this CL?  Alternatively, I
>> could do
>> the vloglevel/minloglevel integration in another CL so as not to enlarge
>> this
>>
> CL
>
>> even more.
>>
>
>
>
> http://codereview.chromium.org/4164011/show
>

[Sigurður Ásgeirsson, 2010-11-03 21:24:06]

http://codereview.chromium.org/4164011/diff/61001/62001
File base/logging.cc (right):

http://codereview.chromium.org/4164011/diff/61001/62001#newcode386
base/logging.cc:386: min_log_level = level;
On 2010/11/03 21:11:59, akalin wrote:
> I think this is a merge error.

Ah, good catch, I thought I'd fixed that. 
Thanks!

[akalin, 2010-11-03 21:27:46]

LGTM

On 2010/11/03 21:24:06, Ruðrugis wrote:
> http://codereview.chromium.org/4164011/diff/61001/62001
> File base/logging.cc (right):
> 
> http://codereview.chromium.org/4164011/diff/61001/62001#newcode386
> base/logging.cc:386: min_log_level = level;
> On 2010/11/03 21:11:59, akalin wrote:
> > I think this is a merge error.
> 
> Ah, good catch, I thought I'd fixed that. 
> Thanks!

[rvargas (doing something else), 2010-11-03 21:44:48]

LGTM

[rvargas (doing something else), 2010-11-04 01:08:48]

http://codereview.chromium.org/4164011/diff/38001/39003
File base/logging_win.cc (right):

http://codereview.chromium.org/4164011/diff/38001/39003#newcode72
base/logging_win.cc:72: event.SetField(0, sizeof(depth), &depth);
> Fields are generally ordered this way in the system's logs, e.g. with the
> variable length stuff at the end. This means you get aligned data at the
front,
> and I'd be hesitant to change that.

Well... 3 out of the 5 fields here have variable size so it is hard to have the
front aligned (or "the" variable field at the end)... but I can see your point
with very long messages.

[Sigurður Ásgeirsson, 2010-11-04 13:33:05]

My apologies, I missed vlog.cc and the unittest from the previous upload. Please
take one last look.

[Sigurður Ásgeirsson, 2010-11-04 14:53:54]

On 2010/11/04 13:33:05, Ruðrugis wrote:
> My apologies, I missed vlog.cc and the unittest from the previous upload.
Please
> take one last look.

Also fixed other users of the log message filter, now passes trybots on
win,mac,linux and linux_chromeos.

[tommi (sloooow) - chröme, 2010-11-04 15:50:06]

lgtm

[Sigurður Ásgeirsson, 2010-11-04 15:54:29]

Thanks, submitting.

On Thu, Nov 4, 2010 at 11:50 AM, <tommi@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.org/4164011/show
>