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

Take control over Kopia log level #2842

Merged
merged 7 commits into from
May 15, 2024
Merged

Take control over Kopia log level #2842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
83ecc63
When building Kopia command line take LogLevel and FileLogLevel from …
e-sumin Apr 19, 2024
2d9fe1c
Add unit test for common Kopia command line builder
e-sumin May 8, 2024
e283654
Reuse functionality of getting default value for env variable
e-sumin May 14, 2024
83be3c4
Do not use Kopia in user facing env names.
e-sumin May 14, 2024
729c5d6
Fix comment
e-sumin May 14, 2024
bb887cc
Merge remote-tracking branch 'origin/master' into add-kopia-file-log-…
e-sumin May 15, 2024
7936745
Merge branch 'master' into add-kopia-file-log-level
mergify[bot] May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 38 additions & 4 deletions pkg/kopia/command/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,48 @@
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 datastore log level.
LogLevelVarName = "DATA_STORE_LOG_LEVEL"
// FileLogLevelVarName is the environment variable that controls datastore file log level.
FileLogLevelVarName = "DATA_STORE_FILE_LOG_LEVEL"
)

func NonEmptyOrDefault[T comparable](t T, def T) T {
var empty T
if t != empty {
return t
}
return def
}

func GetEnvOrDefault(name, def string) string {
return NonEmptyOrDefault(os.Getenv(name), def)
}

// LogLevel will return either value from env or "error" as default value
func LogLevel() string {
return GetEnvOrDefault(LogLevelVarName, LogLevelError)
}

// FileLogLevel will return value from env
func FileLogLevel() string {
return os.Getenv(FileLogLevelVarName)
viveksinghggits marked this conversation as resolved.
Show resolved Hide resolved
}

Copy link
Contributor

Choose a reason for hiding this comment

The 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?
For example, we could use a generic function like nonEmptyOrDefault to handle defaults more elegantly.

Here’s an example of what this might look like:

const (
...
    DefaultLogLevel     = "info"
    DefaultFileLogLevel = "debug"
)

func nonEmptyOrDefault[T comparable](t T, def T) T {
    var empty T
    if t != empty {
        return t
    }
    return def
}

func getEnv(name, def string) string {
    return nonEmptyOrDefault(os.Getenv(name), def)
}

func LogLevel() string {
    return getEnv(LogLevelVarName, DefaultLogLevel)
}

func FileLogLevel() string {
    return getEnv(FileLogLevelVarName, DefaultFileLogLevel)
}

and after we can refactor commonArgs as:

OLD

        ...
	logLevel := cmdArgs.LogLevel
	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)
	}
	...

NEW

        ...
	logLevel := nonEmptyOrDefault(cmdArgs.LogLevel, LogLevel())
	c = c.AppendLoggableKV(logLevelFlag, logLevel)

	fileLogLevel := nonEmptyOrDefault(cmdArgs.FileLogLevel, FileLogLevel())
	c = c.AppendLoggableKV(fileLogLevelFlag, fileLogLevel)
	...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand All @@ -40,10 +72,12 @@ 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)
} else {
c = c.AppendLoggableKV(logLevelFlag, LogLevelError)
logLevel := NonEmptyOrDefault(cmdArgs.LogLevel, LogLevel())
c = c.AppendLoggableKV(logLevelFlag, logLevel)

fileLogLevel := NonEmptyOrDefault(cmdArgs.FileLogLevel, FileLogLevel())
if fileLogLevel != "" {
c = c.AppendLoggableKV(fileLogLevelFlag, fileLogLevel)
}

if cmdArgs.ConfigFilePath != "" {
Expand Down
134 changes: 134 additions & 0 deletions pkg/kopia/command/common_test.go
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests do not need to use check.v1 and could be built using the support in the plain Go testing package.

Since this is (was) a new test, wouldn't it make sense to avoid check.v1 here?

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to use testing.T.Setenv here to simplify the setup and make the test more robust, but that requires having access to testing.T.

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)
}
}
1 change: 1 addition & 0 deletions pkg/kopia/command/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const (
jsonFlag = "--json"
logDirectoryFlag = "--log-dir"
logLevelFlag = "--log-level"
fileLogLevelFlag = "--file-log-level"
LogLevelError = "error"
LogLevelInfo = "info"
parallelFlag = "--parallel"
Expand Down