Store log messages in global activity tracker.

Store log messages in global activity tracker.

BUG=620813
Committed: https://crrev.com/7a30eb4d95f6ab0fa7e6692be3a8ba5cb4474760
Cr-Commit-Position: refs/heads/master@{#436027}

[bcwhite, 2016-12-02 15:56:49]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2016-12-02 15:57:11]

bcwhite@chromium.org changed reviewers:
+ manzagop@chromium.org

[commit-bot: I haz the power, 2016-12-02 15:57:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 18:05:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 18:05:58]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2016-12-02 19:10:41]

Great! lgtm % the question about truncation.

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker.cc
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:862: PersistentMemoryAllocator::Reference ref =
Wdyt of setting a max size that we'll truncate to? Even though we only log fatal
messages, we want to make sure the message 1) fits 2) doesn't blow too much
space if there's something else to log.

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:863: allocator_->Allocate(message.size() + 1,
kTypeIdGlobalLogMessage);
The extra character is to ensure null termination, and we know the memory we get
is already null, right? Worth mentioning or is that a property of Allocate?

[bcwhite, 2016-12-02 19:28:07]

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker.cc
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:862: PersistentMemoryAllocator::Reference ref =
On 2016/12/02 19:10:41, manzagop wrote:
> Wdyt of setting a max size that we'll truncate to? Even though we only log
fatal
> messages, we want to make sure the message 1) fits 2) doesn't blow too much
> space if there's something else to log.

If it's somehow too big, the allocation will fail and GetAsArray will return
NULL.

Since right now it's only the browser that has the tracker enabled, there should
only be one and it'll be the last thing going in.  We may have to restrict it
(and/or reuse it) once this extends to other processes.

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:863: allocator_->Allocate(message.size() + 1,
kTypeIdGlobalLogMessage);
On 2016/12/02 19:10:41, manzagop wrote:
> The extra character is to ensure null termination, and we know the memory we
get
> is already null, right? Worth mentioning or is that a property of Allocate?

Done.

[bcwhite, 2016-12-02 19:29:47]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 19:30:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-12-02 19:30:22]

bcwhite@chromium.org changed reviewers:
+ mark@chromium.org

[bcwhite, 2016-12-02 19:30:23]

mark@chromium.org: Please review changes in 

base/logging.cc

[Mark Mentovai, 2016-12-02 19:38:46]

LGTM

You might also be able to use https://codereview.chromium.org/2034393004/ if it
ever lands. That would be preferable if it works for your use case.

[bcwhite, 2016-12-02 19:42:27]

On 2016/12/02 19:38:46, Mark Mentovai wrote:
> LGTM
> 
> You might also be able to use https://codereview.chromium.org/2034393004/ if
it
> ever lands. That would be preferable if it works for your use case.

It would likely work.  5 months?  Been there.  :-)

I'll keep an eye on it and look at refactoring when it lands.

[bcwhite, 2016-12-02 19:42:37]

The CQ bit was unchecked by bcwhite@chromium.org

[bcwhite, 2016-12-02 19:42:38]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-12-02 19:42:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
manzagop@chromium.org
Link to the patchset: https://codereview.chromium.org/2544853003/#ps20001
(title: "added comment about NUL terminator")

[bcwhite, 2016-12-02 19:42:44]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2016-12-02 19:43:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2016-12-02 20:08:09]

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker.cc
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:862: PersistentMemoryAllocator::Reference ref =
On 2016/12/02 19:28:07, bcwhite wrote:
> On 2016/12/02 19:10:41, manzagop wrote:
> > Wdyt of setting a max size that we'll truncate to? Even though we only log
> fatal
> > messages, we want to make sure the message 1) fits 2) doesn't blow too much
> > space if there's something else to log.
> 
> If it's somehow too big, the allocation will fail and GetAsArray will return
> NULL.
> 
> Since right now it's only the browser that has the tracker enabled, there
should
> only be one and it'll be the last thing going in.  We may have to restrict it
> (and/or reuse it) once this extends to other processes.

Yeah, the worry is if it's too big then you get nothing. It would need to rely
on some kind of "best effort allocation" (you asked for X but I only have Y,
here they are).

https://codereview.chromium.org/2544853003/diff/20001/base/debug/activity_tra...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2544853003/diff/20001/base/debug/activity_tra...
base/debug/activity_tracker.h:658: // only store critical messages such as FATAL
ones.
nit for your next cl: can you mention this is best effort on there being space
available?

[bcwhite, 2016-12-02 20:16:30]

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker.cc
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:862: PersistentMemoryAllocator::Reference ref =
On 2016/12/02 20:08:09, manzagop wrote:
> On 2016/12/02 19:28:07, bcwhite wrote:
> > On 2016/12/02 19:10:41, manzagop wrote:
> > > Wdyt of setting a max size that we'll truncate to? Even though we only log
> > fatal
> > > messages, we want to make sure the message 1) fits 2) doesn't blow too
much
> > > space if there's something else to log.
> > 
> > If it's somehow too big, the allocation will fail and GetAsArray will return
> > NULL.
> > 
> > Since right now it's only the browser that has the tracker enabled, there
> should
> > only be one and it'll be the last thing going in.  We may have to restrict
it
> > (and/or reuse it) once this extends to other processes.
> 
> Yeah, the worry is if it's too big then you get nothing. It would need to rely
> on some kind of "best effort allocation" (you asked for X but I only have Y,
> here they are).

Somewhat related:  You should report the IsFull() flag of the allocator in your
data upload.  If true, any kind of information could be missing, including
thread activities.

That flag is persistent and is set if any allocation fails for being too big for
the remaining space.

[manzagop (departed), 2016-12-02 20:50:09]

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker.cc
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2544853003/diff/1/base/debug/activity_tracker...
base/debug/activity_tracker.cc:862: PersistentMemoryAllocator::Reference ref =
On 2016/12/02 20:16:30, bcwhite wrote:
> On 2016/12/02 20:08:09, manzagop wrote:
> > On 2016/12/02 19:28:07, bcwhite wrote:
> > > On 2016/12/02 19:10:41, manzagop wrote:
> > > > Wdyt of setting a max size that we'll truncate to? Even though we only
log
> > > fatal
> > > > messages, we want to make sure the message 1) fits 2) doesn't blow too
> much
> > > > space if there's something else to log.
> > > 
> > > If it's somehow too big, the allocation will fail and GetAsArray will
return
> > > NULL.
> > > 
> > > Since right now it's only the browser that has the tracker enabled, there
> > should
> > > only be one and it'll be the last thing going in.  We may have to restrict
> it
> > > (and/or reuse it) once this extends to other processes.
> > 
> > Yeah, the worry is if it's too big then you get nothing. It would need to
rely
> > on some kind of "best effort allocation" (you asked for X but I only have Y,
> > here they are).
> 
> Somewhat related:  You should report the IsFull() flag of the allocator in
your
> data upload.  If true, any kind of information could be missing, including
> thread activities.
> 
> That flag is persistent and is set if any allocation fails for being too big
for
> the remaining space.

Thanks for the heads up! Added to the list.

[commit-bot: I haz the power, 2016-12-02 21:23:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480707764813660,
"parent_rev": "d81a4e208046b4d3ad13832d2dfbf57a11772348", "commit_rev":
"eebf90e0cd723d4a2017a66a65356904b3ddecdd"}

[commit-bot: I haz the power, 2016-12-02 21:24:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-02 21:27:23]

Description was changed from

==========
Store log messages in global activity tracker.

BUG=620813
==========

to

==========
Store log messages in global activity tracker.

BUG=620813

Committed: https://crrev.com/7a30eb4d95f6ab0fa7e6692be3a8ba5cb4474760
Cr-Commit-Position: refs/heads/master@{#436027}
==========

[commit-bot: I haz the power, 2016-12-02 21:27:24]

Patchset 2 (id:??) landed as
https://crrev.com/7a30eb4d95f6ab0fa7e6692be3a8ba5cb4474760
Cr-Commit-Position: refs/heads/master@{#436027}