Skip to content

Simplify server logger #17271

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.

Sign up for GitHub

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

Merged
4 commits merged into from
Aug 8, 2017
Merged

Simplify server logger #17271

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c41025
Simplify server logger
Jul 18, 2017
f8acc7e
Move function printProjects out of inner closure
Aug 8, 2017
a209a38
Merge branch 'master' into group
Aug 8, 2017
2de4021
Merge branch 'master' into group
Aug 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/harness/harnessLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,11 +681,11 @@ namespace Harness.LanguageService {
}

info(message: string): void {
return this.host.log(message);
this.host.log(message);
}

msg(message: string) {
return this.host.log(message);
err(message: string): void {
this.host.log(message);
}

loggingEnabled() {
Expand All @@ -700,17 +700,12 @@ namespace Harness.LanguageService {
return false;
}


endGroup(): void {
}
group() { throw ts.notImplemented(); }

perftrc(message: string): void {
return this.host.log(message);
}

startGroup(): void {
}

setTimeout(callback: (...args: any[]) => void, ms: number, ...args: any[]): any {
return setTimeout(callback, ms, args);
}
Expand Down
14 changes: 1 addition & 13 deletions src/harness/unittests/cachingInServerLSHost.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,9 @@ namespace ts {
}

function createProject(rootFile: string, serverHost: server.ServerHost): { project: server.Project, rootScriptInfo: server.ScriptInfo } {
const logger: server.Logger = {
close: noop,
hasLevel: () => false,
loggingEnabled: () => false,
perftrc: noop,
info: noop,
startGroup: noop,
endGroup: noop,
msg: noop,
getLogFileName: (): string => undefined
};

const svcOpts: server.ProjectServiceOptions = {
host: serverHost,
logger,
logger: projectSystem.nullLogger,
cancellationToken: { isCancellationRequested: () => false },
useSingleInferredProject: false,
typingsInstaller: undefined
Expand Down
18 changes: 3 additions & 15 deletions src/harness/unittests/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,6 @@ namespace ts.server {
createHash: Harness.LanguageService.mockHash,
};

const mockLogger: Logger = {
close: noop,
hasLevel(): boolean { return false; },
loggingEnabled(): boolean { return false; },
perftrc: noop,
info: noop,
startGroup: noop,
endGroup: noop,
msg: noop,
getLogFileName: (): string => undefined
};

class TestSession extends Session {
getProjectService() {
return this.projectService;
Expand All @@ -58,7 +46,7 @@ namespace ts.server {
typingsInstaller: undefined,
byteLength: Utils.byteLength,
hrtime: process.hrtime,
logger: mockLogger,
logger: projectSystem.nullLogger,
canUseEvents: true
};
return new TestSession(opts);
Expand Down Expand Up @@ -408,7 +396,7 @@ namespace ts.server {
typingsInstaller: undefined,
byteLength: Utils.byteLength,
hrtime: process.hrtime,
logger: mockLogger,
logger: projectSystem.nullLogger,
canUseEvents: true
});
this.addProtocolHandler(this.customHandler, () => {
Expand Down Expand Up @@ -475,7 +463,7 @@ namespace ts.server {
typingsInstaller: undefined,
byteLength: Utils.byteLength,
hrtime: process.hrtime,
logger: mockLogger,
logger: projectSystem.nullLogger,
canUseEvents: true
});
this.addProtocolHandler("echo", (req: protocol.Request) => ({
Expand Down
13 changes: 6 additions & 7 deletions src/harness/unittests/tsserverProjectSystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,13 @@ namespace ts.projectSystem {
}

export const nullLogger: server.Logger = {
close: () => void 0,
hasLevel: () => void 0,
close: noop,
hasLevel: () => false,
loggingEnabled: () => false,
perftrc: () => void 0,
info: () => void 0,
startGroup: () => void 0,
endGroup: () => void 0,
msg: () => void 0,
perftrc: noop,
info: noop,
err: noop,
group: noop,
getLogFileName: (): string => undefined
};

Expand Down
44 changes: 21 additions & 23 deletions src/server/editorServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,30 +931,28 @@ namespace ts.server {
return;
}

this.logger.startGroup();

let counter = 0;
counter = printProjects(this.logger, this.externalProjects, counter);
counter = printProjects(this.logger, this.configuredProjects, counter);
counter = printProjects(this.logger, this.inferredProjects, counter);

this.logger.info("Open files: ");
for (const rootFile of this.openFiles) {
this.logger.info(rootFile.fileName);
}

this.logger.endGroup();

function printProjects(logger: Logger, projects: Project[], counter: number) {
for (const project of projects) {
project.updateGraph();
logger.info(`Project '${project.getProjectName()}' (${ProjectKind[project.projectKind]}) ${counter}`);
logger.info(project.filesToString());
logger.info("-----------------------------------------------");
counter++;
this.logger.group(info => {
let counter = 0;
printProjects(this.externalProjects);
printProjects(this.configuredProjects);
printProjects(this.inferredProjects);

info("Open files: ");
for (const rootFile of this.openFiles) {
info(rootFile.fileName);
}

function printProjects(projects: Project[]) {
for (const project of projects) {
project.updateGraph();
info(`Project '${project.getProjectName()}' (${ProjectKind[project.projectKind]}) ${counter}`);
info(project.filesToString());
info("-----------------------------------------------");
counter++;
Copy link
Member

@sandersn sandersn Jul 20, 2017

Choose a reason for hiding this comment

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

Can you remove the count updating responsibility from printProjects and replace it with const counter = this.externalProjects.length + this.configuredProjects.length + this.inferredProjects.length. Then printProjects can be lifted out of this function entirely.

Copy link
Contributor

@rbuckton rbuckton Aug 8, 2017

Choose a reason for hiding this comment

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

@sandersn, counter is used on line 948, though its still feasible to do something like:

function printProjects(projects: Project[], counter: number) {
  for (const project of projects) {
    ...
    counter++;
  }
  return counter;
}

And then use it like this:

let counter = 0;
counter = printProjects(this.externalProjects, counter);
counter = printProjects(this.configuredProjects, counter);
printProjects(this.inferredProjects, counter);

}
}
return counter;
}
});

}

private findConfiguredProjectByProjectName(configFileName: NormalizedPath) {
Expand Down
64 changes: 35 additions & 29 deletions src/server/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,6 @@ namespace ts.server {
class Logger implements server.Logger {
private fd = -1;
private seq = 0;
private inGroup = false;
private firstInGroup = true;

constructor(private readonly logFilename: string,
private readonly traceToConsole: boolean,
Expand Down Expand Up @@ -172,22 +170,24 @@ namespace ts.server {
}

perftrc(s: string) {
this.msg(s, Msg.Perf);
Copy link
Contributor

@rbuckton rbuckton Aug 8, 2017

Choose a reason for hiding this comment

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

Could we instead turn Msg into a string enum? That would allow us to continue to find all references if necessary.

this.msg(s, "Perf");
}

info(s: string) {
this.msg(s, Msg.Info);
this.msg(s, "Info");
}

startGroup() {
this.inGroup = true;
this.firstInGroup = true;
err(s: string) {
this.msg(s, "Err");
}

endGroup() {
this.inGroup = false;
group(logGroupEntries: (log: (msg: string) => void) => void) {
let firstInGroup = false;
logGroupEntries(s => {
this.msg(s, "Info", /*inGroup*/ true, firstInGroup);
firstInGroup = false;
});
this.seq++;
this.firstInGroup = true;
}

loggingEnabled() {
Expand All @@ -198,26 +198,32 @@ namespace ts.server {
return this.loggingEnabled() && this.level >= level;
}

msg(s: string, type: Msg.Types = Msg.Err) {
if (this.fd >= 0 || this.traceToConsole) {
s = s + "\n";
private msg(s: string, type: string, inGroup = false, firstInGroup = false) {
if (!this.canWrite) return;

s = s + "\n";
if (!inGroup || firstInGroup) {
const prefix = Logger.padStringRight(type + " " + this.seq.toString(), " ");
if (this.firstInGroup) {
s = prefix + s;
this.firstInGroup = false;
}
if (!this.inGroup) {
this.seq++;
this.firstInGroup = true;
}
if (this.fd >= 0) {
const buf = new Buffer(s);
// tslint:disable-next-line no-null-keyword
fs.writeSync(this.fd, buf, 0, buf.length, /*position*/ null);
}
if (this.traceToConsole) {
console.warn(s);
}
s = prefix + s;
}
this.write(s);
if (!inGroup) {
this.seq++;
}
}

private get canWrite() {
return this.fd >= 0 || this.traceToConsole;
}

private write(s: string) {
if (this.fd >= 0) {
const buf = new Buffer(s);
// tslint:disable-next-line no-null-keyword
fs.writeSync(this.fd, buf, 0, buf.length, /*position*/ null);
}
if (this.traceToConsole) {
console.warn(s);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ namespace ts.server {
msg += "\n" + (<StackTraceError>err).stack;
}
}
this.logger.msg(msg, Msg.Err);
this.logger.err(msg);
}

public send(msg: protocol.Message) {
Expand Down Expand Up @@ -1955,7 +1955,7 @@ namespace ts.server {
return this.executeWithRequestId(request.seq, () => handler(request));
}
else {
this.logger.msg(`Unrecognized JSON command: ${JSON.stringify(request)}`, Msg.Err);
this.logger.err(`Unrecognized JSON command: ${JSON.stringify(request)}`);
this.output(undefined, CommandNames.Unknown, request.seq, `Unrecognized JSON command: ${request.command}`);
return { responseRequired: false };
}
Expand Down
15 changes: 2 additions & 13 deletions src/server/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,11 @@ namespace ts.server {
loggingEnabled(): boolean;
perftrc(s: string): void;
info(s: string): void;
startGroup(): void;
endGroup(): void;
msg(s: string, type?: Msg.Types): void;
err(s: string): void;
group(logGroupEntries: (log: (msg: string) => void) => void): void;
getLogFileName(): string;
}

export namespace Msg {
Copy link
Contributor

@rbuckton rbuckton Aug 8, 2017

Choose a reason for hiding this comment

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

Why not turn this into a string enum?

Copy link
Author

@ghost ghost Aug 8, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

There should only be one reference to each kind of message -- one each in perftrc(), info(), and err().

export type Err = "Err";
export const Err: Err = "Err";
export type Info = "Info";
export const Info: Info = "Info";
export type Perf = "Perf";
export const Perf: Perf = "Perf";
export type Types = Err | Info | Perf;
}

function getProjectRootPath(project: Project): Path {
switch (project.projectKind) {
case ProjectKind.Configured:
Expand Down