Take control over Kopia log level

[e-sumin]

Change Overview

When building Kopia command line take LogLevel and FileLogLevel from env variables if it not specified in parameters

Pull request type

Please check the type of change your PR introduces:

• 🚧 Work in Progress
• 🌈 Refactoring (no functional changes, no api changes)
• 🐹 Trivial/Minor
• 🐛 Bugfix
• 🌻 Feature
• 🗺️ Documentation
• 🤖 Test

Issues

• fixes #issue-number

Test Plan

• 💪 Manual
• ⚡ Unit test
• 💚 E2E

[pavannd1]

Is it possible to add these flags to one of the command unit tests we have?

[e-sumin]

Is it possible to add these flags to one of the command unit tests we have?

Good idea. I've added new unit test for common args builder.

[hairyhum]

LGTM

[plar]

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)
	...

[e-sumin]

accepted, done.

[PrasadG193]

If we are publishing these flags to users, IMO we shouldn't use KOPIA term. We can use generic a term like DATA_STORE. Users don't need to know about what tool we are using for data uploads. @pavannd1 @hairyhum @viveksinghggits wdyt?

[viveksinghggits]

yes, I remember we discussed something like this.

[e-sumin]

Thank you for pointing, renamed.

[viveksinghggits]

Just for my understand Eugen, can you please very briefly talk about what is the difference between Log Level and File Log Level?

[hairyhum]

https://kopia.io/docs/advanced/logging/#controlling-log-level

[e-sumin]

Thanks for @hairyhum for link.

Just to have an answer right here:

You can control how much data is written to console and log files by using flags:

--log-level - sets log level for console output (defaults to info)
--file-log-level - sets log level for file output (defaults to debug)

[viveksinghggits]

how is this environment variable set? should we default to something if this env var is not set?

[e-sumin]

In updated code, default value is provided.

[viveksinghggits]

do you think this requires change in the consumers of this function for example SnapshotCreate etc? Are we planning to do that in other PR?

[e-sumin]

I think it is not required.

Behavior before change:
Either level from var OR error

Behavior after change:
Either level from var OR level from env OR error

Taking level from env is needed only when debugging.
I went through the invokers and LogLevel is not set there. But if it will be set there to some value, I believe, decision of priority between env and their value should be done on their side.

[julio-lopez]

@e-sumin
+1: LG 👍 Thanks for doing this.

A couple of minor comments / questions.

[julio-lopez]

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?

[julio-lopez]

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.