Merge pull request #34 from wsargent/patch-2

High level feedback of log-levels.md

Author: ktoso

docs/log-levels.md

@@ -1,6 +1,6 @@
 # Log Levels
 
-This guide serves as guidelines for library authors with regards to what [SwiftLog](https://github.com/apple/swift-log) log levels are appropriate for use in libraries, and in what situations to use what level.
+This guide serves as guidelines for library authors with regards to what [SwiftLog](https://github.com/apple/swift-log) log levels are appropriate for use in libraries, and in which situations to use which level.
 
 ## Guidelines for Libraries
 
@@ -14,28 +14,29 @@ SwiftLog defines the following 7 log levels via the [`Logger.Level` enum](https:
 * `error`
 * `critical`
 
-Out of those, only levels _less severe than_ info (exclusive) are generally okay to be used by libraries.
+Out of those, only levels _less severe than_ info (exclusively) are generally okay to be used by libraries.
+
 In the following section we'll explore how to use them in practice.
 
 ### Recommended log levels
 
 It is always fine for a library to log at `trace` and `debug` levels, and these two should be the primary levels any library is logging at.
 
-`trace` is the finest log level, and end-users of a library will not usually use it unless debugging very specific issues. You should consider it as a way for library developers to "log everything we could possibly need to diagnose a hard to reproduce bug." It is expected to take a toll on the performance of a system, and developers can assume trace level logging will not be used in production deployments, unless enabled specifically to locate some specific issue.
+`trace` is the finest log level, and end-users of a library will not usually use it unless debugging very specific issues. You should consider it as a way for library developers to "log everything we could possibly need to diagnose a hard to reproduce bug."  Unrestricted logging at `trace` level may take a toll on the performance of a system, and developers can assume trace level logging will not be used in production deployments, unless enabled specifically to locate some specific issue.
 
 This is in contrast with `debug` which some users _may_ choose to run enabled on their production systems.
 
 > Debug level logging should be not "too" noisy. Developers should assume some production deployments may need to (or want to) run with debug level logging enabled. 
 >
 > Debug level logging should not completely undermine the performance of a production system.
 
-As such, `debug` logging should not be overly noisy. It should provide a high value understanding of what is going on in the library for end users without diving deep into its internals. This is what `trace` is intended for.
+As such, `debug` logging should provide a high value understanding of what is going on in the library for end users, using domain relevant language.  Logging at `debug` level should not be overly noisy or dive deep into internals; this is what `trace` is intended for.
 
-Use `warning` level sparingly. Whenever possible, try to rather return or throw `Error` to end users that are descriptive enough so they can inspect, log them and figure out the issue. Potentially they may then enable debug logging to find out more about the issue.
+Use `warning` level sparingly. Whenever possible, try to rather return or throw `Error` to end users that are descriptive enough so they can inspect, log them and figure out the issue. Potentially, they may then enable debug logging to find out more about the issue.
 
 It is okay to log a `warning` "once", for example on system startup. This may include some one off "more secure configuration is available, try upgrading to it!" log statement upon a server's startup. You may also log warnings from background processes, which otherwise have no other means of informing the end user about some issue.
 
-Logging on `error` level is similar to warnings: prefer to avoid doing so whenever possible. Instead, report errors via your library's API. For example, it is _not_ a good idea to log "connection failed" from an HTTP client. Perhaps the end-user intended to make this request to a known offline server to _confirm_ it is offline? From their perspective this connection error is not a "real" error, it is just what they expected -- as such the HTTP client should return or throw such an error, but _not_ log it.
+Logging on `error` level is similar to warnings: prefer to avoid doing so whenever possible. Instead, report errors via your library's API. For example, it is _not_ a good idea to log "connection failed" from an HTTP client. Perhaps the end-user intended to make this request to a known offline server to _confirm_ it is offline? From their perspective, this connection error is not a "real" error, it is just what they expected -- as such the HTTP client should return or throw such an error, but _not_ log it.
 
 Do also note that in situations when you decide to log an error, be mindful of error rates. Will this error potentially be logged for every single operation while some network failure is happening? Some teams and companies have alerting systems set up based on the rate of errors logged in a system, and if it exceeds some threshold it may start calling and paging people in the middle of the night. When logging at error level, consider if the issue indeed is something that should be waking up people at night. You may also want to consider offering configuration in your library: "at what log level should this issue be reported?" This can come in handy in clustered systems which may log network failures themselves, or depend on external systems detecting and reporting this.
 
@@ -48,13 +49,13 @@ Some libraries and situations may not be entirely clear with regards to what log
 `trace` level logging:
 
 - Could include various additional information about a request, such as various diagnostics about created data structures, the state of caches or similar, which are created in order to serve a request.
-- Could include "begin operation" and "end operation" logs.
-  - However, please consider using [swift-distributed-tracing](https://github.com/apple/swift-distributed-tracing) to instrument "begin" and "end" events, as tracing can be more efficient than logging. Logging may need to create string representations of the durations, log levels, and other fields.
+- Could include "begin operation" and "end operation" logging statements.
+- However, please consider using [swift-distributed-tracing](https://github.com/apple/swift-distributed-tracing) to instrument "begin" and "end" events, as tracing can be more efficient than logging. Logging may need to create string representations of the durations, log levels, and other fields.
 
 `debug` level logging:
 
 - May include a single log statement for opening a connection, accepting a request, and so on.
-- It can include a _high level_ overview of what is going on in the library. For example: "started work, processing step X, finished work X, result code 200".
+- It can include a _high level_ overview of control flow in an operation. For example: "started work, processing step X, made X decision, finished work X, result code 200".  This overview may consist of high cardinality structured data.
 
 ### Log levels to avoid
 
@@ -104,11 +105,11 @@ And a minor yet important hint: avoid inserting newlines and other control chara
 
 #### Structured Logging (Semantic Logging)
 
-Libraries may want to embrace the structured logging style.
+Libraries may want to embrace the structured logging style, which renders logs in a [semi-structured data format](https://en.wikipedia.org/wiki/Semi-structured_data).
 
-It is a fantastic pattern which makes consuming logs in automated systems, and through grepping and other means much easier and future proof.
+It is a fantastic pattern which makes it easier and more reliable for automated code to process logged information.
 
-Consider the following not structured log statement:
+Consider the following "not structured" log statement:
 
 ```swift
 // NOT structured logging style
@@ -137,7 +138,7 @@ log.info("Accepted connection", metadata: [
 // <date> info [connection.id:?,connection.peer:?, connections.total:?] Accepted connection
 ```
 
-This structured log can be formatted, depending on the logging backend, slightly differently on various systems. Even in the simple string representation of such log, we'd be able to grep for `connections.total: 100` rather than having to guess the correct string.
+This structured log can be formatted, depending on the logging backend, slightly differently on various systems. Even in the simple string representation of such a log, we'd be able to grep for `connections.total: 100` rather than having to guess the correct string.
 
 Also, since the message now does not contain all that much "human readable wording", it is less prone to randomly change from "Accepted" to "We have accepted" or vice versa. This kind of change could break alerting systems which are set up to parse and alert on specific log messages.
 
@@ -202,8 +203,8 @@ These are only general guidelines, and there always will be exceptions to these
 
 Here are a few examples of situations when logging a message on a relatively high level might still be tolerable for a library.
 
-It's permissible for a library to log at `critical` level right before a _hard crash_, as a last resort of informing the log collection systems or end-user about additional information detailing the reason for the crash. This should be _in addition to_ the message from a `fatalError` and can lead to an improved diagnosis/debugging experience for end users.
+It's permissible for a library to log at `critical` level right before a _hard_ crash of the process, as a last resort of informing the log collection systems or end-user about additional information detailing the reason for the crash. This should be _in addition to_ the message from a `fatalError` and can lead to an improved diagnosis/debugging experience for end users.
 
-Sometimes libraries may be able to detect a harmful misconfiguration of the library. For example, selecting deprecated protocol versions. In such situations it may be useful to inform users in production by issuing a `warning`. However you should ensure that the warning is not logged repeatedly! For example, it is not acceptable for an HTTP client to log a warning on every single http request using some misconfiguration of the client. It _may_ be acceptable however for the client to log such warning, for example, _once_ at configuration time, if the library has a good way to do this.
+Sometimes libraries may be able to detect a harmful misconfiguration of the library. For example, selecting deprecated protocol versions. In such situations it may be useful to inform users in production by issuing a `warning`. However you should ensure that the warning is not logged repeatedly! For example, it is not acceptable for an HTTP client to log a warning on every single http request using some misconfiguration of the client. It _may_ be acceptable however for the client to log such a warning, for example, _once_ at configuration time, if the library has a good way to do this.
 
 Some libraries may implement a "log this warning only once", "log this warning only at startup", "log this error only once an hour", or similar tricks to keep the noise level low but still informative enough to not be missed. This is, however, usually a pattern reserved for stateful long running libraries, rather than clients of databases and related persistent stores.