Repository: go-logr/logr
Pull request #66 (2021-08-11 08:35:04)

export Logger.Sink
This is necessary to support the "breaking glass" use cases. Two
examples for that, access to the underlying implementation and a
custom With* function, get implemented for funcr and tested with
runable examples.

Fixes: #63 

pohly: That's why I filed issue #67 and referenced the apidiff testing from klog :grin:  I had a quick look right now and tried to run apidiff manually, but must have done something wrong because it didn't even warn about some intentionally API break that I introduced for testing the tool.

FWIW, I also think the change is safe.
thockin: hm it seems to work for me.  let me see.

Patch:
diff --git a/.github/workflows/apidiff.yaml b/.github/workflows/apidiff.yaml
deleted file mode 100644
index 340fffb..0000000
--- a/.github/workflows/apidiff.yaml
+++ /dev/null
@@ -1,29 +0,0 @@
-name: Run apidiff
-
-on: [ pull_request ]
-
-jobs:
-  apidiff:
-    runs-on: ubuntu-latest
-    if: github.base_ref
-    steps:
-      - name: Install Go
-        uses: actions/setup-go@v2
-        with:
-          go-version: 1.16.x
-      - name: Add GOBIN to PATH
-        run: echo "PATH=$(go env GOPATH)/bin:$PATH" >>$GITHUB_ENV
-      - name: Install dependencies
-        run: GO111MODULE=off go get golang.org/x/exp/cmd/apidiff
-      - name: Checkout old code
-        uses: actions/checkout@v2
-        with:
-          ref: ${{ github.base_ref }}
-          path: "old"
-      - name: Checkout new code
-        uses: actions/checkout@v2
-        with:
-          path: "new"
-      - name: APIDiff
-        run: ./_tools/apidiff.sh -d ../old
-        working-directory: "new"
diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml
deleted file mode 100644
index 5c84b2d..0000000
--- a/.github/workflows/lint.yaml
+++ /dev/null
@@ -1,28 +0,0 @@
-name: Run lint
-
-on: [ push, pull_request ]
-
-jobs:
-  lint:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Install Go
-        uses: actions/setup-go@v2
-      - name: Checkout code
-        uses: actions/checkout@v2
-      - name: Lint
-        run: |
-          docker run --rm -v `pwd`:/go/src/prj -w /go/src/prj \
-            golangci/golangci-lint golangci-lint run \
-                -v \
-                --disable-all \
-                -E deadcode \
-                -E gocritic \
-                -E gofmt \
-                -E govet \
-                -E ineffassign \
-                -E misspell \
-                -E revive \
-                -E staticcheck \
-                -E unused \
-                -E varcheck
diff --git a/README.md b/README.md
index ed5dd43..0bf3a2c 100644
--- a/README.md
+++ b/README.md
@@ -238,20 +238,10 @@ info-type logs.)
 
 #### How do I choose my keys?
 
-Keys are fairly flexible, and can hold more or less any string
-value. For best compatibility with implementations and consistency
-with existing code in other projects, there are a few conventions you
-should consider.
-
 - Make your keys human-readable.
 - Constant keys are generally a good idea.
 - Be consistent across your codebase.
 - Keys should naturally match parts of the message string.
-- Use lower case for simple keys and
-  [lowerCamelCase](https://en.wiktionary.org/wiki/lowerCamelCase) for
-  more complex ones. Kubernetes is one example of a project that has
-  [adopted that
-  convention](https://github.com/kubernetes/community/blob/HEAD/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments).
 
 While key names are mostly unrestricted (and spaces are acceptable),
 it's generally a good idea to stick to printable ascii characters, or at
diff --git a/_tools/apidiff.sh b/_tools/apidiff.sh
deleted file mode 100755
index 81ec70a..0000000
--- a/_tools/apidiff.sh
+++ /dev/null
@@ -1,127 +0,0 @@
-#!/usr/bin/env bash
-
-# Copyright 2020 The Kubernetes Authors.
-# Copyright 2021 The logr Authors.
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-set -o errexit
-set -o nounset
-set -o pipefail
-
-function usage {
-  local script="$(basename $0)"
-
-  echo >&2 "Usage: ${script} [-r <branch|tag> | -d <dir>]
-
-This script should be run at the root of a module.
-
--r <branch|tag>
-  Compare the exported API of the local working copy with the
-  exported API of the local repo at the specified branch or tag.
-
--d <dir>
-  Compare the exported API of the local working copy with the
-  exported API of the specified directory, which should point
-  to the root of a different version of the same module.
-
-Examples:
-  ${script} -r master
-  ${script} -r v1.10.0
-  ${script} -r release-1.10
-  ${script} -d /path/to/historical/version
-"
-  exit 1
-}
-
-ref=""
-dir=""
-while getopts r:d: o
-do case "$o" in
-  r)    ref="$OPTARG";;
-  d)    dir="$OPTARG";;
-  [?])  usage;;
-  esac
-done
-
-# If REF and DIR are empty, print usage and error
-if [[ -z "${ref}" && -z "${dir}" ]]; then
-  usage;
-fi
-# If REF and DIR are both set, print usage and error
-if [[ -n "${ref}" && -n "${dir}" ]]; then
-  usage;
-fi
-
-if ! which apidiff > /dev/null; then
-  echo "Installing golang.org/x/exp/cmd/apidiff"
-  pushd "${TMPDIR:-/tmp}" > /dev/null
-    GO111MODULE=off go get golang.org/x/exp/cmd/apidiff
-  popd > /dev/null
-fi
-
-output=$(mktemp -d -t "apidiff.output.XXXX")
-cleanup_output () { rm -fr "${output}"; }
-trap cleanup_output EXIT
-
-# If ref is set, clone . to temp dir at $ref, and set $dir to the temp dir
-clone=""
-base="${dir}"
-if [[ -n "${ref}" ]]; then
-  base="${ref}"
-  clone=$(mktemp -d -t "apidiff.clone.XXXX")
-  cleanup_clone_and_output () { rm -fr "${clone}"; cleanup_output; }
-  trap cleanup_clone_and_output EXIT
-  git clone . -q --no-tags "${clone}"
-  git -C "${clone}" co "${ref}"
-  dir="${clone}"
-fi
-
-pushd "${dir}" >/dev/null
-  echo "Inspecting API of ${base}"
-  go list ./... > packages.txt
-  for pkg in $(cat packages.txt); do
-    mkdir -p "${output}/${pkg}"
-    apidiff -w "${output}/${pkg}/apidiff.output" "${pkg}"
-  done
-popd >/dev/null
-
-retval=0
-
-echo "Comparing with ${base}"
-for pkg in $(go list ./...); do
-  # New packages are ok
-  if [ ! -f "${output}/${pkg}/apidiff.output" ]; then
-    continue
-  fi
-
-  # Check for incompatible changes to previous packages
-  incompatible=$(apidiff -incompatible "${output}/${pkg}/apidiff.output" "${pkg}")
-  if [[ -n "${incompatible}" ]]; then
-    echo >&2 "FAIL: ${pkg} contains incompatible changes:
-${incompatible}
-"
-    retval=1
-  fi
-done
-
-# Check for removed packages
-removed=$(comm -23 "${dir}/packages.txt" <(go list ./...))
-if [[ -n "${removed}" ]]; then
-  echo >&2 "FAIL: removed packages:
-${removed}
-"
-  retval=1
-fi
-
-exit $retval
diff --git a/discard.go b/discard.go
index 9d92a38..389f6a3 100644
--- a/discard.go
+++ b/discard.go
@@ -22,7 +22,7 @@ package logr
 func Discard() Logger {
 	return Logger{
 		level: 0,
-		sink:  discardLogSink{},
+		Sink:  discardLogSink{},
 	}
 }
 
diff --git a/funcr/funcr.go b/funcr/funcr.go
index ad4569c..261d8ec 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -35,6 +35,14 @@ func New(fn func(prefix, args string), opts Options) logr.Logger {
 	return logr.New(newSink(fn, opts))
 }
 
+// Underlier exposes access to the underlying logging function. Since
+// callers only have a logr.Logger, they have to know which
+// implementation is in use, so this interface is less of an
+// abstraction and more of a way to test type conversion.
+type Underlier interface {
+	GetUnderlying() func(prefix, args string)
+}
+
 func newSink(fn func(prefix, args string), opts Options) logr.LogSink {
 	return &fnlogger{
 		prefix:    "",
@@ -79,6 +87,7 @@ type fnlogger struct {
 // Assert conformance to the interfaces.
 var _ logr.LogSink = &fnlogger{}
 var _ logr.CallDepthLogSink = &fnlogger{}
+var _ Underlier = &fnlogger{}
 
 func flatten(kvList ...interface{}) string {
 	if len(kvList)%2 != 0 {
@@ -296,7 +305,7 @@ func (l fnlogger) Error(err error, msg string, kvList ...interface{}) {
 // in the provided name string, but this library does not actually enforce that.
 func (l fnlogger) WithName(name string) logr.LogSink {
 	if len(l.prefix) > 0 {
-		l.prefix += "/"
+		l.prefix = l.prefix + "/"
 	}
 	l.prefix += name
 	return &l
@@ -313,3 +322,7 @@ func (l fnlogger) WithCallDepth(depth int) logr.LogSink {
 	l.depth += depth
 	return &l
 }
+
+func (l fnlogger) GetUnderlying() func(prefix, args string) {
+	return l.write
+}
diff --git a/logr.go b/logr.go
index f0a9cc5..eecdaf8 100644
--- a/logr.go
+++ b/logr.go
@@ -126,7 +126,6 @@ limitations under the License.
 //   * be constant (not dependent on input data)
 //   * contain only printable characters
 //   * not contain whitespace or punctuation
-//   * use lower case for simple keys and lowerCamelCase for more complex ones
 //
 // These guidelines help ensure that log data is processed properly regardless
 // of the log implementation.  For example, log implementations will try to
@@ -162,6 +161,33 @@ limitations under the License.
 //   type Underlier interface {
 //       GetUnderlying() <underlying-type>
 //   }
+//
+// Logger exports the Sink field to enable type assertions like this:
+//   func DoSomethingWithImpl(log logr.Logger) {
+//       if underlier, ok := log.Sink(impl.Underlier) {
+//          implLogger := underlier.GetUnderlying()
+//          ...
+//       }
+//   }
+//
+// Custom `With*` functions can be implemented by copying the complete
+// Logger struct and replacing the Sink field in the copy:
+//   // WithFooBar changes the foobar parameter in the log sink and returns a new logger with that modified sink.
+//   // It does nothing for loggers where the sink doesn't support that parameter.
+//   func WithFoobar(log logr.Logger, foobar int) logr.Logger {
+//      if foobarLogSink, ok := log.Sink(FoobarSink); ok {
+//         log.Sink = foobarLogSink.WithFooBar(foobar)
+//      }
+//      return log
+//   }
+//
+// Don't use New to construct a new Logger with a LogSink retrieved
+// from an existing Logger. Source code attribution might not work
+// correctly and unexported fields in Logger get lost.
+//
+// Beware that the same LogSink instance may be shared by different
+// logger instances. Calling functions that modify the LogSink will
+// affect all of those.
 package logr
 
 import (
@@ -172,7 +198,7 @@ import (
 // implementing LogSink, rather than end users.
 func New(sink LogSink) Logger {
 	logger := Logger{
-		sink: sink,
+		Sink: sink,
 	}
 	if withCallDepth, ok := sink.(CallDepthLogSink); ok {
 		logger.withCallDepth = withCallDepth
@@ -186,8 +212,13 @@ func New(sink LogSink) Logger {
 // to a LogSink.  Implementations of LogSink should provide their own
 // constructors that return Logger, not LogSink.
 type Logger struct {
+	// Sink is the underlying logger implementation and normally
+	// should not be accessed directly. The field gets exported to
+	// support the implementation of custom extensions (see "Break
+	// Glass" in the package documentation).
+	Sink LogSink
+
 	level         int
-	sink          LogSink
 	withCallDepth CallDepthLogSink
 }
 
@@ -195,7 +226,7 @@ type Logger struct {
 // flags might be used to set the logging verbosity and disable some info
 // logs.
 func (l Logger) Enabled() bool {
-	return l.sink.Enabled(l.level)
+	return l.Sink.Enabled(l.level)
 }
 
 // Info logs a non-error message with the given key/value pairs as context.
@@ -206,7 +237,7 @@ func (l Logger) Enabled() bool {
 // keys and arbitrary values.
 func (l Logger) Info(msg string, keysAndValues ...interface{}) {
 	if l.Enabled() {
-		l.sink.Info(l.level, msg, keysAndValues...)
+		l.Sink.Info(l.level, msg, keysAndValues...)
 	}
 }
 
@@ -219,7 +250,7 @@ func (l Logger) Info(msg string, keysAndValues ...interface{}) {
 // while the err argument should be used to attach the actual error that
 // triggered this log line, if present.
 func (l Logger) Error(err error, msg string, keysAndValues ...interface{}) {
-	l.sink.Error(err, msg, keysAndValues...)
+	l.Sink.Error(err, msg, keysAndValues...)
 }
 
 // V returns a new Logger instance for a specific verbosity level, relative to
@@ -237,7 +268,7 @@ func (l Logger) V(level int) Logger {
 // WithValues returns a new Logger instance with additional key/value pairs.
 // See Info for documentation on how key/value pairs work.
 func (l Logger) WithValues(keysAndValues ...interface{}) Logger {
-	l.sink = l.sink.WithValues(keysAndValues...)
+	l.Sink = l.Sink.WithValues(keysAndValues...)
 	return l
 }
 
@@ -247,7 +278,7 @@ func (l Logger) WithValues(keysAndValues ...interface{}) Logger {
 // contain only letters, digits, and hyphens (see the package documentation for
 // more information).
 func (l Logger) WithName(name string) Logger {
-	l.sink = l.sink.WithName(name)
+	l.Sink = l.Sink.WithName(name)
 	return l
 }
 
@@ -266,7 +297,7 @@ func (l Logger) WithCallDepth(depth int) Logger {
 	if l.withCallDepth == nil {
 		return l
 	}
-	l.sink = l.withCallDepth.WithCallDepth(depth)
+	l.Sink = l.withCallDepth.WithCallDepth(depth)
 	return l
 }
 


Test patch:
diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
deleted file mode 100644
index e99538e..0000000
--- a/.github/workflows/tests.yaml
+++ /dev/null
@@ -1,22 +0,0 @@
-name: Run tests
-
-on: [ push, pull_request ]
-
-jobs:
-  test:
-    strategy:
-      matrix:
-        go-versions: [ 1.14.x, 1.15.x, 1.16.x ]
-        platform: [ ubuntu-latest, macos-latest, windows-latest ]
-    runs-on: ${{ matrix.platform }}
-    steps:
-    - name: Install Go
-      uses: actions/setup-go@v2
-      with:
-        go-version: ${{ matrix.go-version }}
-    - name: Checkout code
-      uses: actions/checkout@v2
-    - name: Build
-      run: go build -v ./...
-    - name: Test
-      run: go test -v -race ./...
diff --git a/discard_test.go b/discard_test.go
index 55aff48..89ffb43 100644
--- a/discard_test.go
+++ b/discard_test.go
@@ -24,7 +24,7 @@ import (
 
 func TestDiscard(t *testing.T) {
 	l := Discard()
-	if _, ok := l.sink.(discardLogSink); !ok {
+	if _, ok := l.Sink.(discardLogSink); !ok {
 		t.Error("did not return the expected underlying type")
 	}
 	// Verify that none of the methods panic, there is not more we can test.
diff --git a/funcr/example_test.go b/funcr/example_test.go
new file mode 100644
index 0000000..ebd8d20
--- /dev/null
+++ b/funcr/example_test.go
@@ -0,0 +1,36 @@
+/*
+Copyright 2021 The logr Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package funcr_test
+
+import (
+	"fmt"
+
+	"github.com/go-logr/logr"
+	"github.com/go-logr/logr/funcr"
+)
+
+func ExampleUnderlier() {
+	var log logr.Logger = funcr.New(func(prefix, args string) {
+		fmt.Println(prefix, args)
+	}, funcr.Options{})
+
+	if underlier, ok := log.Sink.(funcr.Underlier); ok {
+		fn := underlier.GetUnderlying()
+		fn("hello", "world")
+	}
+	// Output: hello world
+}
diff --git a/logr_test.go b/logr_test.go
index 8d27537..9b89a20 100644
--- a/logr_test.go
+++ b/logr_test.go
@@ -103,8 +103,8 @@ func TestNew(t *testing.T) {
 	}
 	logger := New(sink)
 
-	if logger.sink == nil {
-		t.Errorf("expected sink to be set, got %v", logger.sink)
+	if logger.Sink == nil {
+		t.Errorf("expected sink to be set, got %v", logger.Sink)
 	}
 	if calledInit != 1 {
 		t.Errorf("expected sink.Init() to be called once, got %d", calledInit)
@@ -277,7 +277,7 @@ func TestWithValues(t *testing.T) {
 	if calledWithValues != 1 {
 		t.Errorf("expected sink.WithValues() to be called once, got %d", calledWithValues)
 	}
-	if p := out.sink.(*testLogSink); p == sink {
+	if p := out.Sink.(*testLogSink); p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -299,7 +299,7 @@ func TestWithName(t *testing.T) {
 	if calledWithName != 1 {
 		t.Errorf("expected sink.WithName() to be called once, got %d", calledWithName)
 	}
-	if p := out.sink.(*testLogSink); p == sink {
+	if p := out.Sink.(*testLogSink); p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -311,7 +311,7 @@ func TestWithCallDepthNotImplemented(t *testing.T) {
 	logger := New(sink)
 
 	out := logger.WithCallDepth(depthInput)
-	if p := out.sink.(*testLogSink); p != sink {
+	if p := out.Sink.(*testLogSink); p != sink {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -333,7 +333,7 @@ func TestWithCallDepthImplemented(t *testing.T) {
 	if calledWithCallDepth != 1 {
 		t.Errorf("expected sink.WithCallDepth() to be called once, got %d", calledWithCallDepth)
 	}
-	if p := out.sink.(*testCallDepthLogSink); p == sink {
+	if p := out.Sink.(*testCallDepthLogSink); p == sink {
 		t.Errorf("expected output to be different from input, got in=%p, out=%p", sink, p)
 	}
 }
@@ -348,7 +348,7 @@ func TestContext(t *testing.T) {
 	}
 
 	out := FromContextOrDiscard(ctx)
-	if _, ok := out.sink.(discardLogSink); !ok {
+	if _, ok := out.Sink.(discardLogSink); !ok {
 		t.Errorf("expected a discardLogSink, got %#v", out)
 	}
 
@@ -357,11 +357,11 @@ func TestContext(t *testing.T) {
 	lctx := NewContext(ctx, logger)
 	if out, err := FromContext(lctx); err != nil {
 		t.Errorf("unexpected error: %v", err)
-	} else if p := out.sink.(*testLogSink); p != sink {
+	} else if p := out.Sink.(*testLogSink); p != sink {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 	out = FromContextOrDiscard(lctx)
-	if p := out.sink.(*testLogSink); p != sink {
+	if p := out.Sink.(*testLogSink); p != sink {
 		t.Errorf("expected output to be the same as input, got in=%p, out=%p", sink, p)
 	}
 }