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

[BUG] contrib/log/slog: log/slog.Handler is replaced by internal handler after Logger.With or Logger.WithGroup call #2838

Closed
shota3506 opened this issue Aug 31, 2024 · 3 comments · Fixed by #2857
Assignees
Labels
apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed

Comments

@shota3506
Copy link

Version of dd-trace-go

v1.67.0

Describe what happened:
dd-trace-go.v1/contrib/log/slog.Handler is replaced by internal handler after Logger.With or Logger.WithGroup call.

That's because it doesn't implement its own WithAttrs and WithGroup and just uses internal handler's method.
https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/log/slog/logger.go;l=132

Describe what you expected:

slog.Logger keeps dd-trace-go.v1/contrib/log/slog.Handler after Logger.With or Logger.WithGroup call.

Steps to reproduce the issue:

You can confirm that the logger's Handler is replaced after logger.With or logger.WithGroup call.

package main

import (
	"fmt"
	"log/slog"
	"os"

	ddslog "gopkg.in/DataDog/dd-trace-go.v1/contrib/log/slog"
)

func main() {
	logger := slog.New(ddslog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{}))

	fmt.Printf("before logger.With call: %T\n", logger.Handler())
	// before With call: *slog.handler

	logger = logger.With("key", "value")

	fmt.Printf("after logger.With call: %T\n", logger.Handler())
	// after With call: *slog.JSONHandler
}

Additional environment details (Version of Go, Operating System, etc.):

N/A

@shota3506 shota3506 added the bug unintended behavior that has to be fixed label Aug 31, 2024
@github-actions github-actions bot added the needs-triage New issues that have not yet been triaged label Aug 31, 2024
@shota3506
Copy link
Author

shota3506 commented Aug 31, 2024

I addressed the issue in this #2839

As I mentioned in the PR comment, I'm not sure how I should treat Logger.WithGroup call in this case.
I would like some advice on this.

@shota3506 shota3506 changed the title [BUG] log/slog.Handler is replaced by internal handler after Logger.With or Logger.WithGroup call [BUG] contrib/log/slog: log/slog.Handler is replaced by internal handler after Logger.With or Logger.WithGroup call Sep 3, 2024
@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Sep 3, 2024
@darccio
Copy link
Member

darccio commented Sep 3, 2024

@shota3506 Thanks for reaching out. We've reviewed it and we understand your concern. We'll check it soon.

@darccio darccio removed the needs-triage New issues that have not yet been triaged label Sep 3, 2024
@shota3506
Copy link
Author

It might be a possible solution that the handler holds attributes and groups as its private fields and passes them to the inner handler at handler.Handle method. Although it will make the handler implementation complex and a little bit slow.
https://cs.opensource.google/go/x/exp/+/e7e105de:slog/handler.go;l=223-253

I thought ReplaceAttr resolves this issue, but it cannot modify the attribute's group as far as I checked
https://pkg.go.dev/golang.org/x/exp/slog#HandlerOptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed
Projects
None yet
4 participants
@darccio @rarguelloF @shota3506 and others