feature (logging): Logging sdk key with every log message. #242
Conversation
There was a problem hiding this comment.
Given the recent activity with auth datafiles and the aforementioned request on the Java-SDK, perhaps using the SDK key is going to cause more waves than we'd like. As an alternative can we switch to supporting a configurable "instance-name" field that is added to the logger fields as opposed to the strict SDK key. Somewhat inspired by the NamedThreadFactory in the java-sdk, but also to add a bit more color to the existing logs since the current names are bit opaque since it's not even clear they originate from "optimizely" at all. Using a sequenced name like optimizey-1,optimizely-2...optimizely-n should give us the visibility we want and support a multi-instance environment without proliferating the SDK keys throughout the logs.
I think this should be a relatively minor change to the existing changes you've already made since moving to a struct-based logger was still crucial for this to implemented effectively.
| eg := utils.NewExecGroup(ctx) | ||
| appClient := &OptimizelyClient{execGroup: eg, notificationCenter: registry.GetNotificationCenter(f.SDKKey)} | ||
| eg := utils.NewExecGroup(ctx, logging.GetLogger(f.SDKKey, "ExecGroup")) | ||
| appClient := &OptimizelyClient{execGroup: eg, |
There was a problem hiding this comment.
Nit, are you running go fmt? You can add it to your IDE or run it manually.
There was a problem hiding this comment.
ran it.
There was a problem hiding this comment.
so, by default you will have a sdk key mapping of a log mapping which goes to optimizely-1,2,3, etc. To override, you would simply call UseSdkKeyForLogging(sdkKey) before creating your client factory.
| @@ -41,8 +42,10 @@ func (m *MockRequester) Get(uri string, headers ...utils.Header) (response []byt | |||
| return args.Get(0).([]byte), args.Get(1).(http.Header), args.Int(2), args.Error(3) | |||
| } | |||
|
|
|||
| var logger = logging.GetLogger("polling_manager_test", "PollingManager") | |||
There was a problem hiding this comment.
Should probably do the same thing in client_test.go
| @@ -129,6 +127,8 @@ func NewBatchEventProcessor(options ...BPOptionConfig) *BatchEventProcessor { | |||
| opt(p) | |||
| } | |||
|
|
|||
| p.logger = logging.GetLogger(p.sdkKey, "BatchEventProcessor") | |||
There was a problem hiding this comment.
Nit, can you directly to the &BatchEventProcessor{...} call above? This is more inline with how all the other loggers are set on the respective struct.
There was a problem hiding this comment.
no, because the sdk key is part of the config options. So, first I run my config options, they, I grab the sdk key, then I get the logger with the sdk key. Look at the whole method for context.
pkg/logging/level_log_consumer.go
Outdated
| messBuilder := strings.Builder{} | ||
| _, _ = messBuilder.WriteString("[") | ||
|
|
||
| _, _ = messBuilder.WriteString(level.String()) | ||
| _, _ = messBuilder.WriteString("]") | ||
| for _, v := range fields { | ||
| if s, ok := v.(string); ok { | ||
| _, _ = messBuilder.WriteString("[") | ||
| _, _ = messBuilder.WriteString(s) | ||
| _, _ = messBuilder.WriteString("]") | ||
| } | ||
| } | ||
| _, _ = messBuilder.WriteString(" ") | ||
| _, _ = messBuilder.WriteString(message) | ||
| l.logger.Println(messBuilder.String()) |
There was a problem hiding this comment.
Worth noting that some customers might not want the SDK key included by default.
optimizely/java-sdk#362
There was a problem hiding this comment.
now using optimizely-1,2,3,4,5,etc. as a mapping.
pkg/notification/manager.go
Outdated
| @@ -19,14 +19,11 @@ package notification | |||
|
|
|||
| import ( | |||
| "fmt" | |||
| "github.com/optimizely/go-sdk/pkg/logging" | |||
There was a problem hiding this comment.
Nit, should be after the builtins.
pkg/logging/level_log_consumer.go
Outdated
| message = fmt.Sprintf("[%s][%s] %s", level, fields["name"], message) | ||
| l.logger.Println(message) | ||
| messBuilder := strings.Builder{} | ||
| _, _ = messBuilder.WriteString("[") |
There was a problem hiding this comment.
"_, _" is not needed here, and errors are not checked, I think fmt.Sprintf will be cleaner here.
There was a problem hiding this comment.
at the consumer level, there is no telling how many interfaces have been added. so, a sprintf is not appropriate here.
There was a problem hiding this comment.
also, the , was a result of go lint. our linter required it.
There was a problem hiding this comment.
I think that linters complain about not checking an error. Anyway, WriteString is returning nil error all the time, so there is really no point of checking that error.
I wanted to simplify this:
_, _ = messBuilder.WriteString("[")
_, _ = messBuilder.WriteString(level.String())
_, _ = messBuilder.WriteString("]")
with
message = fmt.Sprintf("[%d]", level )
_, _ = messBuilder.WriteString(message)
maybe it's more readable
| } | ||
|
|
||
| // NewMurmurhashBucketer returns a new instance of the murmurhash bucketer | ||
| func NewMurmurhashBucketer(hashSeed uint32) *MurmurhashBucketer { | ||
| func NewMurmurhashBucketer(sdkKey string, hashSeed uint32) *MurmurhashBucketer { |
There was a problem hiding this comment.
little weird that NewMurmurhashBucketer takes sdkKey .
There was a problem hiding this comment.
good catch. at that level, it should just take a logger. fixed.
The sdk key is passed to composite classes and classes that create other objects that require logging. Small grained classes just accept a logger.
The logger logs sdk key mapped value by default along with name.
There are setters and getters for the sdk key log mapping.
Added unit test for sdk key in log.
Also updated the travis unit tests from 1.8 to 1.10. and from 1.13 to 1.14.