Add ExitFunc for FatalLevel log messages #717

FriedEgg · 2025-03-20T20:26:06Z

resolves #397

inspiration from logrus...

This change is primarily motivated by the need to facilitate easier testing in applications that frequently use Fatal(). Additionally, it could benefit scenarios where applications require custom shutdown procedures for cleanup or other special handling but still choose to rely on zerolog to perform process termination.

resolves rs#397

ccoVeille

While I understand the need, I'm afraid the implementation could be improved

Please accept my feedbacks and review

ccoVeille · 2025-03-20T20:30:30Z

globals.go

@@ -120,6 +121,9 @@ var (
 	// be thread safe and non-blocking.
 	ErrorHandler func(err error)

+	// ExitFunc is the fucntion called to exit the application, defaults to `os.Exit()`


🖊️ typo

Suggested change

// ExitFunc is the fucntion called to exit the application, defaults to `os.Exit()`

// ExitFunc is the function called to exit the application, defaults to `os.Exit()`

Please note this is pointless if you follow my later suggestion

ccoVeille · 2025-03-20T20:34:50Z

globals.go

@@ -120,6 +121,9 @@ var (
 	// be thread safe and non-blocking.
 	ErrorHandler func(err error)

+	// ExitFunc is the fucntion called to exit the application, defaults to `os.Exit()`
+	ExitFunc exitFunc = os.Exit


You are not using exitFunc except here and you didn't add a comment.

So I don't think you use this type, so I would like to suggest this

Suggested change

ExitFunc exitFunc = os.Exit

ExitFunc func(int) = os.Exit

So I'm also suggesting to delete the exitFunc type here

Please note this is pointless if you follow my later suggestion

ccoVeille · 2025-03-20T20:35:01Z

globals.go

@@ -163,6 +167,8 @@ var (
 	disableSampling = new(int32)
 )

+type exitFunc func(int)


Please see here why I suggest to remove this

#717 (comment)

Suggested change

type exitFunc func(int)

Also having an non-exported type on ExitFunc variable that is a globally exported func is a bad thing

Please note this is pointless if you follow my later suggestion

ccoVeille · 2025-03-20T20:40:14Z

globals.go

@@ -120,6 +121,9 @@ var (
 	// be thread safe and non-blocking.
 	ErrorHandler func(err error)

+	// ExitFunc is the fucntion called to exit the application, defaults to `os.Exit()`
+	ExitFunc exitFunc = os.Exit


I see a real problem here, with the implementation

You are doing this change for test purpose, but you are introducing a global exiter that anyone can overload.

It shouldn't be global and exported.

This would lead to unpredictable behavior with tests, but also with code.

Think about someone anywhere in log that would do a zerolog.ExitFunc = func(_ int) {}

That's why I'm suggesting this

ccoVeille · 2025-03-20T20:46:06Z

log.go

+		if ExitFunc == nil {
+			ExitFunc = os.Exit
+		}
+		ExitFunc(1)
 	})
 }


About the problem I raised here, here is what I suggest you to do

type ExitFunc func(int) type Logger struct { w LevelWriter level Level sampler Sampler context []byte hooks []Hook stack bool ctx context.Context exiter ExitFunc }

Suggested change

if ExitFunc == nil {

ExitFunc = os.Exit

}

ExitFunc(1)

})

}

exitFunc = l.exitFunc

if exitFunc == nil {

exitFunc = os.Exit

}

exitFunc(1)

})

}

func(l *logger) SetTestExiter(t *testing.T, exitFunc ExitFunc) {

l.exitFunc = exitFunc

t.Cleanup(func() {

l.exitFunc = nil

})

}

This is a safe way, there won't be data race, no global overlapping between packages calling module

super valid criticisms, thanks for the feedback

for the setter, it seems more inline with other functions to have a function such as the following, to return a new child Logger instance with the desired ExitFunc than to modify it?

// ExitFunc returns a logger with the e ExitFunc. func (l Logger) ExitFunc(e ExitFunc) Logger { l.exiter = e return l }

the intention wasn't to make this functionality exclusively available for testing purposes so I am not sure I see a reason to add the suggest SetTestExiter? Would having both make sense in your eyes?

Good remark and questions

Having both doesn't make sense I think

For me, having an exiter without test seems to be bad idea, as it could be used for unplanned thing.

But the fact I'm thinking that doesn't mean my voice is more valuable than anyone else.

I suggested the implementation with testing.T and t.Cleanup because it allows to create one logger and reset it after the test with a cleanup.

I feel it's safer, as the method:

is clearly identified for test purpose, as it requires a testing.T argument

cannot be used for anything else than test.

Please note I'm not against using a different name for the method I suggested, or a signature that return a new logger

func(l *Logger) ExitFunc(t *testing.T, exitFunc ExitFunc)

func(l *Logger) ExitFuncT(t *testing.T, exitFunc ExitFunc)

func(l Logger) ExitFunc(t *testing.T, exitFunc ExitFunc) Logger

func(l Logger) ExitFuncT(t *testing.T, exitFunc ExitFunc) Logger

We could also consider this

func(l Logger) ExitFunc(_ *testing.T, exitFunc ExitFunc) Logger { l.exitFunc = exitFunc return l }

Here we would only enforce the existing of testing.T

The following solution is also possible:

func(l Logger) ExitFunc(exitFunc ExitFunc) Logger { if !testing.Testing() { panic("ExitFunc can only be used for test purpose") } l.exitFunc = exitFunc return l }

I dislike panic, but I suggest it as a possible solution.

But I wouldn't mind either if you decide to use this for consistency

ExitFunc(exitFunc ExitFunc) Logger

I'm a random Gopher, jumping on your PR. I'm not a maintainer of the repository and my voice in only one among many others.

So feel fre to choose what you think to be a good solution, and let's wait for a maintainer feedback.

I have pushed some modifications based on your feedback. For the time being I have chosen to not tie the setting of l.exitFunc to testing and will wait for additional feedback from others.

@ccoVeille

address review feedback from @ccoVeille

ccoVeille · 2025-03-20T22:46:51Z

log_test.go

+		out := &bytes.Buffer{}
+		log := New(out).ExitFunc(func(_ int) {})
+		log.Fatal().Msg("")
+		if got, want := decodeIfBinaryToString(out.Bytes()), `{"level":"fatal"}`+"\n"; got != want {
+			t.Errorf("invalid log output:\n got:  %v\nwant: %v", got, want)
+		}


Adding such a thing would help to validate the exiter is called

Suggested change

out := &bytes.Buffer{}

log := New(out).ExitFunc(func(_ int) {})

log.Fatal().Msg("")

if got, want := decodeIfBinaryToString(out.Bytes()), `{"level":"fatal"}`+"\n"; got != want {

t.Errorf("invalid log output:\n got: %v\nwant: %v", got, want)

}

out := &bytes.Buffer{}

var receivedCode int

log := New(out).ExitFunc(func(code int) {

receivedCode = code

})

log.Fatal().Msg("")

if got, want := receivedCode, 1; got != want {

t.Errorf("invalid exitCode:\n got: %v\nwant: %v", got, want)

}

if got, want := decodeIfBinaryToString(out.Bytes()), `{"level":"fatal"}`+"\n"; got != want {

t.Errorf("invalid log output:\n got: %v\nwant: %v", got, want)

}

ccoVeille · 2025-03-20T22:47:49Z

log_test.go

+	t.Run("with level fatal should not exit", func(t *testing.T) {
+		out := &bytes.Buffer{}
+		log := New(out)
+		log.WithLevel(FatalLevel).Msg("")
+		if got, want := decodeIfBinaryToString(out.Bytes()), `{"level":"fatal"}`+"\n"; got != want {
+			t.Errorf("invalid log output:\n got:  %v\nwant: %v", got, want)
+		}
+	})


You are not setting the exiter

I'm unsure what you are testing here

ccoVeille · 2025-03-20T22:57:17Z

log_test.go

+		err = cmd.Wait()
+		if err == nil {
+			t.Error("Expected log Fatal to exit with non-zero status")
+		}


You should take a look at

https://pkg.go.dev/os/exec#Cmd.Wait

https://pkg.go.dev/os/exec#ExitError

https://pkg.go.dev/os#ProcessState.ExitCode

This way you could to something like this

Suggested change

err = cmd.Wait()

if err == nil {

t.Error("Expected log Fatal to exit with non-zero status")

}

err = cmd.Wait()

if err == nil {

t.Error("Expected test to fail")

}

var errExit exec.ExitError

if !errors.As(err, &errExit) {

t.Error("Expected an os.ExitError")

}

if errExit.ExitCode() != 1 {

t.Error("Expected log Fatal to exit with non-zero status")

}

rs · 2025-03-20T23:48:36Z

Thanks for the proposal.

A Fatal should not be used in most cases and certainly not in testable code.

Adding a func pointer in the logger is not something I’m willing to add for this as it adds more weight to a pass by copy type.

Sorry to disappoint but I’m not convinced this is a necessary addition.

FriedEgg · 2025-03-21T00:23:52Z

No disappointment here it can certainly be lived without especially if only for the benefit of testing.

ccoVeille · 2025-03-21T06:51:32Z

I accept the argument of memory consumption. It would add extra memory for anyone using the lib. For a feature that would be rarely used.

@FriedEgg good PR and discussion anyway.

Add ExitFunc for FatalLevel log messages

455a81f

resolves rs#397

ccoVeille suggested changes Mar 20, 2025

View reviewed changes

move ExitFunc from globals to Logger struct

e33f573

address review feedback from @ccoVeille

FriedEgg force-pushed the pr-add-exitfunc branch from 07a4d35 to e33f573 Compare March 20, 2025 22:24

ccoVeille reviewed Mar 20, 2025

View reviewed changes

FriedEgg closed this Mar 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExitFunc for FatalLevel log messages #717

Add ExitFunc for FatalLevel log messages #717

FriedEgg commented Mar 20, 2025

ccoVeille left a comment

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025

FriedEgg Mar 20, 2025

FriedEgg Mar 20, 2025

ccoVeille Mar 20, 2025 •

edited

Loading

FriedEgg Mar 20, 2025

ccoVeille Mar 20, 2025

ccoVeille Mar 20, 2025

ccoVeille Mar 20, 2025

rs commented Mar 20, 2025

FriedEgg commented Mar 21, 2025

ccoVeille commented Mar 21, 2025

	// ExitFunc is the fucntion called to exit the application, defaults to `os.Exit()`
	// ExitFunc is the function called to exit the application, defaults to `os.Exit()`

-		if ExitFunc == nil {
-			ExitFunc = os.Exit
-		}
-		ExitFunc(1)
-	})
-}
+		exitFunc = l.exitFunc
+		if exitFunc == nil {
+			exitFunc = os.Exit
+		}
+		exitFunc(1)
+ 	})
+ }
+func(l *logger) SetTestExiter(t *testing.T, exitFunc ExitFunc) {
+	l.exitFunc = exitFunc
+	t.Cleanup(func() {
+		l.exitFunc = nil
+	})
+}

Add ExitFunc for FatalLevel log messages #717

Add ExitFunc for FatalLevel log messages #717

Conversation

FriedEgg commented Mar 20, 2025

ccoVeille left a comment

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025

Choose a reason for hiding this comment

FriedEgg Mar 20, 2025

Choose a reason for hiding this comment

FriedEgg Mar 20, 2025

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

FriedEgg Mar 20, 2025

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025

Choose a reason for hiding this comment

ccoVeille Mar 20, 2025

Choose a reason for hiding this comment

rs commented Mar 20, 2025

FriedEgg commented Mar 21, 2025

ccoVeille commented Mar 21, 2025

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading

ccoVeille Mar 20, 2025 •

edited

Loading