update(cli): Improve filtering of iOS logs by ginev · Pull Request #3389 · NativeScript/nativescript-cli

ginev · 2018-02-22T16:24:02Z

This PR improves the overall behaviour of the IOSLogFilter within the CLI. We now filter data based on the following core assumption:

all messages coming from the related NS app contain the following artefact: <app-name>(NativeScript)?[<pid>], where (NativeScript) might be missing
all relevant multiline messages do not contain iOS artefacts like May 24 15:54:52 Dragons-iPhone NativeScript250(NativeScript)[356] <Notice>: and directly follow a message with the first artefact

Relying on these principles, we pass messages even if they are multiline (console.dir, stack-traces, etc).

Unit tests have been slightly updated to reflect the currently expected behaviour.

This PR should address this issue: #3105 and is also related to supporting this feature: NativeScript/ios-jsc#884

Merge after telerik/mobile-cli-lib#1062

rosen-vladimirov

The current implementation will cause some issues in Sidekick, as the projectData is not initialized in it. I have to think how to resolve this and come with a suggestion, but meanwhile you can take a look at my comments,

rosen-vladimirov · 2018-02-25T20:27:58Z

@@ -4,62 +4,80 @@ import { cache } from "../common/decorators";
 import * as iOSLogFilterBase from "../common/mobile/ios/ios-log-filter";

 export class IOSLogFilter extends iOSLogFilterBase.IOSLogFilter implements Mobile.IPlatformLogFilter {


As you are no longer calling the method from the base class, I suggest to remove the inheritance of iOSLogFilterBase.IOSLogFilter. Implementing the interface is enough.

rosen-vladimirov · 2018-02-25T20:50:38Z

 		private $projectData: IProjectData) {
 		super($loggingLevels);
+		this.projectName = this.$projectData.projectName;
+		this.loggingLevels = $loggingLevels;


instead of setting a new property here, you can just add access modifier to the $loggingLevels in the constructor, for example:

constructor(private $loggingLevels: Mobile.ILoggingLevels,

rosen-vladimirov · 2018-02-25T21:54:21Z

 					const actualData = logFilter.filterData(line, infoLogLevel, null);
 					const expectedData = data.infoExpectedArr[index];
-					assert.deepEqual(actualData && actualData.trim(), expectedData && expectedData);
+					assert.equal(actualData && actualData.trim(), expectedData && expectedData);


why is this changed? Also can you fix the expected result - expectedData && expectedData seems incorrect

I think deepEqual is not needed here as we are comparing strings.

rosen-vladimirov · 2018-02-27T22:25:10Z

-		testInjector = createTestInjector();
-		logFilter = testInjector.resolve(IOSLogFilter);
-	});
+	// beforeEach(() => {


please remove these comments

ginev · 2018-03-01T12:55:46Z

run ci

KristianDD

Overall great work. Thank you for the effort.

KristianDD · 2018-03-02T11:26:18Z

 		private $fs: IFileSystem,
 		private $projectData: IProjectData) {
-		super($loggingLevels);
+		this.projectName = this.$projectData.projectName;


I think this might not work in Sidekick as there only one instance is created of the filter. Try getting projectData from projectDataService.getProjectData and using the project name from there on evry invocation of the filterData function or pass it as an argument if available where the method gets called.

KristianDD · 2018-03-02T11:52:07Z

+			}
+
+			if (i === chunkLines.length - 1 && skipLastLine) {
+				this.partialLine = currentLine;


Would be grate if we could persist this per device identifier so that we don't mix lines from different devices. (This is not mandatory as it was the same way before).

As far as I know, there is no way of receiving log messages from parallel processes with the latest version of CLI.

ginev · 2018-03-05T07:38:24Z

run ci

rosen-vladimirov · 2018-03-05T21:21:04Z

As @KristianDD mentioned, using $projectData will not work in Sidekick. Using $projectDataService is not an option, as you still have to know the projectDir every time when you filter the logs. I've been thinking how to resolve this issue and I think we can use the same approach as the one we have for Android log filters.
Currently, for Android applications, whenever CLI calls startApplication (i.e. runs application on device), we find the new PID of the application and set it to the filter. You can check the logic here
The $deviceLogProvider later passes the value to Android log filter, as shown here

So my suggestion is to use the same approach for iOS logs filtering - whenever CLI calls startApplication, we can set the projectName to the filter. The filtering method will receive it as an argument from the deviceLogProvider. So we have to add the logic of setting the projectName to the deviceLogProvider in both [ios-application-manager
(https://github.com/telerik/mobile-cli-lib/blob/6ec3a807b3d18399366f50d186266b9610c210d8/mobile/ios/device/ios-application-manager.ts#L83) and ios-simulator-application-manager.
After that, in the deviceLogProvider, we have to get the already set projectName and pass it to the filterData method.

When strating iOS Application, set the projectName to the log filter, so we can filter the logs by it. Same logic is used for Android, where we are using the PID of the started application. So introduce IDeviceLogOptions, where logLevel, pid and projectName can be set. Pass the projectName wherever is required in order to get it in startApplication.

…thods In order to make future refactorings easier, introduce a single object as an argument to some of the methods in devices' ApplicationManagers.

ginev added this to the 4.0.0 milestone Feb 22, 2018

ginev self-assigned this Feb 22, 2018

ginev requested review from KristianDD, Mitko-Kerezov, mbektchiev and rosen-vladimirov February 22, 2018 16:24

ginev mentioned this pull request Feb 22, 2018

fix(console-dir): Fix missing console.dir output NativeScript/ios-jsc#884

Merged

ginev force-pushed the ginev/ios-filter-revamp branch 4 times, most recently from e9bf8a7 to 596a867 Compare February 22, 2018 16:53

ginev requested a review from tdermendjiev February 23, 2018 07:59

ginev force-pushed the ginev/ios-filter-revamp branch from 596a867 to f30ecbd Compare February 26, 2018 08:49

rosen-vladimirov suggested changes Feb 27, 2018

View reviewed changes

ginev force-pushed the ginev/ios-filter-revamp branch from 326b756 to 1f348e5 Compare March 1, 2018 12:45

ginev force-pushed the ginev/ios-filter-revamp branch 3 times, most recently from 4d51e32 to 2fbdcfa Compare March 1, 2018 15:36

mbektchiev approved these changes Mar 2, 2018

View reviewed changes

KristianDD reviewed Mar 2, 2018

View reviewed changes

ginev force-pushed the ginev/ios-filter-revamp branch 2 times, most recently from bc4754c to 443afaf Compare March 2, 2018 16:09

ginev force-pushed the ginev/ios-filter-revamp branch from e624956 to 3f8b730 Compare March 5, 2018 08:43

rosen-vladimirov force-pushed the ginev/ios-filter-revamp branch 4 times, most recently from 26b1a4c to 2caaf19 Compare March 13, 2018 12:02

rosen-vladimirov approved these changes Mar 13, 2018

View reviewed changes

Deyan Ginev and others added 4 commits March 13, 2018 14:52

update(cli): Improve filtering of iOS logs

8049a2a

Addressing linter issues and comments.

eb4f8b5

refactor: Use single object as arg for some of application manager me…

605df4a

…thods In order to make future refactorings easier, introduce a single object as an argument to some of the methods in devices' ApplicationManagers.

rosen-vladimirov force-pushed the ginev/ios-filter-revamp branch from 2caaf19 to 605df4a Compare March 13, 2018 12:53

rosen-vladimirov merged commit 414d0d3 into master Mar 13, 2018

rosen-vladimirov deleted the ginev/ios-filter-revamp branch March 13, 2018 13:40

		@@ -4,62 +4,80 @@ import { cache } from "../common/decorators";
		import * as iOSLogFilterBase from "../common/mobile/ios/ios-log-filter";

		export class IOSLogFilter extends iOSLogFilterBase.IOSLogFilter implements Mobile.IPlatformLogFilter {

Uh oh!

Conversation

ginev commented Feb 22, 2018 • edited by rosen-vladimirov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ginev commented Mar 1, 2018

Uh oh!

KristianDD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ginev commented Mar 5, 2018

Uh oh!

rosen-vladimirov commented Mar 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ginev commented Feb 22, 2018 •

edited by rosen-vladimirov

Loading