[memory-infra] Log reasons for memory dump failure

[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
TBR=shatch

Committed: https://crrev.com/7d8aadbdcca5302d1c84637aeca865a791afd5da
Cr-Commit-Position: refs/heads/master@{#398838}

[petrcermak, 2016-06-08 17:45:22]

Description was changed from

==========
[memory-infra] Log reasons for memory dump failure

BUG=616483
==========

to

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
==========

[petrcermak, 2016-06-08 17:45:36]

petrcermak@chromium.org changed reviewers:
+ primiano@chromium.org

[petrcermak, 2016-06-08 17:45:37]

PTAL.

Thanks,
Petr

[Primiano Tucci (use gerrit), 2016-06-08 18:00:45]

Wow, thanks for doing all this yourself. 
Super happy LGTM with some small comments.

+ssid just because this might conflict with some of his on-flight patches.

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:340: VLOG(1) << "Global memory dump
failed because " << kTraceCategory
I'd say s/Global memory dump/memory-infra dump/.
Imagine this shows in the log, more people know what memory-infra is. "GLobal
memory dump" is more obscure, can sound like a crash reporting thing.

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:428: << "Global memory dump failed
because dump thread was destroyed before "
you probably want to make "Global Memory Dump" (or the new suggestion) a
kConstant, so you are guaranteed that you can grep for that constatn in the logs
and they will all start with the same prefix.

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:608: VLOG(1) << "Memory dump failed:
Tracing is not enabled any more";
I'd say here: tracing disabled before the memory dump was completed.

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
File components/tracing/child/child_memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
components/tracing/child/child_memory_dump_manager_delegate_impl.cc:79: VLOG(1)
<< "Process memory dump failed because child trace message filter "
and use the same kConstant here

[ssid, 2016-06-08 18:12:41]

ssid@chromium.org changed reviewers:
+ ssid@chromium.org

[ssid, 2016-06-08 18:12:41]

Thanks, I have rebase my cl on top of this.

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
File components/tracing/child/child_memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
components/tracing/child/child_memory_dump_manager_delegate_impl.cc:100: << "has
been destroyed";
destroyed while collecting the dump or something?

[petrcermak, 2016-06-08 19:15:48]

petrcermak@chromium.org changed reviewers:
+ simonhatch@chromium.org

[petrcermak, 2016-06-08 19:15:49]

simonhatch: Can I please get an owner stamp?

Thanks,
Petr

[petrcermak, 2016-06-09 09:44:19]

Patchset #2 (id:20001) has been deleted

[petrcermak, 2016-06-09 09:46:09]

Thanks for your comments. Could you please have one more look?

Thanks,
Petr

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:340: VLOG(1) << "Global memory dump
failed because " << kTraceCategory
On 2016/06/08 18:00:45, Primiano Tucci wrote:
> I'd say s/Global memory dump/memory-infra dump/.
> Imagine this shows in the log, more people know what memory-infra is. "GLobal
> memory dump" is more obscure, can sound like a crash reporting thing.

Done.

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:428: << "Global memory dump failed
because dump thread was destroyed before "
On 2016/06/08 18:00:45, Primiano Tucci wrote:
> you probably want to make "Global Memory Dump" (or the new suggestion) a
> kConstant, so you are guaranteed that you can grep for that constatn in the
logs
> and they will all start with the same prefix.

Done.

https://codereview.chromium.org/2049143002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:608: VLOG(1) << "Memory dump failed:
Tracing is not enabled any more";
On 2016/06/08 18:00:45, Primiano Tucci wrote:
> I'd say here: tracing disabled before the memory dump was completed.

Done.

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
File components/tracing/child/child_memory_dump_manager_delegate_impl.cc
(right):

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
components/tracing/child/child_memory_dump_manager_delegate_impl.cc:79: VLOG(1)
<< "Process memory dump failed because child trace message filter "
On 2016/06/08 18:00:45, Primiano Tucci wrote:
> and use the same kConstant here

Done.

https://codereview.chromium.org/2049143002/diff/1/components/tracing/child/ch...
components/tracing/child/child_memory_dump_manager_delegate_impl.cc:100: << "has
been destroyed";
On 2016/06/08 18:12:41, ssid wrote:
> destroyed while collecting the dump or something?

Added "while switching threads"

[Primiano Tucci (use gerrit), 2016-06-09 09:51:22]

LGTM

https://codereview.chromium.org/2049143002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2049143002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:43: static const char* const
kMemoryInfraDumpLogName;
s/kMemoryInfraDumpLogName/kLogPrefix/

This is already in MemoryDumpManager scope, MemoryInfra seems reundant here in
the constant name.

[petrcermak, 2016-06-09 09:58:30]

Thanks for your comments.

Petr

https://codereview.chromium.org/2049143002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2049143002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:43: static const char* const
kMemoryInfraDumpLogName;
On 2016/06/09 09:51:22, Primiano Tucci wrote:
> s/kMemoryInfraDumpLogName/kLogPrefix/
> 
> This is already in MemoryDumpManager scope, MemoryInfra seems reundant here in
> the constant name.

Done. Note that since the constant is now called a "prefix", I changed it to
start with an uppercase letter.

[petrcermak, 2016-06-09 09:59:18]

Description was changed from

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
==========

to

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
TBR=shatch
==========

[Primiano Tucci (use gerrit), 2016-06-09 10:00:39]

I think it's OK to TBR Simon or Oystein for the change in
tracing_controller_impl.cc. That is just changing the log statements and it
seems we need this log as soon as possible to solve flakiness problems on
telemetry.

[petrcermak, 2016-06-09 10:01:05]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2016-06-09 10:01:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2049143002/#ps60001
(title: "Shorten constant name")

[commit-bot: I haz the power, 2016-06-09 10:01:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2049143002/60001

[commit-bot: I haz the power, 2016-06-09 11:13:15]

Description was changed from

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
TBR=shatch
==========

to

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
TBR=shatch
==========

[commit-bot: I haz the power, 2016-06-09 11:13:16]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-09 11:14:27]

Description was changed from

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
TBR=shatch
==========

to

==========
[memory-infra] Log reasons for memory dump failure

All memory dump failures will now lead to VLOG(1) error messages so that
the reason for failure could be determined from outside chrome (e.g. in
Telemetry).

BUG=616483
TBR=shatch

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

[commit-bot: I haz the power, 2016-06-09 11:14:28]

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