Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(language-service): add perf tracing to LanguageService #41401

Closed
wants to merge 1 commit into from

Conversation

zarend
Copy link
Contributor

@zarend zarend commented Mar 31, 2021

Adds perf tracing for the public methods in LanguageService. If the log level is verbose or higher,
trace performance results to the tsServer logger. This logger is implemented on the extension side
in angular/vscode-ng-language-service.

@zarend zarend added action: review The PR is still awaiting reviews from at least one requested reviewer merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 31, 2021
@zarend zarend requested a review from alxhub March 31, 2021 17:15
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Mar 31, 2021
@google-cla google-cla bot added the cla: yes label Mar 31, 2021
@zarend zarend added area: language-service Issues related to Angular's VS Code language service and removed cla: yes labels Mar 31, 2021
@ngbot ngbot bot added this to the Backlog milestone Mar 31, 2021
@pullapprove pullapprove bot requested a review from kyliau March 31, 2021 17:15
@google-cla google-cla bot added the cla: yes label Mar 31, 2021
@zarend zarend added the target: patch This PR is targeted for the next patch release label Mar 31, 2021
@zarend zarend removed the request for review from kyliau March 31, 2021 18:04
@pullapprove pullapprove bot requested a review from kyliau March 31, 2021 18:04
@zarend
Copy link
Contributor Author

zarend commented Mar 31, 2021

Note to caretaker: this should not require presubmit, since it does not affect google3

@zarend
Copy link
Contributor Author

zarend commented Mar 31, 2021

@alxhub technically we're not supposed to merge features into patch because of semantic versioning. The commit message uses feat, how do you feel about changing it to perf to get around that problem?

It doesn't seem like a big deal in this case, and I wouldn't consider perf logging to be a full feature.

@AndrewKushnir
Copy link
Contributor

Adding the "blocked" label for now, since the commit message refers to this change as a feature, but we can not merge features into patch branch (and make them available in patch versions) according to SemVer.

Adds perf tracing for the public methods in LanguageService. If the log level is verbose or higher,
trace performance results to the tsServer logger. This logger is implemented on the extension side
in angular/vscode-ng-language-service.
@zarend zarend removed action: review The PR is still awaiting reviews from at least one requested reviewer state: blocked labels Apr 1, 2021
@zarend zarend removed the request for review from kyliau April 1, 2021 17:04
@pullapprove pullapprove bot requested a review from kyliau April 1, 2021 17:04
this.compilerFactory.registerLastKnownProgram();

const logger = this.project.projectService.logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const logger = this.project.projectService.logger;
const {logger} = this.project.projectService;


const logger = this.project.projectService.logger;
if (logger.hasLevel(ts.server.LogLevel.verbose)) {
logger.perftrc(`LanguageService#${PerfPhase[phase]}: ${
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.perftrc(`LanguageService#${PerfPhase[phase]}: ${
logger.perftrc(`NgLanguageService#${PerfPhase[phase]}: ${

Copy link
Contributor

Choose a reason for hiding this comment

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

or just NGLS? TypeScript's language service also logs to the same file, it's better to differentiate the two.

@kyliau
Copy link
Contributor

kyliau commented Apr 1, 2021

Since there's no change in user facing features, I suggest we rename the commit to refactor.

@kyliau kyliau added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 1, 2021
@zarend zarend removed action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Apr 1, 2021
@zarend zarend added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 1, 2021
@atscott atscott added the target: patch This PR is targeted for the next patch release label Apr 2, 2021
@atscott
Copy link
Contributor

atscott commented Apr 2, 2021

Closed by commit 7b0a800

@atscott atscott closed this Apr 2, 2021
atscott pushed a commit that referenced this pull request Apr 2, 2021
Adds perf tracing for the public methods in LanguageService. If the log level is verbose or higher,
trace performance results to the tsServer logger. This logger is implemented on the extension side
in angular/vscode-ng-language-service.

PR Close #41401
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: language-service Issues related to Angular's VS Code language service cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants