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.

Sign up for GitHub

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

Jump to bottom

proposal: log/slog: skip caller for add source #63319

Closed
Ja7ad opened this issue Oct 1, 2023 · 2 comments
Closed

proposal: log/slog: skip caller for add source #63319

Ja7ad opened this issue Oct 1, 2023 · 2 comments
Labels
Proposal
Milestone
Proposal

Comments

@Ja7ad
Copy link

Ja7ad commented Oct 1, 2023 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.21.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/javad/.cache/go-build'
GOENV='/home/javad/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/javad/go/pkg/mod'
GONOPROXY='git.sensifia.vc'
GONOSUMDB='git.sensifia.vc'
GOOS='linux'
GOPATH='/home/javad/go'
GOPRIVATE='git.sensifia.vc'
GOPROXY='https://goproxy.io,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/home/javad/go/src/zapper/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2238607536=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I wrap slog methods for send specific log with custom feature.

for example :

158: func (l *Log) Error(toSentry bool, msg string, keyValue ...any) {
159:	if toSentry {
160:		defer l.sentry.Flush(_defaultSentryFlushTimeout)
161:		l.sentry.CaptureMessage(msg, nil, nil)
162:	}
163:	l.slog.Error(msg, keyValue...)
}

What did you expect to see?

But AddSource send call function line 163 in logger.go,If you have to send called line in logger_test.go.

func TestLog_Error(t *testing.T) {
	logger := setup(t)
	logger.Error(false, "error example", "test", 2)
}

Output:

time=2023-10-01T09:18:06.586+03:30 level=ERROR source=/home/javad/go/src/microservice/logger/logger.go:163 msg="error example" test=2

What did you see instead?

I think you should add a skip caller option to the HandlerOptions struct{} to prevent a call in the first path. For example, Zap has a method named zap.AddCallerSkip(1) to prevent a call in the first path.
@Ja7ad Ja7ad changed the title log/slog: skip caller of add source log/slog: skip caller for add source Oct 1, 2023
@seankhliao seankhliao changed the title log/slog: skip caller for add source proposal: log/slog: skip caller for add source Oct 1, 2023
@gopherbot gopherbot added this to the Proposal milestone Oct 1, 2023
@empire
Copy link
Contributor

empire commented Oct 2, 2023 •
edited

Based on my observation, this proposal seems to be the same as #59145.

The idea has been declined in the past.

I believe it would be a mistake to tie the call depth into the logger as a WithCallDepth method implies. More generally we went through the whole problem with WithContext of trying to provide a fluent API, and we walked away from that. I think we would need a very good reason to go back to fluent-style APIs. Let's focus on solutions that don't overload Logger with extra state.

As per the recommendation of @jba, it is feasible to write while using the following guidelines.

why wouldn't the approach in #59145 (comment) work?

@Ja7ad
Copy link
Author

Ja7ad commented Oct 2, 2023

Based on my observation, this proposal seems to be the same as #59145.

The idea has been declined in the past.

I believe it would be a mistake to tie the call depth into the logger as a WithCallDepth method implies. More generally we went through the whole problem with WithContext of trying to provide a fluent API, and we walked away from that. I think we would need a very good reason to go back to fluent-style APIs. Let's focus on solutions that don't overload Logger with extra state.

As per the recommendation of @jba, it is feasible to write while using the following guidelines.

why wouldn't the approach in #59145 (comment) work?

Thank you, Fixed in #59145 (comment)

@Ja7ad Ja7ad closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
3 participants
@empire @gopherbot @Ja7ad