Skip to content

Conversation

@morvencao
Copy link
Contributor

@morvencao morvencao commented Feb 27, 2025

ref: https://issues.redhat.com/browse/ACM-17785

This PR replaces klog with zap logger. The log levels are:

  • DebugLevel: Detailed debugging.
  • InfoLevel: General info.
  • WarnLevel: Warnings.
  • ErrorLevel: Errors that don’t stop the program.
  • DPanicLevel: Serious issues causing a panic when synced.
  • PanicLevel: Immediate panic.
  • FatalLevel: Critical errors that stop the app.

It also allows changing the log level at runtime via a ConfigMap in the maestro namespace:

# oc -n maestro get cm maestro-logging-config -o yaml
apiVersion: v1
data:
  config.yaml: |
    log_level: debug
kind: ConfigMap
metadata:
  name: maestro-logging-config
  namespace: maestro

This allows changing the log level without restarting the maestro pod.

@morvencao
Copy link
Contributor Author

/assign @clyang82

@morvencao morvencao force-pushed the br_use_zap_log branch 2 times, most recently from aaad376 to 40633ac Compare February 28, 2025 01:52
_ = d.consumerSet.Append(toAddConsumers...)
d.consumerSet.RemoveAll(toRemoveConsumers...)
log.V(4).Infof("Consumers set for current instance: %s", d.consumerSet.String())
log.Infof("Consumers set for current instance: %s", d.consumerSet.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Please lower this log level, I missed this in last change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// default log level is info
viper.SetDefault(varLogLevel, "info")
if err := viper.ReadInConfig(); err != nil {
if _, ok := err.(*os.PathError); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

will the PathError handle the non-existed file path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I did the test of no config file.

viper.SetDefault(varLogLevel, "info")
if err := viper.ReadInConfig(); err != nil {
if _, ok := err.(*os.PathError); ok {
log.Infof("no config file '%s' not found", logConfigFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove redundant not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


// InitLogger initializes the logger with the given environment.
// Must be called before using the logger.
func InitLogger(env string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we combine InitLogger and GetLogger into new GetLogger:

Suggested change
func InitLogger(env string) {
func GetLogger() *zap.SugaredLogger {
if zapLogger == nil {
zapLogLevel = zap.NewAtomicLevel()
zapConfig := zap.NewDevelopmentConfig()
zapConfig.DisableStacktrace = true
zapConfig.Encoding = "console"
if environments.GetEnvironmentStrFromEnv() == "development" {
zapLogLevel.SetLevel(zapcore.DebugLevel)
} else {
zapLogLevel.SetLevel(zapcore.InfoLevel)
}
zapLogLevel.SetLevel(zapcore.InfoLevel)
zapConfig.Level = zapLogLevel
zlog, err := zapConfig.Build()
if err != nil {
panic(err)
}
zapLogger = zlog.Sugar()
}
return zapLogger
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't merge InitLogger and zapLogger because InitLogger is called by the framework before the environment is initialized. GetLogger() is used in every package that needs to log messages. The zapLogger == nil check is necessary because, in unit tests, we don't use the framework, so it doesn't call InitLogger, in that case we initialize the logger in GetLogger().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged InitLogger() and GetLogger() as we discussed offline.

Signed-off-by: morvencao <[email protected]>
@morvencao morvencao force-pushed the br_use_zap_log branch 3 times, most recently from 1b2740c to 832a5ce Compare March 10, 2025 06:20
Signed-off-by: morvencao <[email protected]>
@clyang82
Copy link
Contributor

/ok-to-test

Copy link
Contributor

@clyang82 clyang82 left a comment

Choose a reason for hiding this comment

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

LGTM

@clyang82 clyang82 merged commit 65b11a6 into openshift-online:main Mar 10, 2025
8 checks passed
@morvencao morvencao deleted the br_use_zap_log branch March 10, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants