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.

Sign up for GitHub

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

Jump to bottom

Logger input Environment variables #338

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Logger input Environment variables #338

merged 1 commit into from
Mar 5, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Mar 2, 2021 •
edited
Loading

Code implements depresolver part - logger inputs
Input variables defines how will looger act.

  • propagation new config variables: LOG_FORMAT ,LOG_LEVEL, NO_COLOR
  • All covered by unit-tests
  • LOG_LEVEL accepts zerolog values panic, fatal, error,warn,info,debug,trace and ''.
    Values could be case insensitive, so PANIC, Trace are still valid values. If we don't set variable,
    then depresolver use info as default value. If LOG_LEVEL is invalid, depresolver returns an error
    so main can exit.
  • LOG_FORMAT saying how are log records printed. It accepts case insensitive values ['','json','simple'].
    Default value json. In case of invalid value returns an error so main can exit.
  • NO_COLOR is bool value indicating colored input. Variable is useful only if LOG_FORMAT == simple.
    default value is true.

Related to #331

ytsarev
ytsarev reviewed Mar 2, 2021
View reviewed changes
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got the config propagation part, the logger itself is unclear. We didn't switch to the new one yet, do we?

@kuritka
Copy link
Collaborator Author

kuritka commented Mar 2, 2021 •
edited
Loading

@ytsarev, true, that's just config propagation, nothing else.
Logger is not there yet, will be part of next PR (or should I extend this one?). I'm splitting into multiple PR's to make it more clear for reviewers, because final implementation affects almost everything.

The plan for PR's was

  • extend configuration (doesn't affect k8gb)
  • bring new logger, but keep old one (still wouldn't affect k8gb)
  • remove old logger and change all log messages to new one (affect)
  • adding debug, move some info messages into debug (affect)

@somaritane
Copy link
Contributor

I suggest to discuss possible logging solutions conceptually before we proceed with PRs

ytsarev
ytsarev previously approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan 👍 Thanks a lot for the clarification

somaritane
somaritane suggested changes Mar 2, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to stick to general log level configuration instead of using separate "debug" mode variable?
Same goes about "json" mode.
i.e. we can use:
LOGGER_FORMAT setting with json as one of the possible values,
and LOGGER_LOGLEVEL with DEBUG as one of possible values.
Each of logging solutions might also have their own concepts of configuration. We might need to investigate them.

@kuritka
Copy link
Collaborator Author

kuritka commented Mar 2, 2021

@somaritane Actually, an interesting idea. I can change:

zerolog allows for logging at the following levels (from highest to lowest), so I can take over

panic (zerolog.PanicLevel, 5)
fatal (zerolog.FatalLevel, 4)
error (zerolog.ErrorLevel, 3)
warn (zerolog.WarnLevel, 2)
info (zerolog.InfoLevel, 1)
debug (zerolog.DebugLevel, 0)
trace (zerolog.TraceLevel, -1)

LOGGER_DEBUG_MODE will be removed, while LOG_LEVEL will come and will support following values:

  • panic, fatal, error,warn,info,debug,trace
  • invalid value except undefined LOG_LEVEL will lead to fail
  • default value (LOG_LEVEL is undefined) -> info

@kuritka kuritka force-pushed the logger-depresolver branch 2 times, most recently from e090a59 to 9ea371e Compare March 2, 2021 16:57
@kuritka kuritka requested review from somaritane and ytsarev March 2, 2021 17:10
@kuritka kuritka force-pushed the logger-depresolver branch 4 times, most recently from 74312d8 to 4c2d268 Compare March 3, 2021 17:09
@kuritka
Copy link
Collaborator Author

kuritka commented Mar 3, 2021 •
edited
Loading

I removed LOGGER_JSON_MODE and exposed LOGGER_OUTPUT_FORMAT saying, how are log records printed. It accepts values ['','json','mono','color']. Default value mono; see: PR description ⬆️ .

@kuritka kuritka force-pushed the logger-depresolver branch 5 times, most recently from ff5869b to 525e616 Compare March 4, 2021 16:40
@somaritane
Copy link
Contributor

@kuritka, what about helm chart, is it out of scope for this change?

somaritane
somaritane suggested changes Mar 4, 2021
View reviewed changes
@kuritka kuritka force-pushed the logger-depresolver branch 3 times, most recently from a9129c8 to 8a4762e Compare March 5, 2021 15:20
@kuritka
Log input Environment variables
f85c9e4 
Code implements depresolver part - logger inputs
Input variables defines how will looger act.
 - propagation new config variables: ~~`LOGGER_DEBUG_MODE`~~,~~`LOGGER_JSON_MODE`~~,`LOG_FORMAT `,`LOG_LEVEL`
 - All covered by unit-tests
 - `LOG_LEVEL` accepts zerolog values `panic`, `fatal`, `error`,`warn`,`info`,`debug`,`trace` and `''`.
  Values could be  **case insensitive**, so `PANIC`, `Trace` are still valid values.  If we don't set variable,
  then depresolver use `info` as default value. If `LOG_LEVEL` is invalid, depresolver returns an error
  so `main` can exit.
 - `LOG_FORMAT` saying how are log records printed. It accepts case insensitive values `['','json','simple']`.
  Default value `json`. In case of invalid value returns an error so `main` can exit.
 - `NO_COLOR` is bool value indicating colored input. Variable is useful only if `LOG_FORMAT` == simple.
  default value is true.

Related to #331
@kuritka kuritka force-pushed the logger-depresolver branch from 8a4762e to f85c9e4 Compare March 5, 2021 15:20
@kuritka kuritka requested a review from somaritane March 5, 2021 15:33
@kuritka
Copy link
Collaborator Author

kuritka commented Mar 5, 2021

@somaritane amended

somaritane
somaritane approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@kuritka kuritka merged commit 4e05a43 into master Mar 5, 2021
@kuritka kuritka deleted the logger-depresolver branch March 5, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somaritane somaritane somaritane approved these changes

@donovanmuller donovanmuller Awaiting requested review from donovanmuller donovanmuller is a code owner

@k0da k0da Awaiting requested review from k0da k0da is a code owner

@ytsarev ytsarev Awaiting requested review from ytsarev ytsarev is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kuritka @somaritane @ytsarev