Filter android logs by process id #990

petekanev · 2017-07-24T13:44:09Z

Problem

NativeScript/nativescript-cli#2818 - When an app was started the CLI's logging filter would parse each incoming adb line and check it against a regex that either JS: was the tag used, or that any of the accepted tags's string was contained within the logcat line.
That would at times return incorrect, or incomplete logs.

Proposed solution

Read the process id for the application process from adb, then filter logs by the pid.

justcodebuilduser · 2017-07-24T13:44:23Z

💔

rosen-vladimirov · 2017-07-24T23:13:38Z

mobile/android/android-application-manager.ts

@@ -49,10 +50,39 @@ export class AndroidApplicationManager extends ApplicationManagerBase {
 			"1"]);

 		if (!this.$options.justlaunch) {
+			let processIdentifier = await this.getProcessId(appIdentifier);
+			if (processIdentifier) {
+				let deviceIdentifier = this.adb.identifier;


why do you use this.adb.identifier instead of this.identifier

rosen-vladimirov · 2017-07-24T23:15:08Z

mobile/android/android-application-manager.ts

+		// Information is returned in the following format
+		// USER      PID   PPID  VSIZE  RSS   WCHAN            PC  NAME
+		// u0_a64    4831  488   851380 94888    ep_poll f1e43bb9 S org.nativescript.myApplication
+		let processes = await this.adb.executeShellCommand(["ps"]);


Instead of executing this and parsing the result, can't you use the $androidProcessService: https://github.com/telerik/mobile-cli-lib/blob/master/mobile/mobile-core/android-process-service.ts#L206-L218

rosen-vladimirov · 2017-07-24T23:16:36Z

mobile/android/android-log-filter.ts

@@ -7,14 +7,14 @@ export class AndroidLogFilter implements Mobile.IPlatformLogFilter {

 	// sample line is "11-23 12:39:07.310  1584  1597 I art     : Background sticky concurrent mark sweep GC freed 21966(1780KB) AllocSpace objects, 4(80KB) LOS objects, 77% free, 840KB/3MB, paused 4.018ms total 158.629ms"
 	// or '12-28 10:45:08.020  3329  3329 W chromium: [WARNING:data_reduction_proxy_settings.cc(328)] SPDY proxy OFF at startup'
-	private static API_LEVEL_23_LINE_REGEX = /.+?\s+?(?:[A-Z]\s+?)([A-Za-z ]+?)\s*?\: (.*)/;
+	private static API_LEVEL_23_LINE_REGEX = /.+?\s+?(?:[A-Z]\s+?)([A-Za-z \.]+?)\s*?\: (.*)/;


Why is this changed? I couldn't understand it from the commit message.

After personal discussion - the reason is that we have added a new tag, that has . in it.

rosen-vladimirov · 2017-07-24T23:20:13Z

mobile/android/android-log-filter.ts

-		let acceptedTags = ["chromium", "Web Console", "JS"];
+	private getConsoleLogFromLine(lineText: string, pid: string): any {
+		// filter log line if it does not belong to the current application process id
+		if (lineText.indexOf(pid) < 0) {


So in case we do not have pid, we'll always return null. But in case we have PID, we'll still apply the incorrect filters?
For iOS Simulator we have similar case, but the logic there is:

in case there's a pid, find it in the logs and show each line that contains it

in case we do not have a PID, apply the old filtering

Can't we do the same here?

rosen-vladimirov · 2017-07-24T23:22:57Z

mobile/android/android-log-filter.ts

 		let match = lineText.match(AndroidLogFilter.LINE_REGEX) || lineText.match(AndroidLogFilter.API_LEVEL_23_LINE_REGEX);

 		if (match && acceptedTags.indexOf(match[1].trim()) !== -1) {
 			return { tag: match[1].trim(), message: match[2] };
 		}
-		let matchingTag = _.some(acceptedTags, (tag: string) => { return lineText.indexOf(tag) !== -1; });


Why is this line deleted? Its idea is to save us in cases where the logs are changed and our regex fails, but we still check if the line contains "chromium" for example.

rosen-vladimirov · 2017-07-24T23:24:01Z

mobile/android/device-android-debug-bridge.ts

@@ -6,7 +6,7 @@ interface IComposeCommandResult {
 }

 export class DeviceAndroidDebugBridge extends AndroidDebugBridge implements Mobile.IDeviceAndroidDebugBridge {
-	constructor(private identifier: string,
+	constructor(public identifier: string,


I believe you do not need this change as the only place where you are using the property outside of the current class is on a place, where you already have the identifier.

justcodebuilduser · 2017-07-24T23:25:02Z

💔

rosen-vladimirov

Unit tests are failing.
Also, once we have an agreement for the expected behavior, new unit tests should be added for the new filtering.

rosen-vladimirov · 2017-07-24T23:30:29Z

mobile/android/android-application-manager.ts

+		let currentProcessLine = processes.find((line: Array<string>) => line[line.length - 1] === appId);
+
+		if (!currentProcessLine) {
+			return Promise.resolve(null);


you do not need to return Promise here, async keyword does this for you.

return null;

is enough

justcodebuilduser · 2017-07-25T11:38:06Z

💔

justcodebuilduser · 2017-07-25T11:49:48Z

💔

nadyaA · 2017-07-25T11:45:43Z

mobile/android/android-application-manager.ts

@@ -49,6 +50,12 @@ export class AndroidApplicationManager extends ApplicationManagerBase {
 			"1"]);

 		if (!this.$options.justlaunch) {
+			let deviceIdentifier = this.identifier;


const maybe?

As I see you use 'let' in almost all cases, if there is no specific reason (if so what), you may consider updating your code. As you know declaring the variables with 'let' indicates the'll be changed and most of these are not. 😸

Noted, thank you!

justcodebuilduser · 2017-07-25T12:13:15Z

💔

justcodebuilduser · 2017-07-25T13:48:26Z

💔

rosen-vladimirov

After green build.

Great work, I love the tests. 🥇

rosen-vladimirov · 2017-07-25T22:14:35Z

test/unit-tests/android-log-filter.ts

+
+			it("when API level 23 or later is used, and application pid doesn't match any logcat pids", () => {
+				const appPid = "99999";
+				const expectedOutputMap = androidApiLevel23MapForPid8141.map((testData) => { return { input: testData.input, output: null } });


you can shorten this:

const expectedOutputMap = androidApiLevel23MapForPid8141.map(testData => ({ input: testData.input, output: null }) );

justcodebuilduser · 2017-07-26T08:13:45Z

💔

justcodebuilduser · 2017-07-26T08:16:55Z

💔

petekanev self-assigned this Jul 24, 2017

petekanev requested review from nadyaA, rosen-vladimirov and TsvetanMilanov July 24, 2017 13:44

rosen-vladimirov suggested changes Jul 24, 2017

View reviewed changes

rosen-vladimirov closed this Jul 24, 2017

rosen-vladimirov reopened this Jul 24, 2017

rosen-vladimirov suggested changes Jul 24, 2017

View reviewed changes

petekanev force-pushed the pete/log-by-pid-android branch from 33354a9 to 189c7d1 Compare July 25, 2017 11:37

petekanev force-pushed the pete/log-by-pid-android branch from 189c7d1 to 07d3a83 Compare July 25, 2017 11:49

nadyaA reviewed Jul 25, 2017

View reviewed changes

petekanev force-pushed the pete/log-by-pid-android branch from 07d3a83 to 2ec92a7 Compare July 25, 2017 12:12

petekanev force-pushed the pete/log-by-pid-android branch from 2ec92a7 to 6b3c100 Compare July 25, 2017 13:48

rosen-vladimirov approved these changes Jul 25, 2017

View reviewed changes

Filter android logs by process id

25cc134

petekanev force-pushed the pete/log-by-pid-android branch from 6b3c100 to 25cc134 Compare July 26, 2017 08:13

Merge branch 'master' into pete/log-by-pid-android

3dcc8f9

petekanev merged commit b0bd648 into master Jul 26, 2017

petekanev deleted the pete/log-by-pid-android branch July 26, 2017 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter android logs by process id #990

Filter android logs by process id #990

petekanev commented Jul 24, 2017

justcodebuilduser commented Jul 24, 2017

rosen-vladimirov Jul 24, 2017

rosen-vladimirov Jul 24, 2017

rosen-vladimirov Jul 24, 2017

rosen-vladimirov Jul 25, 2017

rosen-vladimirov Jul 24, 2017

rosen-vladimirov Jul 24, 2017

rosen-vladimirov Jul 24, 2017

justcodebuilduser commented Jul 24, 2017

rosen-vladimirov left a comment

rosen-vladimirov Jul 24, 2017

justcodebuilduser commented Jul 25, 2017

justcodebuilduser commented Jul 25, 2017

nadyaA Jul 25, 2017

nadyaA Jul 25, 2017

petekanev Jul 25, 2017

justcodebuilduser commented Jul 25, 2017

justcodebuilduser commented Jul 25, 2017

rosen-vladimirov left a comment

rosen-vladimirov Jul 25, 2017

justcodebuilduser commented Jul 26, 2017

justcodebuilduser commented Jul 26, 2017

Filter android logs by process id #990

Filter android logs by process id #990

Conversation

petekanev commented Jul 24, 2017

Problem

Proposed solution

justcodebuilduser commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Jul 24, 2017

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Jul 25, 2017

justcodebuilduser commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Jul 25, 2017

justcodebuilduser commented Jul 25, 2017

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Jul 26, 2017

justcodebuilduser commented Jul 26, 2017