Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logr Interoperability #1

Merged
merged 6 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 32 additions & 26 deletions .github/workflows/build_and_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ on:
push:
tags:
branches:
- main
pull_request:
branches:

Expand All @@ -27,25 +28,25 @@ jobs:
working-directory: ${{ matrix.go-module }}

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- name: Set up Go ${{ matrix.go-version }}
- name: Set up Go ${{ matrix.go-version }} ${{ matrix.go-module }}
uses: actions/setup-go@v4
with:
go-version: ${{ matrix.go-version }}

- name: Display Go version
- name: Display Go version ${{ matrix.go-version }} ${{ matrix.go-module }}
run: go version

- name: Install dependencies
- name: Install dependencies ${{ matrix.go-version }} ${{ matrix.go-module }}
run: |
go get -t -u golang.org/x/tools/cmd/cover
go mod download

- name: Build
- name: Build ${{ matrix.go-version }} ${{ matrix.go-module }}
run: go build -v ./...

- name: Test
- name: Test ${{ matrix.go-version }} ${{ matrix.go-module }}
run: go test -v ./...

# Build and Test as a workspace, create unified test coverage file
Expand All @@ -56,29 +57,34 @@ jobs:
go-version: [ '1.21' ]

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- name: Set up Go ${{ matrix.go-version }}
uses: actions/setup-go@v4
with:
go-version: ${{ matrix.go-version }}
- name: Set up Go ${{ matrix.go-version }}
uses: actions/setup-go@v4
with:
go-version: ${{ matrix.go-version }}

- name: Display Go version
run: go version
- name: Display Go version ${{ matrix.go-version }}
run: go version

- name: Create workspace
run: |
go work init .
go work use ./examples
go work use ./otel
- name: Create workspace ${{ matrix.go-version }}
run: |
go work init .
go work use ./examples
go work use ./otel

- name: Install dependencies ${{ matrix.go-version }}
run: |
go get -t -u golang.org/x/tools/cmd/cover
go mod download

- name: Build
run: go build -v ./... ./otel/... ./examples/...
- name: Build ${{ matrix.go-version }}
run: go build -v ./... ./otel/... ./examples/...

- name: Test
run: go test -v -race -coverprofile=coverage.out -covermode=atomic ./... ./otel/... ./examples/...
- name: Test ${{ matrix.go-version }}
run: go test -v -race -coverprofile=coverage.out -covermode=atomic ./... ./otel/... ./examples/...

- name: Upload coverage reports to Codecov
uses: codecov/codecov-action@v3
env:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
- name: Upload coverage reports to Codecov ${{ matrix.go-version }}
uses: codecov/codecov-action@v3
env:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
15 changes: 13 additions & 2 deletions .github/workflows/golangci-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,21 @@ jobs:
golangci:
name: lint
runs-on: ubuntu-latest
strategy:
matrix:
go-module: [ '.', './otel', './examples']

defaults:
run:
working-directory: ${{ matrix.go-module }}

steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v4
with:
go-version: 'stable'
cache: false
- name: golangci-lint
- name: Github-action golangci-lint ${{ matrix.go-module }}
uses: golangci/golangci-lint-action@v3
with:
# Require: The version of golangci-lint to use.
Expand All @@ -35,7 +43,7 @@ jobs:
#
# Note: By default, the `.golangci.yml` file should be at the root of the repository.
# The location of the configuration file can be changed by using `--config=`
# args: --timeout=30m --config=/my/path/.golangci.yml --issues-exit-code=0
# args: --timeout=30m --config=/my/path/.golangci.yml --issues-exit-code=0

# Optional: show only new issues if it's a pull request. The default value is `false`.
only-new-issues: true
Expand All @@ -52,3 +60,6 @@ jobs:

# Optional: The mode to install golangci-lint. It can be 'binary' or 'goinstall'.
# install-mode: "goinstall"

- name: Manual golangci-lint ${{ matrix.go-module }}
run: golangci-lint run ./...
14 changes: 12 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Automatically read any custom context values, such as OpenTelemetry TraceID.
This library supports two different workflows for using slog and context.
These workflows can be used individually or together at the same time.

##### Attributes Extracted from Context Workflow:
#### Attributes Extracted from Context Workflow:

Using the `slogcontext.Handler` lets us `Prepend` and `Append` attributes to
log lines, even when a logger is not passed into a function or in code we don't
Expand All @@ -31,13 +31,23 @@ prepended or appended to all log lines using that context. For example, the
(OpenTelemetry) TraceID and SpanID, and add them to the log record, while also
annotating the Span with an error code if the log is at error level.

##### Logger in Context Workflow:
#### Logger in Context Workflow:

Using `ToCtx` and `Logger` lets us store the logger itself within a context,
and get it back out again. Wrapper methods `With`/`WithGroup`/`Debug`/`Info`/
`Warn`/`Error`/`Log`/`LogAttrs` let us work directly with a logger residing
with the context (or the default logger if no logger is stored in the context).

#### Compatibility with both Slog and Logr
slog-context is compatible with both standard library [slog](https://pkg.go.dev/log/slog)
and with [logr](https://github.com/go-logr/logr), which is an alternative
logging api/interface/frontend.

If only slog is used, only `*slog.Logger`'s will be stored in the context.
If both slog and logr are used, `*slog.Logger` will be automatically converted
to a `logr.Logger` as needed, and vice versa. This allows full interoperability
down the stack and with any libraries that use either slog-context or logr.
Comment on lines +41 to +49
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pohly @thockin hey guys, i have a few requests:

  1. Can you please make a new versioned release of logr, so I can update my go mod for it?
  2. Are you good with this section added to my readme?
  3. If you have time, I wouldn't mind a review of my library. In particular I am debating the names I've chosen, like for putting the logger in (ToCtx) and getting it out again (Logger).
    They would all be prefixed with slogcontext in user code...
    Putting the logger into a context and getting a context back could be: ToCtx, LoggerToCtx, WithLogger, CtxWithLogger, NewCtx, WithCtx, or just Ctx
    Getting it out again could be: Logger, ToLogger, CtxToLogger, LoggerFromCtx, WithLogger, or FromCtx

Copy link

@thockin thockin Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. let's not rush (or we can do an rc1). I already found at least 1 (minor, because @pohly is awesome) bug, and I have proposed funcr support for slog.

  2. yes

  3. the whole lib or just the PR? I am known for being opinionated so be careful what you ask for. :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Ok
  2. Thanks
  3. As much as you have time for. I may not take all advice, but I will consider everything. My priority would be on settling the external api (naming and function signatures) for my whole lib, followed by everything else. The library is still new and still version 0.x.x, so there is room to change things.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README looks good to me.

I'll leave 3 to @thockin - he's better at criticizing naming than I am 😝


## Install
`go get github.com/veqryn/slog-context`

Expand Down
19 changes: 10 additions & 9 deletions ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,18 @@ package slogcontext
import (
"context"
"log/slog"
)

// Logger key for context.valueCtx
type ctxKey struct{}
"github.com/go-logr/logr"
)

// ToCtx returns a copy of ctx with the logger attached.
// The parent context will be unaffected.
// Passing in a nil logger will force future calls of Logger(ctx) on the
// returned context to return the slog.Default() logger.
func ToCtx(parent context.Context, logger *slog.Logger) context.Context {
if parent == nil {
parent = context.Background()
}
return context.WithValue(parent, ctxKey{}, logger)

return logr.NewContextWithSlogLogger(parent, logger)
}

// Logger returns the slog.Logger associated with the ctx.
Expand All @@ -26,8 +24,11 @@ func Logger(ctx context.Context) *slog.Logger {
if ctx == nil {
return slog.Default()
}
if l, ok := ctx.Value(ctxKey{}).(*slog.Logger); ok && l != nil {
return l

l := logr.FromContextAsSlogLogger(ctx)
if l == nil {
return slog.Default()
}
return slog.Default()

return l
}
6 changes: 6 additions & 0 deletions examples/.golangci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
run:
tests: false

linters:
disable:
- errcheck
2 changes: 1 addition & 1 deletion examples/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
)

require (
github.com/go-logr/logr v1.3.0 // indirect
github.com/go-logr/logr v1.4.0 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
go.opentelemetry.io/otel/metric v1.21.0 // indirect
golang.org/x/sys v0.14.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions examples/go.sum
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
github.com/go-logr/logr v1.3.0 h1:2y3SDp0ZXuc6/cjLSZ+Q3ir+QB9T/iG5yYRXqsagWSY=
github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
github.com/go-logr/logr v1.4.0 h1:wx+BduGRXjIL6VPeeb7DRX+ii7sR/ch8DlRifHR589o=
github.com/go-logr/logr v1.4.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
module github.com/veqryn/slog-context

go 1.21

require github.com/go-logr/logr v1.4.0
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
github.com/go-logr/logr v1.4.0 h1:wx+BduGRXjIL6VPeeb7DRX+ii7sR/ch8DlRifHR589o=
github.com/go-logr/logr v1.4.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
29 changes: 29 additions & 0 deletions handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,22 @@ package slogcontext

import (
"context"
"encoding/json"
"log/slog"
"slices"
"strings"
"testing"
"time"
)

type logLine struct {
Source struct {
Function string `json:"function"`
File string `json:"file"`
Line int `json:"line"`
} `json:"source"`
}

func TestHandler(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -47,6 +57,25 @@ func TestHandler(t *testing.T) {
if string(b) != expectedJSON {
t.Errorf("Expected:\n%s\nGot:\n%s\n", expectedText, string(b))
}

// Check the source location fields
tester.source = true
b, err = tester.MarshalJSON()
if err != nil {
t.Fatal(err)
}

var unmarshalled logLine
err = json.Unmarshal(b, &unmarshalled)
if err != nil {
t.Fatal(err)
}

if unmarshalled.Source.Function != "github.com/veqryn/slog-context.TestHandler" ||
!strings.HasSuffix(unmarshalled.Source.File, "slog-context/handler_test.go") ||
unmarshalled.Source.Line != 42 {
t.Errorf("Expected source fields are incorrect: %#+v\n", unmarshalled)
}
}

func TestHandlerMultipleAttrExtractor(t *testing.T) {
Expand Down
5 changes: 3 additions & 2 deletions helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ var defaultTime = time.Date(2023, 9, 29, 13, 0, 59, 0, time.UTC)
type testHandler struct {
Ctx context.Context
Record slog.Record
source bool
}

func (h *testHandler) Enabled(context.Context, slog.Level) bool {
Expand All @@ -35,7 +36,7 @@ func (h *testHandler) WithAttrs([]slog.Attr) slog.Handler {

func (h *testHandler) String() string {
buf := &bytes.Buffer{}
err := slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug}).Handle(context.Background(), h.Record)
err := slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug, AddSource: h.source}).Handle(context.Background(), h.Record)
if err != nil {
panic(err)
}
Expand All @@ -44,7 +45,7 @@ func (h *testHandler) String() string {

func (h *testHandler) MarshalJSON() ([]byte, error) {
buf := &bytes.Buffer{}
err := slog.NewJSONHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug}).Handle(context.Background(), h.Record)
err := slog.NewJSONHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug, AddSource: h.source}).Handle(context.Background(), h.Record)
if err != nil {
return nil, err
}
Expand Down
2 changes: 2 additions & 0 deletions otel/.golangci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
run:
tests: false