-
Notifications
You must be signed in to change notification settings - Fork 15
[log] logr.Logger and logr.LogSink Support for controller-runtime Compatibility #200
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
Changes from 4 commits
6b5ffb0
143bde6
4004679
4629a14
f21e9a0
139bc91
864ecbb
f656209
0e811ee
d73a8c5
e77cfe7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,114 @@ | ||||||
| // Copyright © 2022 - 2025 Kaleido, Inc. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| // | ||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||
| // | ||||||
| // 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 log | ||||||
|
|
||||||
| import ( | ||||||
| "context" | ||||||
|
|
||||||
| "github.com/go-logr/logr" | ||||||
| "github.com/sirupsen/logrus" | ||||||
| ) | ||||||
|
|
||||||
| // logr.LogSink duck type backed by firefly-common's logrus wrapper | ||||||
| type Sink struct { | ||||||
| name string | ||||||
| logger *logrus.Entry | ||||||
| ctx context.Context | ||||||
| } | ||||||
|
|
||||||
| // NewLogr creates a new logr.Logger backed by firefly-common's logrus wrapper | ||||||
| func NewLogr(ctx context.Context, name string) logr.Logger { | ||||||
| return logr.New(&Sink{ | ||||||
| name: name, | ||||||
| ctx: ctx, | ||||||
| logger: L(ctx), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe I should be using the private impl that |
||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| // Init initializes the sink | ||||||
| func (l *Sink) Init(_ logr.RuntimeInfo) { | ||||||
| // Optional: store callDepth if needed | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ???
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah don't think we need to implement this for now, will leave a different comment |
||||||
| } | ||||||
|
|
||||||
| func (l *Sink) Enabled(level int) bool { | ||||||
| // Map logr V-levels to ff-common levels | ||||||
| // logr: V(0)=info, V(1)=debug, V(2+)=trace | ||||||
| switch level { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check for negative level?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't be possible for logr but it is is |
||||||
| case 0: | ||||||
| return l.logger.Level >= logrus.InfoLevel | ||||||
| case 1: | ||||||
| return l.logger.Level >= logrus.DebugLevel | ||||||
| default: | ||||||
| return l.logger.Level >= logrus.TraceLevel | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Info logs an info message with the given keys and values. keysAndValues is not efficiently implemented, use WithValues instead | ||||||
| func (l *Sink) Info(level int, msg string, keysAndValues ...interface{}) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confused how with Sink we still allow passing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aaah is this implementing just the interface ?? that makes sense then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah this is all the This is just so k8s libs log through our logging, so we can use our logging in k8s controllers w/o having two different loggers going |
||||||
| logger := L(l.buildContext(keysAndValues)) | ||||||
|
|
||||||
| switch level { | ||||||
| case 0: | ||||||
| logger.Infof(msg) | ||||||
| case 1: | ||||||
| logger.Debugf(msg) | ||||||
| default: | ||||||
| logger.Tracef(msg) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Error logs an error message with the given keys and values. keysAndValues is not efficiently implemented, use WithValues instead | ||||||
| func (l *Sink) Error(err error, msg string, keysAndValues ...interface{}) { | ||||||
| logger := L(l.buildContext(keysAndValues)) | ||||||
| if err != nil { | ||||||
| logger = logger.WithError(err) | ||||||
| } | ||||||
| logger.Errorf(msg) | ||||||
| } | ||||||
|
|
||||||
| // WithValues adds the given keys and values to the logger | ||||||
| func (l *Sink) WithValues(keysAndValues ...interface{}) logr.LogSink { | ||||||
| ctx := l.buildContext(keysAndValues) | ||||||
| return &Sink{ | ||||||
| name: l.name, | ||||||
| logger: L(ctx), | ||||||
| ctx: ctx, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // WithName adds the given name to the logger | ||||||
| func (l *Sink) WithName(name string) logr.LogSink { | ||||||
| newName := l.name | ||||||
| if len(newName) > 0 { | ||||||
| newName += "." | ||||||
| } | ||||||
| newName += name | ||||||
|
|
||||||
| return &Sink{ | ||||||
| name: newName, | ||||||
| logger: l.logger.WithField("logger", newName), | ||||||
| ctx: l.ctx, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func (l *Sink) buildContext(keysAndValues []interface{}) context.Context { | ||||||
| fields := make(map[string]string) | ||||||
| for i := 0; i < len(keysAndValues); i += 2 { | ||||||
| fields[keysAndValues[i].(string)] = keysAndValues[i+1].(string) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to be a bit safer with this cast |
||||||
| } | ||||||
| return WithFields(l.ctx, fields) | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||
| // Copyright © 2022 - 2025 Kaleido, Inc. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| // | ||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||
| // | ||||||
| // 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 log | ||||||
|
|
||||||
| import ( | ||||||
| "context" | ||||||
| "errors" | ||||||
| "testing" | ||||||
| ) | ||||||
|
|
||||||
| func TestLogr(t *testing.T) { | ||||||
| ctx := context.Background() | ||||||
| logger := NewLogr(ctx, "test") | ||||||
| logger.Info("test", "key", "value") | ||||||
|
|
||||||
| subLogger := logger.WithName("sub") | ||||||
| subLogger.Info("test", "key", "value") | ||||||
|
|
||||||
| subLogger = subLogger.WithValues("key2", "value2") | ||||||
| subLogger.Info("test", "key", "value") | ||||||
|
|
||||||
| subLogger.V(4).Error(errors.New("test"), "test", "key", "value") | ||||||
| } | ||||||
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.
Should we keep the interface consistent with above and instead of a map have keyValuePairs as well?
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.
Well so this one is meant to feel more
logrusnative, but did struggle with the names to indicate that...