Skip to content

Kddimitrov/track perf decorator#4242

Merged
KristianDD merged 9 commits into
masterfrom
kddimitrov/track-perf-decorator
Jan 15, 2019
Merged

Kddimitrov/track perf decorator#4242
KristianDD merged 9 commits into
masterfrom
kddimitrov/track-perf-decorator

Conversation

@KristianDD

@KristianDD KristianDD commented Dec 20, 2018

Copy link
Copy Markdown
Contributor

PR Checklist

What is the current behavior?

We have no means of consistent profiling and tracking performance of our methods, while executing commands.

What is the new behavior?

We will send performance analytics if usage-reporting is enabled. If --performance flag is added to the command all profiling information will be included in the command output. If --performance options is set to a file path, the profiling information will be saved inside the file.

Fixes/Implements/Closes #4243

Comment thread lib/services/performance-service.ts
Comment thread lib/services/performance-service.ts Outdated
let methodArgs;

try {
methodArgs = JSON.stringify(args, this.getCircularReplacer());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just use something like circular-json (we already have it in our dependencies tree)

Comment thread lib/common/decorators.ts Outdated
const performanceService: IPerformanceService = injector.resolve("performanceService");

//needed for the returned function to have the same name as the original - used in hooks decorator
const tempObject = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tempObject -> functionWrapper

Comment thread lib/options.ts
Comment thread lib/common/decorators.ts Outdated
return result;
}
};
descriptor.value = tempObject[originalMethod.name];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little bit risky, we need to cover it with unit tests for not hiding exceptions, results, not affecting the call stack and so on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add tests for the decorator once the implementation is final.

Comment thread lib/common/helpers.ts Outdated
return JSON.stringify(value, replacer, space || 2);
}

export function getFormattedDate(): string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFormattedDate in which format? As far as I see it makes the date strings orderable and it could be something like getOrderableDateString. Also, we could make it accept the Date as an argument in order to make it unit testable.

Comment thread lib/common/decorators.ts

export function performanceLog(injector?: IInjector): any {
injector = injector || $injector;
return function (target: any, propertyKey: string, descriptor: PropertyDescriptor): any {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we skip this logic based on $options.performance?

@KristianDD
KristianDD force-pushed the kddimitrov/track-perf-decorator branch from f22993f to 7d636c0 Compare January 14, 2019 15:15
@KristianDD
KristianDD force-pushed the kddimitrov/track-perf-decorator branch from 7358b16 to 5265bba Compare January 15, 2019 14:51
@KristianDD
KristianDD force-pushed the kddimitrov/track-perf-decorator branch from 5265bba to e97247e Compare January 15, 2019 15:00
@KristianDD

Copy link
Copy Markdown
Contributor Author

run ci

@KristianDD
KristianDD merged commit f83c95e into master Jan 15, 2019
@KristianDD
KristianDD deleted the kddimitrov/track-perf-decorator branch January 15, 2019 16:39
dtopuzov added a commit to NativeScript/nativescript-cli-tests that referenced this pull request Jan 16, 2019
Logs from hooks not longer displayed in output after NativeScript/nativescript-cli#4242

This is expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants