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

Panic on logger trace level if feature disabled #316

Open
andrewbaxter opened this issue Oct 3, 2022 · 2 comments
Open

Panic on logger trace level if feature disabled #316

andrewbaxter opened this issue Oct 3, 2022 · 2 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Priority: Medium

Comments

@andrewbaxter
Copy link

andrewbaxter commented Oct 3, 2022

Could we make the code panic if the filtering is set to Trace despite Trace being disabled by flag?

I can't think of any situation where you'd want to disable Trace logging by feature but then configure a logger at Trace level, so presumably panicking in this situation wouldn't be bad.

Suggesting it after being bitten by the same "why aren't my trace logs appearing" issue noted in other issues.

@Techcable
Copy link
Member

Techcable commented Oct 10, 2022

I fear this may be a problem when loading options from config files. Remember we have different features to control debug/release builds.

It could be bad for an app config file to succeed in a debug mode (because trace is enabled) but trigger a panic in release. Conventionally, switching from debug -> release should never trigger extra panics.

How about logging a warning message instead? This would avoid a panic (in case behavior is intended) but still inform the user.

@Techcable Techcable added C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Priority: Medium labels Oct 10, 2022
@andrewbaxter
Copy link
Author

If someone uses a config that specifies trace logging and uses a release build, they're probably expecting trace logging and should either change their config (if they don't really want it) or rebuild with the flag enabled (if they do want it).

AFAICT this current behavior is optimizing for some low-touch developer workflow that involves using a single config for testing and production use rather than end user use.

switching from debug -> release should never trigger extra panics

This seems too general to me.

That said, I'd be happy with a warning here too! I imagine introducing a panic at this point might be a breaking change too, which is probably hard unless there's a reason for a major version bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Priority: Medium
Projects
None yet
Development

No branches or pull requests

2 participants