-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add debug logging levels #664
Conversation
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.
@idlewis Thanks for the PR. It's looking promising. Added some questions/comments.
utils/reconciler.go
Outdated
@@ -106,6 +106,8 @@ func (r *ReconcilerBase) SetDiscoveryClient(discovery discovery.DiscoveryInterfa | |||
} | |||
|
|||
var log = logf.Log.WithName("utils") | |||
// Level 1 corresponds to 'DEBUG' | |||
var logv1 = log.V(1) |
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.
Assuming this log level can be set at Operator runtime, we can support users providing the log level in the Operator config map as a next step.
utils/reconciler.go
Outdated
@@ -122,7 +124,7 @@ func (r *ReconcilerBase) CreateOrUpdate(obj client.Object, owner metav1.Object, | |||
var gvk schema.GroupVersionKind | |||
gvk, err = apiutil.GVKForObject(obj, r.scheme) | |||
if err == nil { | |||
log.Info("Reconciled", "Kind", gvk.Kind, "Namespace", obj.GetNamespace(), "Name", obj.GetName(), "Status", result) | |||
logv1.Info("Reconciled", "Kind", gvk.Kind, "Namespace", obj.GetNamespace(), "Name", obj.GetName(), "Status", result) |
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.
How many log levels are there?
Logs such as this was polluting the log file, so we should only log the overall reconcile entry and exit as Info and these ones for debug level.
So that we can read log settings from config
@leochr This is now fully working, could you take another look? It is not currently possible to update the log level dynamically, because the operator only reads config in at startup. |
This is needed as controller runtime logs some messages at V(5)
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.
@idlewis Thanks for the updates. Looks good. Added some comments.
With the log level configuration in Operator's ConfigMap, we must support dynamic updates. Configuration via env variable in Subscription would be more suited if it can only be processed at startup.
common/config.go
Outdated
@@ -17,6 +18,28 @@ const ( | |||
|
|||
// OpConfigCMCADuration default duration for cert-manager issued service certificate | |||
OpConfigCMCertDuration = "certManagerCertDuration" | |||
|
|||
// OpConfigLogLevel the level of logs to be written | |||
OpConfigLogLevel = "logLevel" |
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.
Suggest operatorLogLevel
to avoid any confusion with app log level
common/config.go
Outdated
// The allowed values for OpConfigLogLevel | ||
logLevelWarning = "warning" | ||
logLevelInfo = "info" | ||
logLevelDebug = "debug" |
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.
On second thought about the levels, I think naming similar to Liberty would be better. Suggest warning
, info
, fine
, finer
and finest
Please ignore the comments about dynamic updates. I'd completely misunderstood. |
Only log stack traces for error or higher messages, and only log them when logging level is fine/finer/finest
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.
@idlewis Looks good. Thank you
To help reduce the amount of logging done in the reconcile loop
Partially addresses OpenLiberty/open-liberty-operator#638
Adds configurable logging levels, and moves the 'Reconciled...' messages into a debug level to reduce the amount of logging produced in the default configuration.