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

Add LogWithLevel method to logger #754

Closed
wants to merge 1 commit into from

Conversation

tmshn
Copy link
Contributor

@tmshn tmshn commented Oct 31, 2019

This PR introduces...

LogWithLevel(lvl, msg, fields...) method, which logs messages at specified log level.

This can be seen as shorthand for (but more appropriate caller depth to check())

if ce := logger.Check(lvl, msg); ce != nil {
	ce.Write(fields...)
}

Use-cases

When writing a logging middleware library for a http server, I want to let users to define log level using req/res while hiding logging implementation behind the library.

Here's an example of defining log levels using response status (for Echo).

// User specify log level
func LogLevelFromStatus(ctx echo.Context) zapcore.Level {
	status := ctx.Response().Status
	if status >= 500 {
		return zapcore.ErrorLevel
	}
	return zapcore.InfoLevel
}

With this definition, you can log like

logger.LogWithLevel(LogLevelFromStatus(ctx), "request completed", fields...)

In addition to such usecases, this opens a door to implementing custom log level as discussed in #680

Discussion point

  • Naming...some logging library name this much simpler like Log (e.g.: logrus)

@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #754 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #754      +/-   ##
=========================================
+ Coverage   98.09%   98.1%   +<.01%     
=========================================
  Files          42      42              
  Lines        2150    2159       +9     
=========================================
+ Hits         2109    2118       +9     
  Misses         33      33              
  Partials        8       8
Impacted Files Coverage Δ
sugar.go 100% <100%> (ø) ⬆️
logger.go 94.64% <100%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6015e1...78a1ca8. Read the comment docs.

@prashantv
Copy link
Collaborator

This can be seen as shorthand for (but more appropriate caller depth to check())

if ce := logger.Check(lvl, msg); ce != nil {
	ce.Write(fields...)
}

Can you expand a little more on the "appropriate caller depth" point, as I'm not following exactly why this is required given that Check can do the same thing. Is this about API ergonomics (it's already possible, but this simplifies it), or about a functional difference?

@tmshn
Copy link
Contributor Author

tmshn commented Nov 3, 2019

@prashantv Sorry to be such vague, it's just about the API ergonomics as you said, and it's quite tiny point as you can ignore.

To avoid introducing more noice in the thread, I will clarify the detailed point in the folded section:

Currently I'm writing wrapper function like this (the code is simplified)

func (l *WrapperOfLogger) LogWithLevel(lvl zapcore.Level, msg string, fields ...Field) {
	if ce := l.logger.Check(lvl, msg); ce != nil {
		ce.Write(fields...)
	}
}

func main() {
	zapLogger, _ := zap.NewProduction()
	l := NewWrapperOfLogger(zapLogger)
	l.LogWithLevel(zapcore.Info, "hello")
}

In this case, call stack looks like

zap.Logger#check
↑
zap.Logger#Check
↑
WrapperOfLogger#LogWithLevel
↑
main

As the caller info in check skips 2, it starts from WrapperOfLogger#LogWithLevel

zap/logger.go

Lines 257 to 259 in a6015e1

// check must always be called directly by a method in the Logger interface
// (e.g., Check, Info, Fatal).
const callerSkipOffset = 2

On the other hand, with LogWithLevel directly implemented on the zap.Logger, I call LogWithLevel like this

func main() {
	l, _ := zap.NewProduction()
	l.LogWithLevel(zapcore.Info, "hello")
}

and call stack become

zap.Logger#check
↑
zap.Logger#LogWithLevel
↑
main

In this case, caller info starts from main, which is bit more preferable.

Of course, I can achieve same thing by writing Check+Write pattern in main, , so it's only about "API ergonomics".

func main() {
	l, _ := zap.NewProduction()
	if ce := l.Check(zapcore.Info, "hello"); ce != nil {
		ce.Write()
	}
}

@prashantv
Copy link
Collaborator

Thanks for the detailed explanation @tmshn. I think I have a solution for your WrapperOfLogger type, you can use WithOptions and use the AddCallerSkip option to increase the caller skip, so that when callers use your LogWithLevel function, the caller will skip your wrapped method.

func NewWrapperOfLogger(logger *zap.Logger) *WrapperOfLogger {
  return &WrapperOfLogger{logger.WithOptions(zap.AddCallerSkip(1))}
}

Since this can be solved in the wrapper, I don't think we should expand the API surface of the *Logger right now, but instead encourage those callers to use Check or use a wrapper like yours above.

@prashantv prashantv closed this Nov 4, 2019
@tmshn tmshn deleted the log-with-level branch November 5, 2019 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants