Added log information #368
Conversation
| @@ -51,6 +51,7 @@ fun isPortOpened(port: Int): Boolean = | |||
| Socket("localhost", port).use { it.isConnected } | |||
| } | |||
| catch (e: Exception) { | |||
| logger.warn(e) { "Exception occurred when checking if the port is already open" } | |||
There was a problem hiding this comment.
I would change this into a debug log: the client is asking if a port is open or not (so he expect it could be closed, it's under control and nothing to worry about).
Regarding the message:
- 'Exception occurred' is redundant information
- The closed port would be a great information to give ;)
I would code it this way:
logger.debug { "Checked port: $port is already open" }
| @@ -74,6 +75,7 @@ fun <T> retry(times: Int, delay: Long, block: () -> T): T { | |||
| return block() | |||
| } | |||
| catch (e: Exception) { | |||
| logger.warn(e) { "Exception occurred when executing a lambda until no exception is thrown or a number of times is reached" } | |||
There was a problem hiding this comment.
Here we can skip the log. This function is collecting the errors, so the client would decide to print them or not. Let him do what it thinks best. Following this strategy we could keep the minimum log statements and avoid flooding the output with information.
| @@ -100,15 +102,17 @@ fun List<String>.exec( | |||
|
|
|||
| if (!process.waitFor(timeout, SECONDS)) { | |||
| process.destroy() | |||
| logger.error { "Command timed out: $this" } | |||
There was a problem hiding this comment.
Please do not log thrown exceptions (they will be logged in the catch that take them). Check the #272 task description (which I just updated, sorry I should have been more explicit from the begining).
| error("Command timed out: $this") | ||
| } | ||
|
|
||
| val exitValue = process.exitValue() | ||
| val output = BufferedReader(InputStreamReader(process.inputStream)).readText() | ||
|
|
||
| if (fail && exitValue != 0) | ||
| if (fail && exitValue != 0) { | ||
| logger.trace { "Coded Exception occurred" } |
There was a problem hiding this comment.
Please do not log thrown exceptions (they will be logged in the catch that take them). Check the #272 task description (which I just updated).
| @@ -63,10 +66,12 @@ class JettyServletAdapter : ServerPort { | |||
| serverInstance.handler = context | |||
| serverInstance.stopAtShutdown = true | |||
| serverInstance.start() | |||
| logger.debug { "Jetty Server has started" } | |||
There was a problem hiding this comment.
As this is already logged by the Server class (that also states the Adapter used) it renders this information redundant.
| } | ||
|
|
||
| override fun shutdown() { | ||
| jettyServer?.stop() | ||
| logger.debug { "Jetty Server has shut down" } |
There was a problem hiding this comment.
As this is already logged by the Server class (that also states the Adapter used) it renders this information redundant.
| @@ -87,7 +92,7 @@ class JettyServletAdapter : ServerPort { | |||
|
|
|||
| if (features.contains(ServerFeature.ZIP)) | |||
| context.insertHandler(GzipHandler()) | |||
|
|
|||
| logger.debug { "Server Context has successfully created" } | |||
There was a problem hiding this comment.
This information is also displayed on the Server start up banner (started server features and parameters), so it wouldn't be needed here.
| @@ -56,6 +58,7 @@ internal class RequestAdapter(private val req: HttpServletRequest) : RequestPort | |||
| map.toMap() | |||
| } | |||
| catch (e: Exception) { | |||
| logger.warn(e) { "Exception has occurred when requesting cookies" } | |||
There was a problem hiding this comment.
Good one! You could improve it further saying the action the program is going to take (returning an empty map). I.e.:
"Exception has occurred when requesting cookies. An empty map will be returned"
|
Please add '#272' to the PR title to be able to track all PRs done to that task and the other way around. Also, take a look to the build problem (the PR cannot be merged until that is fixed). I rejected a lot of your log statements. But you added a couple of really good ones! Keep on the good work (it's not about quantity... It's about quality :) I updated the task guidelines to be more precise (sorry, it was my fault). Now that you understand better the guidelines, I would love to see more PRs like this one from you :) Thank you very much for making this Toolkit a little bit better! |
#272 Add more log information
For the Pull Request to be accepted please check:
developbranch (new features) or themasterbranch (releases).