-
Notifications
You must be signed in to change notification settings - Fork 154
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
Take control over Kopia log level #2842
Changes from 1 commit
83ecc63
2d9fe1c
e283654
83be3c4
729c5d6
bb887cc
7936745
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,16 +15,34 @@ | |
package command | ||
|
||
import ( | ||
"os" | ||
|
||
"github.com/kanisterio/kanister/pkg/field" | ||
"github.com/kanisterio/kanister/pkg/log" | ||
"github.com/kanisterio/kanister/pkg/logsafe" | ||
) | ||
|
||
const ( | ||
// LogLevelVarName is the environment variable that controls Kopia log level. | ||
LogLevelVarName = "KOPIA_LOG_LEVEL" | ||
// FileLogLevelVarName is the environment variable that controls Kopia file log level. | ||
FileLogLevelVarName = "KOPIA_FILE_LOG_LEVEL" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for my understand Eugen, can you please very briefly talk about what is the difference between Log Level and File Log Level? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for @hairyhum for link. Just to have an answer right here:
|
||
) | ||
|
||
func LogLevel() string { | ||
return os.Getenv(LogLevelVarName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how is this environment variable set? should we default to something if this env var is not set? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In updated code, default value is provided. |
||
} | ||
|
||
func FileLogLevel() string { | ||
return os.Getenv(FileLogLevelVarName) | ||
viveksinghggits marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Eugen, First off, thanks for log flags! I've missed them recently (and hard-coded locally for tests) :). What do you think about encapsulating the defaulting logic within envvar utility funcs? Here’s an example of what this might look like:
and after we can refactor OLD
NEW
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. accepted, done. |
||
type CommandArgs struct { | ||
RepoPassword string | ||
ConfigFilePath string | ||
LogDirectory string | ||
LogLevel string | ||
FileLogLevel string | ||
} | ||
|
||
func bashCommand(args logsafe.Cmd) []string { | ||
|
@@ -40,12 +58,26 @@ func stringSliceCommand(args logsafe.Cmd) []string { | |
func commonArgs(cmdArgs *CommandArgs) logsafe.Cmd { | ||
c := logsafe.NewLoggable(kopiaCommand) | ||
|
||
if cmdArgs.LogLevel != "" { | ||
c = c.AppendLoggableKV(logLevelFlag, cmdArgs.LogLevel) | ||
logLevel := cmdArgs.LogLevel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think this requires change in the consumers of this function for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is not required. Behavior before change: Behavior after change: Taking level from env is needed only when debugging. |
||
if logLevel == "" { | ||
logLevel = LogLevel() | ||
} | ||
|
||
if logLevel != "" { | ||
c = c.AppendLoggableKV(logLevelFlag, logLevel) | ||
} else { | ||
c = c.AppendLoggableKV(logLevelFlag, LogLevelError) | ||
} | ||
|
||
fileLogLevel := cmdArgs.FileLogLevel | ||
if fileLogLevel == "" { | ||
fileLogLevel = FileLogLevel() | ||
} | ||
|
||
if fileLogLevel != "" { | ||
c = c.AppendLoggableKV(fileLogLevelFlag, fileLogLevel) | ||
} | ||
|
||
if cmdArgs.ConfigFilePath != "" { | ||
c = c.AppendLoggableKV(configFileFlag, cmdArgs.ConfigFilePath) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are publishing these flags to users, IMO we shouldn't use
KOPIA
term. We can use generic a term likeDATA_STORE
. Users don't need to know about what tool we are using for data uploads. @pavannd1 @hairyhum @viveksinghggits wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I remember we discussed something like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing, renamed.