From b006a2c789863fbfdc163cfe1712d6856e3c5c3a Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Thu, 1 Nov 2018 15:47:01 -0700 Subject: [PATCH] Clean up logging (#3247) --- .../java/com/microsoft/signalr/HubConnection.java | 2 +- .../microsoft/signalr/OkHttpWebSocketWrapper.java | 8 +++----- .../microsoft/signalr/WebSocketTransportTest.java | 6 +++--- clients/ts/signalr/src/HttpConnection.ts | 4 ++-- clients/ts/signalr/src/HubConnection.ts | 2 +- clients/ts/signalr/src/LongPollingTransport.ts | 14 +++++++------- .../ts/signalr/src/ServerSentEventsTransport.ts | 2 +- clients/ts/signalr/src/Utils.ts | 2 +- clients/ts/signalr/src/WebSocketTransport.ts | 6 +++--- clients/ts/signalr/src/XhrHttpClient.ts | 2 +- clients/ts/signalr/tests/HttpConnection.test.ts | 10 +++++----- .../ts/signalr/tests/WebSocketTransport.test.ts | 2 +- 12 files changed, 29 insertions(+), 31 deletions(-) diff --git a/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java b/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java index d2f556b165..485137d0a6 100644 --- a/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java +++ b/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java @@ -328,7 +328,7 @@ public class HubConnection { sendHubMessage(PingMessage.getInstance()); } } catch (Exception e) { - logger.warn(String.format("Error sending ping: {}", e.getMessage())); + logger.warn("Error sending ping: {}.", e.getMessage()); // The connection is probably in a bad or closed state now, cleanup the timer so // it stops triggering pingTimer.cancel(); diff --git a/clients/java/signalr/src/main/java/com/microsoft/signalr/OkHttpWebSocketWrapper.java b/clients/java/signalr/src/main/java/com/microsoft/signalr/OkHttpWebSocketWrapper.java index f62d0a140e..0460061ec9 100644 --- a/clients/java/signalr/src/main/java/com/microsoft/signalr/OkHttpWebSocketWrapper.java +++ b/clients/java/signalr/src/main/java/com/microsoft/signalr/OkHttpWebSocketWrapper.java @@ -93,7 +93,7 @@ class OkHttpWebSocketWrapper extends WebSocketWrapper { @Override public void onFailure(WebSocket webSocket, Throwable t, Response response) { - logger.error("Websocket closed from an error: {}.", t.getMessage()); + logger.error("WebSocket closed from an error: {}.", t.getMessage()); closeSubject.onError(new RuntimeException(t)); onClose.invoke(null, t.getMessage()); checkStartFailure(); @@ -103,10 +103,8 @@ class OkHttpWebSocketWrapper extends WebSocketWrapper { // If the start task hasn't completed yet, then we need to complete it // exceptionally. if (!startSubject.hasComplete()) { - String errorMessage = "There was an error starting the Websockets transport."; - logger.error("Websocket closed from an error: {}.", errorMessage); - startSubject.onError(new RuntimeException(errorMessage)); + startSubject.onError(new RuntimeException("There was an error starting the WebSocket transport.")); } } } -} \ No newline at end of file +} diff --git a/clients/java/signalr/src/test/java/com/microsoft/signalr/WebSocketTransportTest.java b/clients/java/signalr/src/test/java/com/microsoft/signalr/WebSocketTransportTest.java index 8b9266e203..df856dcfac 100644 --- a/clients/java/signalr/src/test/java/com/microsoft/signalr/WebSocketTransportTest.java +++ b/clients/java/signalr/src/test/java/com/microsoft/signalr/WebSocketTransportTest.java @@ -17,10 +17,10 @@ import io.reactivex.Single; class WebSocketTransportTest { @Test - public void WebsocketThrowsIfItCantConnect() { + public void WebSocketThrowsIfItCantConnect() { Transport transport = new WebSocketTransport(new HashMap<>(), new DefaultHttpClient()); - RuntimeException exception = assertThrows(RuntimeException.class, () -> transport.start("http://www.example.com").blockingAwait(1, TimeUnit.SECONDS)); - assertEquals("There was an error starting the Websockets transport.", exception.getMessage()); + RuntimeException exception = assertThrows(RuntimeException.class, () -> transport.start("http://url.fake.example").blockingAwait(1, TimeUnit.SECONDS)); + assertEquals("There was an error starting the WebSocket transport.", exception.getMessage()); } @Test diff --git a/clients/ts/signalr/src/HttpConnection.ts b/clients/ts/signalr/src/HttpConnection.ts index 2d7d0f65bf..664a625b7b 100644 --- a/clients/ts/signalr/src/HttpConnection.ts +++ b/clients/ts/signalr/src/HttpConnection.ts @@ -230,7 +230,7 @@ export class HttpConnection implements IConnection { } const negotiateUrl = this.resolveNegotiateUrl(url); - this.logger.log(LogLevel.Debug, `Sending negotiation request: ${negotiateUrl}`); + this.logger.log(LogLevel.Debug, `Sending negotiation request: ${negotiateUrl}.`); try { const response = await this.httpClient.post(negotiateUrl, { content: "", @@ -324,7 +324,7 @@ export class HttpConnection implements IConnection { (transport === HttpTransportType.ServerSentEvents && !this.options.EventSource)) { this.logger.log(LogLevel.Debug, `Skipping transport '${HttpTransportType[transport]}' because it is not supported in your environment.'`); } else { - this.logger.log(LogLevel.Debug, `Selecting transport '${HttpTransportType[transport]}'`); + this.logger.log(LogLevel.Debug, `Selecting transport '${HttpTransportType[transport]}'.`); return transport; } } else { diff --git a/clients/ts/signalr/src/HubConnection.ts b/clients/ts/signalr/src/HubConnection.ts index e2e29a5e61..2ec156a7da 100644 --- a/clients/ts/signalr/src/HubConnection.ts +++ b/clients/ts/signalr/src/HubConnection.ts @@ -373,7 +373,7 @@ export class HubConnection { break; default: - this.logger.log(LogLevel.Warning, "Invalid message type: " + message.type); + this.logger.log(LogLevel.Warning, `Invalid message type: ${message.type}.`); break; } } diff --git a/clients/ts/signalr/src/LongPollingTransport.ts b/clients/ts/signalr/src/LongPollingTransport.ts index a8cbfbf8b6..7ad1c7d129 100644 --- a/clients/ts/signalr/src/LongPollingTransport.ts +++ b/clients/ts/signalr/src/LongPollingTransport.ts @@ -50,7 +50,7 @@ export class LongPollingTransport implements ITransport { this.url = url; - this.logger.log(LogLevel.Trace, "(LongPolling transport) Connecting"); + this.logger.log(LogLevel.Trace, "(LongPolling transport) Connecting."); // Allow binary format on Node and Browsers that support binary content (indicated by the presence of responseType property) if (transferFormat === TransferFormat.Binary && @@ -74,10 +74,10 @@ export class LongPollingTransport implements ITransport { // Make initial long polling request // Server uses first long polling request to finish initializing connection and it returns without data const pollUrl = `${url}&_=${Date.now()}`; - this.logger.log(LogLevel.Trace, `(LongPolling transport) polling: ${pollUrl}`); + this.logger.log(LogLevel.Trace, `(LongPolling transport) polling: ${pollUrl}.`); const response = await this.httpClient.get(pollUrl, pollOptions); if (response.statusCode !== 200) { - this.logger.log(LogLevel.Error, `(LongPolling transport) Unexpected response code: ${response.statusCode}`); + this.logger.log(LogLevel.Error, `(LongPolling transport) Unexpected response code: ${response.statusCode}.`); // Mark running as false so that the poll immediately ends and runs the close logic this.closeError = new HttpError(response.statusText || "", response.statusCode); @@ -122,15 +122,15 @@ export class LongPollingTransport implements ITransport { try { const pollUrl = `${url}&_=${Date.now()}`; - this.logger.log(LogLevel.Trace, `(LongPolling transport) polling: ${pollUrl}`); + this.logger.log(LogLevel.Trace, `(LongPolling transport) polling: ${pollUrl}.`); const response = await this.httpClient.get(pollUrl, pollOptions); if (response.statusCode === 204) { - this.logger.log(LogLevel.Information, "(LongPolling transport) Poll terminated by server"); + this.logger.log(LogLevel.Information, "(LongPolling transport) Poll terminated by server."); this.running = false; } else if (response.statusCode !== 200) { - this.logger.log(LogLevel.Error, `(LongPolling transport) Unexpected response code: ${response.statusCode}`); + this.logger.log(LogLevel.Error, `(LongPolling transport) Unexpected response code: ${response.statusCode}.`); // Unexpected status code this.closeError = new HttpError(response.statusText || "", response.statusCode); @@ -138,7 +138,7 @@ export class LongPollingTransport implements ITransport { } else { // Process the response if (response.content) { - this.logger.log(LogLevel.Trace, `(LongPolling transport) data received. ${getDataDetail(response.content, this.logMessageContent)}`); + this.logger.log(LogLevel.Trace, `(LongPolling transport) data received. ${getDataDetail(response.content, this.logMessageContent)}.`); if (this.onreceive) { this.onreceive(response.content); } diff --git a/clients/ts/signalr/src/ServerSentEventsTransport.ts b/clients/ts/signalr/src/ServerSentEventsTransport.ts index 87c5cdd91e..ee2de43ef5 100644 --- a/clients/ts/signalr/src/ServerSentEventsTransport.ts +++ b/clients/ts/signalr/src/ServerSentEventsTransport.ts @@ -37,7 +37,7 @@ export class ServerSentEventsTransport implements ITransport { Arg.isRequired(transferFormat, "transferFormat"); Arg.isIn(transferFormat, TransferFormat, "transferFormat"); - this.logger.log(LogLevel.Trace, "(SSE transport) Connecting"); + this.logger.log(LogLevel.Trace, "(SSE transport) Connecting."); // set url before accessTokenFactory because this.url is only for send and we set the auth header instead of the query string for send this.url = url; diff --git a/clients/ts/signalr/src/Utils.ts b/clients/ts/signalr/src/Utils.ts index 159219d613..6e0ae91862 100644 --- a/clients/ts/signalr/src/Utils.ts +++ b/clients/ts/signalr/src/Utils.ts @@ -33,7 +33,7 @@ export function getDataDetail(data: any, includeContent: boolean): string { } else if (typeof data === "string") { detail = `String data of length ${data.length}`; if (includeContent) { - detail += `. Content: '${data}'.`; + detail += `. Content: '${data}'`; } } return detail; diff --git a/clients/ts/signalr/src/WebSocketTransport.ts b/clients/ts/signalr/src/WebSocketTransport.ts index 826f0d818c..74db953b5a 100644 --- a/clients/ts/signalr/src/WebSocketTransport.ts +++ b/clients/ts/signalr/src/WebSocketTransport.ts @@ -36,7 +36,7 @@ export class WebSocketTransport implements ITransport { Arg.isRequired(transferFormat, "transferFormat"); Arg.isIn(transferFormat, TransferFormat, "transferFormat"); - this.logger.log(LogLevel.Trace, "(WebSockets transport) Connecting"); + this.logger.log(LogLevel.Trace, "(WebSockets transport) Connecting."); if (this.accessTokenFactory) { const token = await this.accessTokenFactory(); @@ -70,7 +70,7 @@ export class WebSocketTransport implements ITransport { // tslint:disable-next-line:variable-name webSocket.onopen = (_event: Event) => { - this.logger.log(LogLevel.Information, `WebSocket connected to ${url}`); + this.logger.log(LogLevel.Information, `WebSocket connected to ${url}.`); this.webSocket = webSocket; resolve(); }; @@ -127,7 +127,7 @@ export class WebSocketTransport implements ITransport { this.logger.log(LogLevel.Trace, "(WebSockets transport) socket closed."); if (this.onclose) { if (event && (event.wasClean === false || event.code !== 1000)) { - this.onclose(new Error(`Websocket closed with status code: ${event.code} (${event.reason})`)); + this.onclose(new Error(`WebSocket closed with status code: ${event.code} (${event.reason}).`)); } else { this.onclose(); } diff --git a/clients/ts/signalr/src/XhrHttpClient.ts b/clients/ts/signalr/src/XhrHttpClient.ts index da3b337e7b..3b27bdded5 100644 --- a/clients/ts/signalr/src/XhrHttpClient.ts +++ b/clients/ts/signalr/src/XhrHttpClient.ts @@ -72,7 +72,7 @@ export class XhrHttpClient extends HttpClient { }; xhr.onerror = () => { - this.logger.log(LogLevel.Warning, `Error from HTTP request. ${xhr.status}: ${xhr.statusText}`); + this.logger.log(LogLevel.Warning, `Error from HTTP request. ${xhr.status}: ${xhr.statusText}.`); reject(new HttpError(xhr.statusText, xhr.status)); }; diff --git a/clients/ts/signalr/tests/HttpConnection.test.ts b/clients/ts/signalr/tests/HttpConnection.test.ts index a1bb4e71c5..4ec762ccc9 100644 --- a/clients/ts/signalr/tests/HttpConnection.test.ts +++ b/clients/ts/signalr/tests/HttpConnection.test.ts @@ -108,7 +108,7 @@ describe("HttpConnection", () => { ...commonOptions, httpClient: new TestHttpClient() .on("POST", () => { - return Promise.reject("reached negotiate"); + return Promise.reject("reached negotiate."); }) .on("GET", () => ""), logger, @@ -118,14 +118,14 @@ describe("HttpConnection", () => { await expect(connection.start(TransferFormat.Text)) .rejects - .toBe("reached negotiate"); + .toBe("reached negotiate."); await expect(connection.start(TransferFormat.Text)) .rejects - .toBe("reached negotiate"); + .toBe("reached negotiate."); }, - "Failed to complete negotiation with the server: reached negotiate", - "Failed to start the connection: reached negotiate"); + "Failed to complete negotiation with the server: reached negotiate.", + "Failed to start the connection: reached negotiate."); }); it("can stop a starting connection", async () => { diff --git a/clients/ts/signalr/tests/WebSocketTransport.test.ts b/clients/ts/signalr/tests/WebSocketTransport.test.ts index e295a17cdf..4d727ee8ac 100644 --- a/clients/ts/signalr/tests/WebSocketTransport.test.ts +++ b/clients/ts/signalr/tests/WebSocketTransport.test.ts @@ -129,7 +129,7 @@ describe("WebSocketTransport", () => { TestWebSocket.webSocket.onclose(message); expect(closeCalled).toBe(true); - expect(error!).toEqual(new Error("Websocket closed with status code: 1 (just cause)")); + expect(error!).toEqual(new Error("WebSocket closed with status code: 1 (just cause).")); await expect(webSocket.send("")) .rejects