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

Let user decide what to do if logger is not in context #175

Closed
wants to merge 1 commit into from
Closed

Let user decide what to do if logger is not in context #175

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 8, 2019

As for now disabledLogger is being returned if user tries to get logger from context.Context.
Such a behaviour may be beneficial, but in other cases it will be harmful.
Giving user an option to return nil and fail with nil pointer error may prevent logs loss.
This commit is not introducing any breaking change.

ctx.go Outdated Show resolved Hide resolved
@smyrman
Copy link

smyrman commented Aug 9, 2019

Failing a program because a logger is not set in context seams like a bad design to me.

Perhaps you could configure a setable defaultLogger to be used when a logger isn't found in context, and return that instead of the disabled logger. Then you can actually avoid loosing logs without updating all your code to handle the logger == nil case.

@ghost
Copy link
Author

ghost commented Aug 13, 2019

@smyrman That sounds like a good idea, I'll try it and update README if it turns out to be a solution.
UPDATE:
Ok, so since disabledLogger is package scope I basically renamed it to DefaultLogger so that user could control its behavior.

As for now disabledLogger is being returned if user tries to get logger from context.Context.
Such a behaviour may be beneficial, but in other cases it will be harmful.
Giving user an option to return nil and fail with nil pointer error may prevent logs loss.
@ghost ghost closed this Aug 13, 2019
@ghost ghost deleted the feature/strict-mode branch August 13, 2019 06:44
@gitarte
Copy link

gitarte commented Aug 13, 2019

Failing a program because a logger is not set in context seams like a bad design to me.

Perhaps you could configure a setable defaultLogger to be used when a logger isn't found in context, and return that instead of the disabled logger. Then you can actually avoid loosing logs without updating all your code to handle the logger == nil case.

Failing a program when logger is not set may be crucial in some fields:

  • banking
  • military
  • automotive

In general there where lack of log may cause puting you in jail. Please reconcider merging this pull request.

@Jarema
Copy link

Jarema commented Aug 13, 2019

@smyrman to just put my few thoughts: in some mission critical scenarios and industries, missing log, or having incomplete log may break the audit path, which might lead to really bad (even legal) consequences. So yes - not having proper logs is in some cases totally valid reason to fail the application.

@smyrman
Copy link

smyrman commented Aug 13, 2019

@Jarema and @gitarte, I don't know how commonly zerolog or even Go as a language is used in any of the situations you describe.

That said, if you can set a default logger instead of setting a strict mode, it allows for more use-cases, including yours. If you prefer your program to crash, nothing will stop you from setting a default logger that panics when used. E.g. set it to nil.

@smyrman
Copy link

smyrman commented Aug 13, 2019

See #177

This pull request was closed.
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

Successfully merging this pull request may close these issues.

4 participants