Android event log importer.

Android event log importer.

This importer parses the Android event log,
which can be dumped with "adb logcat -b events -d".
The event log contains information about which
activities are created, resumed and paused.

Adds a simple activity class to the model,
which can be used to represent an Android
activity running in the foreground.

BUG=#1127

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/fb5fd186eb3536f9132ae6977377814833fc1877

[coenen, 2015-08-27 14:08:10]

coenen@gmail.com changed reviewers:
+ ccraik@google.com, dsinclair@chromium.org, nduca@chromium.org,
tkjos@google.com

[coenen, 2015-08-27 14:11:18]

This is the follow-up to https://codereview.appspot.com/250700043/ , with all
review comments addressed.

Nat, following your suggestion I made Activity an abstract class in the model,
initially using args to stash some Android-specific stuff. Will probably go with
a subclass as this expands.

The analysis (d8) code using this model data I haven't uploaded yet, currently
looking at map_traces to see where this fits.

[dsinclair, 2015-08-27 15:18:49]

dsinclair@chromium.org changed reviewers:
+ nednguyen@google.com

[dsinclair, 2015-08-27 15:18:51]

I'm not sure about the clock sync stuff. There is a bunch of work going on to
fixup our clock sync code and try to add some sanity.

Adding nednguyen@ as he's been working on the clock sync proposals.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
File tracing/tracing/extras/importer/android/event_log_importer.html (right):

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:25: *
nit: remove blank line.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:51:
this.importPriority = 3;
nit: _ on importPriority

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:61:
'\\s*(\\d+)\\s*(\\d+)\\s*([A-Z])\\s*' +
I think the s*'s on this line should be s+'s. It looks like at least one space
is required, yea?

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:64: var
amCreateRE = new RegExp('\s*\\[.*,.*,.*,(.*),.*,.*,.*,.*\\]');
Having examples for each of these as was done on line 57 would make determining
the correctness of the RE's easier.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:73: *
nit: remove blank line.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:116:
activityMap[activityName] = activity;
var activity = {
  state: ACTIVITY_STATE.NONE,
  name: activityName,
};

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:124:
handleCreateActivity: function(ts, pid, activityName) {
pid is passed into a bunch of these, but not used, can it be removed from the
signature of the methods that don't need it?

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:144: //
On_resume_called shows the actual PID; use this
nit: on_remove_called (since that's the actual name).

nit: period at the end of sentences.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:157: }
nit: don't need {}'s for single line body.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:170: if
(activity.pid !== undefined) {
if (activity.pid === undefined)
  return;

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:176:
activity.lastPauseTs);
Why does an activity only have one range? What happens if I switch between two
activities a few times during a trace, won't this only record the last focus
instead of all focused times?

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:206: // Working
on getting Android event log to include the year.
Is there a bug for this we can link?

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:241: }
nit: {}'s

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:245:
this.handleOnPauseCalled(monotonic_ts, pid, match[1]);
nit: {}'s

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:263:
console.log('Need a trace_event_clock_sync to map realtime.');
This should set an import error instead of logging to the console.

this.model_.importWarning({                                           
   type: 'event_log_clock_sync',                                        
   message: 'Need a trace_event_clock_sync to map realtime clock.') 
});

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:287:
tr.importer.Importer.register(EventLogImporter);
This was aliased above so just Importer should work.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/model/activ...
File tracing/tracing/model/activity.html (right):

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/model/activ...
tracing/tracing/model/activity.html:17: * On Windows/OS X this could for example
representing
s/representing/represent

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/model/activ...
tracing/tracing/model/activity.html:19: *
nit: remove blank line

[Chris Craik, 2015-08-27 17:09:05]

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
File tracing/tracing/extras/importer/android/event_log_importer.html (right):

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:57: //    
[0,com.google.android.googlequicksearchbox/com.google.android.launcher.GEL]
"// @suppress longLineCheck" needed here?

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:263:
console.log('Need a trace_event_clock_sync to map realtime.');
On 2015/08/27 15:18:50, dsinclair wrote:
> This should set an import error instead of logging to the console.
> 
> this.model_.importWarning({                                           
>    type: 'event_log_clock_sync',                                        
>    message: 'Need a trace_event_clock_sync to map realtime clock.') 
> });  

I assume old devices won't have the trace event sync, but will have these events
(we just can't use them). How should that be treated?

[coenen, 2015-09-01 11:55:49]

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
File tracing/tracing/extras/importer/android/event_log_importer.html (right):

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:25: *
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:51:
this.importPriority = 3;
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: _ on importPriority

I think this is used externally to determine the order of import (other
importers also declare this without _).

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:57: //    
[0,com.google.android.googlequicksearchbox/com.google.android.launcher.GEL]
On 2015/08/27 17:09:05, Chris Craik wrote:
> "// @suppress longLineCheck" needed here?

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:61:
'\\s*(\\d+)\\s*(\\d+)\\s*([A-Z])\\s*' +
On 2015/08/27 15:18:50, dsinclair wrote:
> I think the s*'s on this line should be s+'s. It looks like at least one space
> is required, yea?

Yeah agreed, done

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:64: var
amCreateRE = new RegExp('\s*\\[.*,.*,.*,(.*),.*,.*,.*,.*\\]');
On 2015/08/27 15:18:50, dsinclair wrote:
> Having examples for each of these as was done on line 57 would make
determining
> the correctness of the RE's easier.

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:73: *
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:116:
activityMap[activityName] = activity;
On 2015/08/27 15:18:50, dsinclair wrote:
> var activity = {
>   state: ACTIVITY_STATE.NONE,
>   name: activityName,
> };

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:124:
handleCreateActivity: function(ts, pid, activityName) {
On 2015/08/27 15:18:50, dsinclair wrote:
> pid is passed into a bunch of these, but not used, can it be removed from the
> signature of the methods that don't need it?

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:144: //
On_resume_called shows the actual PID; use this
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: on_remove_called (since that's the actual name).
> 
> nit: period at the end of sentences.

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:157: }
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: don't need {}'s for single line body.

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:170: if
(activity.pid !== undefined) {
On 2015/08/27 15:18:50, dsinclair wrote:
> if (activity.pid === undefined)
>   return;

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:176:
activity.lastPauseTs);
On 2015/08/27 15:18:50, dsinclair wrote:
> Why does an activity only have one range? What happens if I switch between two
> activities a few times during a trace, won't this only record the last focus
> instead of all focused times?

With the current implementation, it'll add a separate activity instance for
every pause. Focus/resume always come before pause, so they should be up to
date.

The reason I had Activity only implement one range is that every time an
Activity is showing, it may have a different context (eg invoked for a different
reason, or with different parameters). Not so much needed for Android, but I
thought it would be a nicer abstraction for platforms in general.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:206: // Working
on getting Android event log to include the year.
There is a CL for it here; https://android-review.googlesource.com/#/c/168900/2

want me to put that or the internal bug in the comments here?

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:241: }
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: {}'s

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:245:
this.handleOnPauseCalled(monotonic_ts, pid, match[1]);
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: {}'s

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:263:
console.log('Need a trace_event_clock_sync to map realtime.');
On 2015/08/27 17:09:05, Chris Craik wrote:
> On 2015/08/27 15:18:50, dsinclair wrote:
> > This should set an import error instead of logging to the console.
> > 
> > this.model_.importWarning({                                           
> >    type: 'event_log_clock_sync',                                        
> >    message: 'Need a trace_event_clock_sync to map realtime clock.') 
> > });  
> 
> I assume old devices won't have the trace event sync, but will have these
events
> (we just can't use them). How should that be treated?

Ok, changed to importWarning. I think we just refuse to import in case it's
missing. Anyway there shouldn't be trace files containing the event log but
without the clock sync record, since the code to generate such combined trace
files is not checked in yet. But they could be fed separately to the importers,
in which case I think we should just bail out.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/extras/impo...
tracing/tracing/extras/importer/android/event_log_importer.html:287:
tr.importer.Importer.register(EventLogImporter);
On 2015/08/27 15:18:50, dsinclair wrote:
> This was aliased above so just Importer should work.

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/model/activ...
File tracing/tracing/model/activity.html (right):

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/model/activ...
tracing/tracing/model/activity.html:17: * On Windows/OS X this could for example
representing
On 2015/08/27 15:18:50, dsinclair wrote:
> s/representing/represent

Done.

https://codereview.chromium.org/1319013002/diff/1/tracing/tracing/model/activ...
tracing/tracing/model/activity.html:19: *
On 2015/08/27 15:18:50, dsinclair wrote:
> nit: remove blank line

Done.

[dsinclair, 2015-09-01 13:22:37]

lg w/ nit. I'm good when ccraik@ is happy.

https://codereview.chromium.org/1319013002/diff/20001/tracing/tracing/extras/...
File tracing/tracing/extras/importer/android/event_log_importer.html (right):

https://codereview.chromium.org/1319013002/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/importer/android/event_log_importer.html:280:
console.log('');
Remove the console.log

[coenen, 2015-09-01 13:26:02]

https://codereview.chromium.org/1319013002/diff/20001/tracing/tracing/extras/...
File tracing/tracing/extras/importer/android/event_log_importer.html (right):

https://codereview.chromium.org/1319013002/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/importer/android/event_log_importer.html:280:
console.log('');
On 2015/09/01 13:22:37, dsinclair wrote:
> Remove the console.log

Oops, done.

[Chris Craik, 2015-09-02 18:24:28]

lgtm

[dsinclair, 2015-09-02 19:50:57]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2015-09-02 19:51:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1319013002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1319013002/40001

[commit-bot: I haz the power, 2015-09-02 19:59:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...