-
Notifications
You must be signed in to change notification settings - Fork 214
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
Modify output method of loggingT to handle Logger/InfoLogger correctly #118
Modify output method of loggingT to handle Logger/InfoLogger correctly #118
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vanou The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
df859ab
to
fcbe797
Compare
Sorry @neolit123 . I have made test function. However it becomes little big. If you review it, it's very thankful. |
} | ||
} else if log != nil && s == infoLog { | ||
log.Info(string(data), "severity", severityName[s], "file", file, "line", line) |
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.
the above branching looks mostly correct to me, but i pinged the #klog channel of k8s slack for more reviewers.
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.
Thanks!
This commit modifies output method of loggingT to handle l.logr or argument log correctly, if set. And add test function of output method of loggingT. In this commit l.logr is checked first, because if loggingT.logr, which is only receiver value of output method currently, is set, it should be used unconditionally as comment of logr field of loggingT says. Till this commit applies, output method is only used with global variable logging as receiver and logging.logr/Verbose.logr as argument log. To maintain backward compatibility, in this commit, output method only checks argument s is errorLog or not when l.logr is not nil.
fcbe797
to
d2e1629
Compare
} | ||
} else if log != nil && s == infoLog { |
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.
It should technically never be possible to get into a state where log
== nil, l.logr != nil and s == errorLog
right? Because there is no Error
method on an InfoLogger. I ask because otherwise there is a risk we drop errors here 😄
This logic looks right to me :) just trying to validate my own assumptions!
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.
Thanks @munnerz .
Yes, that is underlying idea of this line. Ideally, it should not happen.
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.
Makes sense. If l.logr == nil
, log != nil
and s
is not infoLog
, it seems like log messages will be silently dropped. I'm not sure if this is a feasible situation to be in, but with the previous code we'd always log unconditionally if log != nil
. Is there a reason for the s == infoLog
check on this line?
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, as you said, if l.logr == nil
, log != nil
and s
is not infoLog
, log messages will be dropped.
I read logr
package's README
and thought it's good approach that klog
only handles infoLog
or errorLog
when logr.InfoLogger
or logr.Logger
is used. Because user of logr.InfoLogger
& logr.Logger
only expects Info or Error, I think.
So, future development of klog
should care about this logr
's idea.
As I wrote in klog_test.go
, currently this commit doesn't hurt klog
package's code. And there is no public method/function with which user can specify severity
value of output
method.
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.
Because user of logr.InfoLogger & logr.Logger only expects Info or Error, I think.
There are log levels in between (e.g. warning) that do not have direct equivalents in a logr world (as logr only has levels). The thinking is to treat any un-levelled log message that specifies a non-info level severity as an Info message and pass along the severity
as a 'key/value' in the log line (i.e. severity=warn
) (except for Errors, which are always passed to the root Logger via the Error method). The reason this severity k/v pair wasn't added when SetLogger
was originally added can be seen here: #20 (comment)
Mostly there wasn't clear consensus as to what 'keys' should be used and what the onus is on implementors of the logr
interface wrt setting these keys so other consumers can consume them reliably 😄
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.
I understand why k/v isn't added. It's hard problem, until some set of standard keys is established.
Thanks!
I'm not actually sure if this has been interpreted correctly:
The wording here was to explain that if For example, here: Lines 1223 to 1245 in d2e1629
The second argument ( Line 1170 in d2e1629
With your change, the This method should:
What's the actual incorrect behaviour you're seeing that has lead to this PR? I think I'm struggling to see the motivation for it now I've dug in more.. 😅 |
What this PR does / why we need it:
This commit modifies
output
method ofloggingT
to handlel.logr
or argumentlog
correctly, if set.In this commit
l.logr
is checked first, because ifloggingT.logr
, which is only receiver value ofoutput
method currently, is set, it should be used unconditionally as comment oflogr
field ofloggingT
says.Till this commit applies,
output
method is only used with global variablelogging
as receiver andlogging.logr
/Verbose.logr
as argumentlog
.To maintain backward compatibility, in this commit,
output
method only checks whether arguments
iserrorLog
or not whenl.logr
is notnil
.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
None
Release note:
None