PR: documentation, defaults, AddSpanContext bug

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

internal/log/format.go

@@ -10,7 +10,6 @@ import (
  "time"
 
  "github.com/containerd/containerd/log"
- "github.com/sirupsen/logrus"
 )
 
 const TimeFormat = log.RFC3339NanoFixed
@@ -84,16 +83,3 @@ func encodeBuffer(buf *bytes.Buffer, v interface{}) ([]byte, error) {
  // encoder.Encode appends a newline to the end
  return bytes.TrimSpace(buf.Bytes()), nil
 }
-
-// GetCallerName checks if the entry appears caller caller information and returns the function name.
-//
-// This is intended to be used with "github.com/Microsoft/go-winio/pkg/etwlogrus".WithGetName.
-func GetCallerName(e *logrus.Entry) string {
- if e.Caller == nil {
- return ""
- }
- if e.Caller.Func != nil {
- return e.Caller.Func.Name()
- }
- return e.Caller.Function
-}

internal/log/hook.go

@@ -23,18 +23,21 @@ type Hook struct {
  // EncodeAsJSON formats structs, maps, arrays, slices, and [bytes.Buffer] as JSON.
  // Variables of [bytes.Buffer] will be converted to []byte.
  //
- // Default is true.
+ // Default is false.
  EncodeAsJSON bool
 
  // FormatTime specifies the format for [time.Time] variables.
- // An empty string disabled formatting.
+ // An empty string disables formatting.
+ // When disabled, the fall back will the JSON encoding, if enabled.
  //
  // Default is [github.com/containerd/containerd/log.RFC3339NanoFixed].
  TimeFormat string
 
  // Duration format converts a [time.Duration] fields to an appropriate encoding.
+ // nil disables formatting.
+ // When disabled, the fall back will the JSON encoding, if enabled.
  //
- // Default is [DurationFormatSeconds].
+ // Default is [DurationFormatString], which appends a duration unit after the value.
  DurationFormat DurationFormat
 
  // AddSpanContext adds [logfields.TraceID] and [logfields.SpanID] fields to
@@ -49,9 +52,9 @@ var _ logrus.Hook = &Hook{}
 
 func NewHook() *Hook {
  return &Hook{
- EncodeAsJSON: true,
+ EncodeAsJSON: false,
  TimeFormat: log.RFC3339NanoFixed,
- DurationFormat: DurationFormatSeconds,
+ DurationFormat: DurationFormatString,
  AddSpanContext: true,
  }
 }
@@ -75,12 +78,14 @@ func (h *Hook) Fire(e *logrus.Entry) (err error) {
 // If [Hook.EncodeAsJSON] is true, then fields that are numeric, boolean, strings, or
 // errors will be encoded via a [json.NewEncoder] in encode().
 //
-// If [Hook.TimeFormat] is empty and [Hook.EncodeAsJSON] is false, then this is a no-op.
+// If [Hook.TimeFormat] and [Hook.DurationFormat] are empty and [Hook.EncodeAsJSON] is false,
+// then this is a no-op.
 func (h *Hook) encode(e *logrus.Entry) {
  d := e.Data
 
  formatTime := h.TimeFormat != ""
- if !(h.EncodeAsJSON || formatTime) {
+ formatDuration := h.DurationFormat != nil
+ if !(h.EncodeAsJSON || formatTime || formatDuration) {
  return
  }
 
@@ -92,11 +97,20 @@ func (h *Hook) encode(e *logrus.Entry) {
  }
  }
 
- if t, ok := v.(time.Time); formatTime && ok {
- d[k] = t.Format(h.TimeFormat)
+ // encode types with dedicated formatting options first
+
+ if vv, ok := v.(time.Time); formatTime && ok {
+ d[k] = vv.Format(h.TimeFormat)
  continue
  }
 
+ if vv, ok := v.(time.Duration); formatDuration && ok {
+ d[k] = h.DurationFormat(vv)
+ continue
+ }
+
+ // general case JSON encoding
+
  if !h.EncodeAsJSON {
  continue
  }
@@ -109,14 +123,6 @@ func (h *Hook) encode(e *logrus.Entry) {
  float32, float64:
  continue
 
- case time.Duration:
- if h.DurationFormat != nil {
- if i := h.DurationFormat(vv); i != nil {
- d[k] = i
- }
- }
- continue
-
  // Rather than setting d[k] = vv.String(), JSON encode []byte value, since it
  // may be a binary payload and not representable as a string.
  // `case bytes.Buffer,*bytes.Buffer:` resolves `vv` to `interface{}`,
@@ -150,16 +156,16 @@ func (h *Hook) encode(e *logrus.Entry) {
  // hooks (ie, exporting to ETW) from firing. So add encoding errors to
  // the entry data to be written out, but keep on processing.
  d[k+"-"+logrus.ErrorKey] = err.Error()
+ // keep the original `v` as the value,
+ continue
  }
-
- // if `err != nil`, then `b == nil` and this will be the empty string
  d[k] = string(b)
  }
 }
 
 func (h *Hook) addSpanContext(e *logrus.Entry) {
  ctx := e.Context
- if ctx == nil {
+ if !h.AddSpanContext || ctx == nil {
  return
  }
  span := trace.FromContext(ctx)

internal/oc/errors.go

@@ -11,7 +11,7 @@ import (
  "google.golang.org/grpc/status"
 )
 
-// todo: break import cycle with "internal/hcs" and errors errors defined there
+// todo: break import cycle with "internal/hcs/errors.go" and reference errors defined there
 // todo: add errors defined in "internal/guest/gcserror" (Hresult does not implement error)
 
 func toStatusCode(err error) codes.Code {
@@ -23,7 +23,7 @@ func toStatusCode(err error) codes.Code {
  }
 
  switch {
- // case IsAny(err):
+ // case isAny(err):
  // return codes.Cancelled
  case isAny(err, os.ErrInvalid):
  return codes.InvalidArgument
@@ -35,23 +35,23 @@ func toStatusCode(err error) codes.Code {
  return codes.AlreadyExists
  case isAny(err, os.ErrPermission):
  return codes.PermissionDenied
- // case IsAny(err):
+ // case isAny(err):
  // return codes.ResourceExhausted
  case isAny(err, os.ErrClosed, net.ErrClosed, io.ErrClosedPipe, io.ErrShortBuffer):
  return codes.FailedPrecondition
- // case IsAny(err):
+ // case isAny(err):
  // return codes.Aborted
- // case IsAny(err):
+ // case isAny(err):
  // return codes.OutOfRange
- // case IsAny(err):
+ // case isAny(err):
  // return codes.Unimplemented
  case isAny(err, io.ErrNoProgress):
  return codes.Internal
- // case IsAny(err):
+ // case isAny(err):
  // return codes.Unavailable
  case isAny(err, io.ErrShortWrite, io.ErrUnexpectedEOF):
  return codes.DataLoss
- // case IsAny(err):
+ // case isAny(err):
  // return codes.Unauthenticated
  default:
  return codes.Unknown