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 F* funcs that allows use func() []Fields as arguments #1041

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

negasus
Copy link

@negasus negasus commented Dec 20, 2021

This PR add new public funcs with signature (msg string, fields ...FieldsFunc), where FieldsFunc is type FieldsFunc func() []Field

  • FDebug
  • FInfo
  • FWarn
  • FError
  • FDPanic
  • FPanic
  • FFatal

This approach allows you to perform the calculation of field values only when the message is actually displayed in the log.

An example:

func main() {
	logger, _ := zap.NewProduction()

	logger.FDebug("hello world", fields)
}

func fields() []zap.Field {
	// Here we do any heavy operations
	// and add fields for logging
	// This function is not called if the level is not appropriate
	
	return []zap.Field{
		// ...
	}
}

@negasus negasus marked this pull request as ready for review December 20, 2021 15:26
@rabbbit
Copy link
Contributor

rabbbit commented Mar 6, 2022

Just to verify my assumptions - have you considered doing the same with https://pkg.go.dev/go.uber.org/zap#Logger.Check?

I see how FInfo is a nicer/shorter format, but then to realistically make it work you'd likely need to pass in a closure, which also is not very pretty?

so the example might look like this, which is not much easier than the Check API, right?

func main() {
	logger, _ := zap.NewProduction()

        a := someStruct....
        // some work
	logger.FDebug("hello world", fields(a))
}

func fields() func() []zap.Field {
    	return func(arg someStruct) []zap.Field {
		// Here we do any heavy operations
		// and add fields for logging
		// This function is not called if the level is not appropriate
	
		return []zap.Field{
			// use arg
		}
        }
}

Note I have no impact/relation to zap maitenance, I'm asking out of my personal curiosity.

@negasus
Copy link
Author

negasus commented Mar 14, 2022

Yes, under the hood these functions use log.check. You can think of an API as syntactic sugar

@prashantv
Copy link
Collaborator

Rather than increasing the API surface for each level, I think it would be nicer to support func Lazy(fieldFunc... func() []Field) Field. The call sites would look like logger.Info("my message", zap.Lazy(field1Func, field2Func))

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.

None yet

4 participants