Issue 250700043: Android event log importer.

Can't Edit
Can't Publish+Mail
Start Review

Created:
8 years, 9 months ago by coenen

Modified:
8 years, 8 months ago

Reviewers:
Chris Craik, nduca, Scott Bauer, dsinclair, beaudoin, nduca(google)

CC:
tracing-review_chromium.org, beaudoin

Base URL:
https://github.com/google/trace-viewer.git@master

Visibility:
Public.

More Reviews

Description

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. Add a simple Android Activity to the model, representing an Android activity running in the foreground. BUG=#1127

Patch Set 1 #

Total comments: 23

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+374 lines, -0 lines)			Patch
M	tracing/trace_viewer.gypi	View	2 chunks	+2 lines, -0 lines	0 comments	Download
A	tracing/tracing/extras/importer/android/event_log_importer.html	View	1 chunk	+307 lines, -0 lines	19 comments	Download
M	tracing/tracing/extras/importer/linux_perf/ftrace_importer.html	View	2 chunks	+8 lines, -0 lines	0 comments	Download
M	tracing/tracing/extras/systrace_config.html	View	1 chunk	+1 line, -0 lines	0 comments	Download
A	tracing/tracing/model/android_activity.html	View	1 chunk	+56 lines, -0 lines	4 comments	Download

Messages

Total messages: 11

Expand All Messages | Collapse All Messages

coenen

This still needs tests, but would be great to have a first review pass to ...

8 years, 9 months ago (2015-07-23 15:31:48 UTC) #1

Scott Bauer

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/importer/android/event_log_importer.html File tracing/tracing/extras/importer/android/event_log_importer.html (right): https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/importer/android/event_log_importer.html#newcode55 tracing/tracing/extras/importer/android/event_log_importer.html:55: // Generic format of event log entries Anyway to ...

8 years, 9 months ago (2015-07-27 18:38:20 UTC) #2

Chris Craik

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/importer/android/event_log_importer.html File tracing/tracing/extras/importer/android/event_log_importer.html (right): https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/importer/android/event_log_importer.html#newcode143 tracing/tracing/extras/importer/android/event_log_importer.html:143: // "entry activity" that was immediately finished. Therefore, set ...

8 years, 9 months ago (2015-07-27 19:50:53 UTC) #3

coenen

8 years, 9 months ago (2015-07-29 13:21:09 UTC) #4

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
File tracing/tracing/extras/importer/android/event_log_importer.html (right):

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:55: // Generic
format of event log entries
On 2015/07/27 18:38:19, Scott Bauer wrote:
> Anyway to get a quick example of what these regex would match, or would that
> clutter this up too much?

Good idea, I'll add a sample line.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:77: if
(eventLogActivityRE.test(events)) {
On 2015/07/27 18:38:19, Scott Bauer wrote:
> return eventLogActivityRE.test(events);

Acknowledged.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:143: // "entry
activity" that was immediately finished. Therefore, set the
On 2015/07/27 19:50:53, Chris Craik wrote:
> do you mean trampoline activity? Or am I mixing the names up?

Yeah. I'm thinking this code should probably not live here - it should just
construct the model. We can accomodate for trampoline activities in the
auditor/analysis code.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:147: // Also
merge in if we started the process for the previous activity
On 2015/07/27 19:50:53, Chris Craik wrote:
> "if we started the process for the previous activity"
> 
> What's the explanation/reasoning here? Is there something special about the
> first activity being launched?

Basically the analysis script I have dumps statistics on an Activity basis; for
example, whether the process was started on behalf of this activity launch. But
trampoline activities make that output kind of messy - they show two activities
in short succession. But as said in previous comment, I will probably do that
merging in a later stage and keep this simple.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:148: if
(previousFocusActivity.procStartTs) {
On 2015/07/27 18:38:20, Scott Bauer wrote:
> Remove { } on this if.

Acknowledged.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:192: var
AndroidActivity = new tr.model.AndroidActivity(activity.name,
On 2015/07/27 18:38:20, Scott Bauer wrote:
> Indentation off here, and on process.activities.push(etc..)?

Acknowledged.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/import...
tracing/tracing/extras/importer/android/event_log_importer.html:233: if (action
== 'am_create_activity') {
On 2015/07/27 18:38:19, Scott Bauer wrote:
> Do these, and below, need to be === instead of ==?

In this case I think it shouldn't matter, but I agree it's better to
consistently use '==='..will fix.

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/model/android...
File tracing/tracing/model/android_activity.html (right):

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/model/android...
tracing/tracing/model/android_activity.html:1: <!DOCTYPE html>
On 2015/07/27 19:50:53, Chris Craik wrote:
> this is strange to include in tracing/model, since all of the android
> application model structure lives in extras/android.
> 
> I think this should be moved there. Additionally, I think it would make sense
> for these to reside in the AndroidApp objects, even if that has to be done as
> part of post processing in runAnnotate()

Yeah I'm happy to have this as a part of AndroidApp. But there needs to be some
intermediate data structure between the importer and the auditor that stores the
data the importer parsed; the best place for that seemed to be the model.

One alternative could be that I simply direct push slices in the importer, and
the auditor converts these to AndroidActivities in AndroidApp..what do you
think?

Chris Craik

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/model/android_activity.html File tracing/tracing/model/android_activity.html (right): https://codereview.appspot.com/250700043/diff/1/tracing/tracing/model/android_activity.html#newcode1 tracing/tracing/model/android_activity.html:1: <!DOCTYPE html> Do they make sense as slices? If ...

8 years, 9 months ago (2015-07-29 17:02:13 UTC) #5

dsinclair

https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/importer/android/event_log_importer.html File tracing/tracing/extras/importer/android/event_log_importer.html (right): https://codereview.appspot.com/250700043/diff/1/tracing/tracing/extras/importer/android/event_log_importer.html#newcode61 tracing/tracing/extras/importer/android/event_log_importer.html:61: var amCreateRE = new RegExp('\s*\\[.*,.*,.*,(.*),.*,.*,.*,.*\\]'); Should these be .*? ...

8 years, 8 months ago (2015-08-04 19:44:15 UTC) #6

coenen

8 years, 8 months ago (2015-08-11 23:17:51 UTC) #7

nduca(google)

Actually, driving by, I'd be fine with model objects about some notion of activities actually. ...

8 years, 8 months ago (2015-08-11 23:22:23 UTC) #8

coenen

On 2015/08/11 23:22:23, nduca(google) wrote: > Actually, driving by, I'd be fine with model objects ...

8 years, 8 months ago (2015-08-11 23:54:05 UTC) #10

nduca(google)

8 years, 8 months ago (2015-08-12 00:03:57 UTC) #11

so, backing up, there is fields vs args, and there's a subtyping strategy you
could deploy too.

i decide between args and fields based on whether i expect there to be
substantial js logic that relies on that thing being present.

e.g. if you're just pulling fields and doing general grouping/summation, args.

but, if you're using a field for actual business logic, e.g. find me all the
things of type="service", then its probably a field.

i always try for args, so i typically put that on all model objects just to help
head off the endless-field-additions, so to speak. whether to add fields is your
call.



then, there's the issue of android vs non-android attributes. in that space,
subclassing is totally cool: you could have AndroidActivity that subs Activity.

both are fine by me.

Expand All Messages | Collapse All Messages