-
Notifications
You must be signed in to change notification settings - Fork 152
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 2 commits
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) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,134 @@ | ||
// Copyright 2024 The Kanister Authors. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package command | ||
|
||
import ( | ||
"os" | ||
|
||
"gopkg.in/check.v1" | ||
) | ||
|
||
type CommonUtilsSuite struct{} | ||
|
||
var _ = check.Suite(&CommonUtilsSuite{}) | ||
|
||
func (s *CommonUtilsSuite) TestCommonArgs(c *check.C) { | ||
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. These tests do not need to use Since this is (was) a new test, wouldn't it make sense to avoid |
||
for _, tc := range []struct { | ||
setup func() func() | ||
arg *CommandArgs | ||
expectedCmd []string | ||
comment string | ||
}{ | ||
{ | ||
setup: func() func() { return func() {} }, | ||
comment: "Default log settings", | ||
arg: &CommandArgs{ | ||
RepoPassword: "pass123", | ||
ConfigFilePath: "/tmp/config.file", | ||
LogDirectory: "/tmp/log.dir", | ||
}, | ||
expectedCmd: []string{"kopia", | ||
"--log-level=error", | ||
"--config-file=/tmp/config.file", | ||
"--log-dir=/tmp/log.dir", | ||
"--password=pass123", | ||
}, | ||
}, { | ||
setup: func() func() { return func() {} }, | ||
comment: "Custom log level passed via args, default file log level", | ||
arg: &CommandArgs{ | ||
LogLevel: "info", | ||
RepoPassword: "pass123", | ||
ConfigFilePath: "/tmp/config.file", | ||
LogDirectory: "/tmp/log.dir", | ||
}, | ||
expectedCmd: []string{"kopia", | ||
"--log-level=info", | ||
"--config-file=/tmp/config.file", | ||
"--log-dir=/tmp/log.dir", | ||
"--password=pass123", | ||
}, | ||
}, { | ||
setup: func() func() { return func() {} }, | ||
comment: "Custom log level and file log level, both passed via args", | ||
arg: &CommandArgs{ | ||
LogLevel: "info", | ||
FileLogLevel: "info", | ||
RepoPassword: "pass123", | ||
ConfigFilePath: "/tmp/config.file", | ||
LogDirectory: "/tmp/log.dir", | ||
}, | ||
expectedCmd: []string{"kopia", | ||
"--log-level=info", | ||
"--file-log-level=info", | ||
"--config-file=/tmp/config.file", | ||
"--log-dir=/tmp/log.dir", | ||
"--password=pass123", | ||
}, | ||
}, { | ||
setup: func() func() { | ||
origLogLevel := os.Getenv(LogLevelVarName) | ||
os.Setenv(LogLevelVarName, "debug") | ||
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. It'd be good to use |
||
return func() { | ||
os.Setenv(LogLevelVarName, origLogLevel) | ||
} | ||
}, | ||
comment: "Custom log level passed via env variable, file log level passed via args", | ||
arg: &CommandArgs{ | ||
FileLogLevel: "info", | ||
RepoPassword: "pass123", | ||
ConfigFilePath: "/tmp/config.file", | ||
LogDirectory: "/tmp/log.dir", | ||
}, | ||
expectedCmd: []string{"kopia", | ||
"--log-level=debug", | ||
"--file-log-level=info", | ||
"--config-file=/tmp/config.file", | ||
"--log-dir=/tmp/log.dir", | ||
"--password=pass123", | ||
}, | ||
}, { | ||
setup: func() func() { | ||
origLogLevel := os.Getenv(LogLevelVarName) | ||
origFileLogLevel := os.Getenv(FileLogLevelVarName) | ||
os.Setenv(LogLevelVarName, "debug") | ||
os.Setenv(FileLogLevelVarName, "debug") | ||
return func() { | ||
os.Setenv(LogLevelVarName, origLogLevel) | ||
os.Setenv(FileLogLevelVarName, origFileLogLevel) | ||
} | ||
}, | ||
comment: "Custom log level and file log level both passed via env variable", | ||
arg: &CommandArgs{ | ||
RepoPassword: "pass123", | ||
ConfigFilePath: "/tmp/config.file", | ||
LogDirectory: "/tmp/log.dir", | ||
}, | ||
expectedCmd: []string{"kopia", | ||
"--log-level=debug", | ||
"--file-log-level=debug", | ||
"--config-file=/tmp/config.file", | ||
"--log-dir=/tmp/log.dir", | ||
"--password=pass123", | ||
}, | ||
}, | ||
} { | ||
c.Log(tc.comment) | ||
cleanup := tc.setup() | ||
defer cleanup() | ||
cmd := stringSliceCommand(commonArgs(tc.arg)) | ||
c.Assert(cmd, check.DeepEquals, tc.expectedCmd) | ||
} | ||
} |
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.