Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

fix logger again #991

Merged
merged 2 commits into from
Aug 15, 2018
Merged

fix logger again #991

merged 2 commits into from
Aug 15, 2018

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Aug 13, 2018

as of #823 / #825, the logger was created before flags were parsed
and the config (file and env vars) were loaded.
so the logger would always get the default value for the flag
and no way to change the level..
I tried creating the logger after flag parsing, and adjusting the log
level after config parsing, but couldn't the second step to work
properly.

Simplest solution is to just print to stderr directly the one time we
actually need it in between the two steps.

PS: I hate this logging library, but we'll get rid of it. see #624

as of #823/#825, the logger was created before flags were parsed
and the config (file and env vars) were loaded.
so the logger would always get the default value for the flag
and no way to change the level..
I tried creating the logger after flag parsing, and adjusting the log
level after config parsing, but couldn't the second step to work
properly.

Simplest solution is to just print to stderr directly the one time we
actually need it in between the two steps.

PS: I hate this logging library, but we'll get rid of it. see #624
@Dieterbe
Copy link
Contributor Author

cc @Spellchaser FYI

@Dieterbe Dieterbe requested a review from woodsaj August 15, 2018 10:29
@Aergonus
Copy link
Contributor

So the logger gets the default value from the init function. Are you saying that

adjusting the log level after config parsing

doesn't work here?

I think I opted to use the logger rather than using fmt because it already had a default log level set.

@Dieterbe
Copy link
Contributor Author

I think I opted to use the logger rather than using fmt because it already had a default log level set.

problem with that is, it would use the default value from the flag definition in the init function. if you pass a custom flag, it wouldn't be applied because the flags get parsed after creating the logger.

there's various things wrong with the logging library we use. i couldn't get it to create a logger the first time (after flag parsing) and updating the level after checking the env vars/config file.

some usability problems with the library are detailed in grafana/grafana#4055 and they are the cause of that code you link to; but i think neither of us should spend the time re-investigating them

i just wanted to loop you in to see if you saw something obviously wrong here. because eventually we're switching logging libraries anyway.

@Dieterbe
Copy link
Contributor Author

Spellchaser4:01 PM
I think it’s fine. Since you plan to replace the logging mechanism anyway the quick fix works.

@Dieterbe Dieterbe merged commit e933e23 into master Aug 15, 2018
@Dieterbe Dieterbe deleted the fix-logger branch September 18, 2018 09:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants