docs: Add logging style guide (strangelove-ventures#69)

Author: DavidNix

README.md

@@ -14,6 +14,8 @@ See [`cmd/ibctest/README.md`](cmd/ibctest/README.md) for more details.
 Note that `ibc-test-framework` is under active development
 and we are not yet ready to commit to any stable APIs around the testing interfaces.
 
+Please read the [logging style guide](./docs/logging.md).
+
 ## Trophies
 
 Significant bugs that were more easily fixed with the `ibc-test-framework`:

chain/cosmos/chain_node.go

@@ -250,7 +250,7 @@ func (tn *ChainNode) maybeLogBlock(height int64) {
 	ctx := context.Background()
 	blockRes, err := tn.Client.Block(ctx, &height)
 	if err != nil {
-		tn.logger().Error("Get block", zap.Error(err))
+		tn.logger().Info("Failed to get block", zap.Error(err))
 		return
 	}
 	txs := blockRes.Block.Txs
@@ -259,7 +259,7 @@ func (tn *ChainNode) maybeLogBlock(height int64) {
 	}
 	pp, err := tendermint.PrettyPrintTxs(ctx, txs, tn.Client.Tx)
 	if err != nil {
-		tn.logger().Error("Pretty print block", zap.Error(err))
+		tn.logger().Info("Failed to pretty print block", zap.Error(err))
 		return
 	}
 	tn.logger().Debug(pp, zap.Int64("height", height), zap.String("block", pp))

chain/cosmos/cosmos_chain.go

@@ -254,7 +254,11 @@ func (c *CosmosChain) initializeChainNodes(testName, home string,
 			Tag:        image.Version,
 		}, docker.AuthConfiguration{})
 		if err != nil {
-			c.log.Error("Pull image", zap.Error(err))
+			c.log.Error("Failed to pull image",
+				zap.Error(err),
+				zap.String("repository", image.Repository),
+				zap.String("tag", image.Version),
+			)
 		}
 	}
 	for i := 0; i < count; i++ {

docs/logging.md

@@ -0,0 +1,73 @@
+# Logging
+
+This code uses the [zap](https://pkg.go.dev/go.uber.org/zap) logging framework.
+
+Zap is a production quality logging library that is performant and highly configurable.
+Zap produces structured logging that can be easily machine-parsed,
+so that people can easily use tooling to filter through logs,
+or logs can easily be sent to log aggregation tooling.
+
+## Conventions
+
+### Instantiation
+
+Structs should hold a reference to their own `*zap.Logger` instance, as an unexported field named `log`.
+Callers should provide the logger instance as the first argument to a `NewFoo` method, setting any extra fields with the `With` method externally.
+For example:
+
+```go
+path := "/tmp/foo"
+foo := NewFoo(log.With(zap.String("path", path)), path)
+```
+
+### Usage
+
+Log messages should begin with an uppercase letter and should not end with any punctuation.
+Log messages should be constant string literals; dynamic content is to be set as log fields.
+Names of log fields should be all lowercase `snake_case` format.
+
+```go
+// Do this:
+x.log.Debug("Updated height", zap.Int64("height", height))
+
+// Don't do this:
+x.log.Debug(fmt.Sprintf("Updated height to %d.", height))
+```
+
+Errors are intended to be logged with `zap.Error(err)`.
+Error values should only be logged when they are not returned.
+
+```go
+// Do this:
+for _, x := range xs {
+  if err := x.Validate(); err != nil {
+    y.log.Info("Skipping x that failed validation", zap.String("id", x.ID), zap.Error(err))
+    continue
+  }
+}
+
+// Don't do this:
+if err := x.Validate(); err != nil {
+  y.log.Info("X failed validation", zap.String("id", x.ID), zap.Error(err))
+  return err // Bad!
+  // Returning an error that has already been logged may cause the same error
+  // to be logged many times.
+}
+```
+
+#### Log levels
+
+Debug level messages are off by default.
+Debug messages should be used sparingly;
+they are typically only intended for consumption by developers when actively debugging an issue.
+
+Info level messages and higher are on by default.
+They may contain any general information useful to an operator.
+It should be safe to discard all info level messages if an operator so desires.
+
+Warn level messages should be used to indicate a problem that may worsen without operator intervention.
+
+Error level messages should only be used to indicate a serious problem that cannot automatically recover.
+Error level messages should be reserved for events that are worthy of paging an engineer.
+
+Do not use Fatal or Panic level messages.