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

otelzap's Fatal(..) methods should be removed #94

Open
GeertJohan opened this issue Feb 1, 2023 · 0 comments
Open

otelzap's Fatal(..) methods should be removed #94

GeertJohan opened this issue Feb 1, 2023 · 0 comments

Comments

@GeertJohan
Copy link

GeertJohan commented Feb 1, 2023

Apologies in advance if I misunderstand or overlook a valid use-case. I have a long experience with zap and opencensus/otel, but only recently started to use the otelzap package.

I think these methods should be removed from the otelzap package:

func (l *Logger) FatalContext(ctx context.Context, msg string, fields ...zapcore.Field)
func (l LoggerWithCtx) Fatal(msg string, fields ...zapcore.Field)
func (s *SugaredLogger) FatalfContext(ctx context.Context, template string, args ...interface{})
func (s *SugaredLogger) FatalwContext(ctx context.Context, msg string, keysAndValues ...interface{})
func (s SugaredLoggerWithCtx) Fatalf(template string, args ...interface{})
func (s SugaredLoggerWithCtx) Fatalw(msg string, keysAndValues ...interface{})

The use-case for the otelzap package is that the user is in a ctx context.Context with an active span, for which the user wants to attach errors. However, the zap Fatal method os.Exit(1)'s immediately. Therefore the span is never ended and it is also never exported. This function promotes behaviour users should never really want. It'll result in fatal errors not being visible in their APM/jaeger/etc. If the user really want to Fatal, they could still use the original (*zap.Logger).Fatal(..). But it just doesn't make sense to do it with the otelzap wrapper.

Panic should still be fine, as they bubble up the stack deferred span.End() and exporter.Close() can still run.

On breaking changes: Removing these methods would be a breaking change to the otelzap package, which generally should never happen within a major version of a Go package. However, one could argue that usage of the Fatal methods must be considered as a bug in the user's code, so users must change to Panic asap, which may warrant breaking their code...???

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant